Commit 4e10aaa9 authored by Rushan Suleymanov's avatar Rushan Suleymanov Committed by Commit Bot

Optimize removing all user bookmarks

While removing all user bookmarks the call site of UrlIndex::Remove is
collecting all |removed_urls|. Each next call already contained all
previously deleted urls. These |removed_urls| were used to check if
there are other bookmarks with the same URL. This led to quadratic
number of iterations over |removed_urls| as each next call iterated over
all previously deleted URLs.

This CL slightly changes behaviour of UrlIndex::Remove. It doesn't read
|removed_urls| anymore, hence it's safe to pass non-empty value.

Bug: 1096984
Change-Id: I4a9aadefe94e6becdc7c6c08ccc7b8d7b98f27f6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2362966Reviewed-by: default avatarScott Violet <sky@chromium.org>
Commit-Queue: Rushan Suleymanov <rushans@google.com>
Cr-Commit-Position: refs/heads/master@{#800271}
parent 70cda43b
......@@ -27,22 +27,6 @@ std::unique_ptr<BookmarkNode> UrlIndex::Remove(BookmarkNode* node,
std::set<GURL>* removed_urls) {
base::AutoLock url_lock(url_lock_);
RemoveImpl(node, removed_urls);
if (removed_urls) {
// RemoveImpl() adds an entry to removed_urls for each node of type URL. As
// duplicates are allowed we need to remove any entries that are still
// bookmarked.
for (auto i = removed_urls->begin(); i != removed_urls->end();) {
if (IsBookmarkedNoLock(*i)) {
// When we erase the iterator pointing at the erasee is
// invalidated, so using i++ here within the "erase" call is
// important as it advances the iterator before passing the
// old value through to erase.
removed_urls->erase(i++);
} else {
++i;
}
}
}
BookmarkNode* parent = node->parent();
return parent->Remove(size_t{parent->GetIndexOf(node)});
}
......@@ -140,7 +124,7 @@ void UrlIndex::RemoveImpl(BookmarkNode* node, std::set<GURL>* removed_urls) {
while (*i != node)
++i;
nodes_ordered_by_url_set_.erase(i);
if (removed_urls)
if (removed_urls && !IsBookmarkedNoLock(node->url()))
removed_urls->insert(node->url());
}
for (const auto& child : base::Reversed(node->children()))
......
......@@ -45,8 +45,9 @@ class UrlIndex : public HistoryBookmarkModel {
size_t index,
std::unique_ptr<BookmarkNode> node);
// Removes |node| and all its descendants from the map, returns the set of
// urls that are no longer contained in the index.
// Removes |node| and all its descendants from the map, adds urls that are no
// longer contained in the index to the |removed_urls| set if provided
// (doesn't clean up existing items in the set).
std::unique_ptr<BookmarkNode> Remove(BookmarkNode* node,
std::set<GURL>* removed_urls);
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment