Closed Bug 1388687 Opened 8 years ago Closed 8 years ago

Improve performance of recent bookmarks when removing bookmarks via the menu

Categories

(Firefox :: Bookmarks & History, enhancement, P1)

Product:
Firefox
Component:
Bookmarks & History
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: standard8, Assigned: standard8)

References

Details

(Keywords: perf, Whiteboard: [fxsearch])

Attachments

(4 files)

Bug 1388687 - Separate out the recent bookmarks menu code into its own object.
59 bytes, text/x-review-board-request
mak
: review+
Details
Bug 1388687 - Add tests for the recently bookmarked menu items.
59 bytes, text/x-review-board-request
mak
: review+
Details
Bug 1388687 - Split out inline functions into members of RecentBookmarksMenuUI to make testing/updates easier.
59 bytes, text/x-review-board-request
mak
: review+
Details
Bug 1388687 - Change the recent bookmarks object to only update the menu items when really necessary, and only do a database query when the recent bookmarks need updating.
59 bytes, text/x-review-board-request
mak
: review+
Details
On Windows/Linux, it is possible to right-click on an item in the Bookmarks menu and delete it. If a lot of items are deleted, then the performance is severely impacted - for each item removed, we rebuild the menu including re-querying the database. This is regardless of if the item was in the recent menu list or not. When I hit this on Mac (due to a different bug), it could easily add a second or so additional delay when deleting a folder with 600ish bookmarks.
Blocks: 1382966
Unfortunately this won't currently improve the case where there's lots of bookmarks being removed that are the most recent, however this should improve all the other cases.
Comment on attachment 8899512 [details] Bug 1388687 - Separate out the recent bookmarks menu code into its own object. https://reviewboard.mozilla.org/r/170788/#review176896 ::: browser/base/content/browser-places.js:1293 (Diff revision 2) > + > + hideRecentlyBookmarked() { > + Services.prefs.setBoolPref(this.RECENTLY_BOOKMARKED_PREF, false); > + }, > + > + _initRecentBookmarks(aHeaderItem, aExtraCSSClass) { Should probably lose the underscore prefix. I'd also evaluate to rename it to just init(). The same could be done for show() and hide() since RecentBookmarksMenuUI.show() is clear enough.
Attachment #8899512 - Flags: review?(mak77) → review+
Comment on attachment 8899513 [details] Bug 1388687 - Add tests for the recently bookmarked menu items. https://reviewboard.mozilla.org/r/170790/#review176912 ::: browser/components/places/tests/chrome/test_RecentBookmarksMenuUI.xul:50 (Diff revision 2) > + clearTimeout, > + clearInterval, > + }, > + }); > + var require = Loader.Require(loader, {id: ""}); > + var sinon = require("sinon-2.3.2"); is all of this loader code really necessary? Can it get a comment about what it is for?
Attachment #8899513 - Flags: review?(mak77) → review+
I am not sure, but if I understand correctly from Mark Banner's comments and code, on each item removed it is checked if it is one of the recent bookmarks , and if true, the recent bookmarks list is updated by querying all bookmarks for the recent bookmarks added (sorting all bookmarks and choosing the 5 most recent by time added or modified) . While this is an improvement over the previous state where the recent bookmarks list was updated for each item removed, this is still not optimal and unnecessary in my opinion. If a batch operation is performed like removing many bookmarks, the recent bookmarks list should be updated just once after all the items are removed (after the end of the batch or transaction). I think it doesn't need a notification for each item removed, since the bookmarks database can be queried once after the operation, and the most recent can be picked by choosing the top 5 by date added or modified . Items that were removed will not appear in the database, and therefore not appear in the most recent .
(In reply to nivtwig from comment #12) > If a batch operation is performed like removing many bookmarks, the recent > bookmarks list should be updated just once after all the items are removed > (after the end of the batch or transaction). It's all good from a theoretical point of view, unfortunately the reality of the code we have to work with and that we are trying to improve doesn't allow to easily do that. It's the usual case of ideal behavior vs reality.
Comment on attachment 8899514 [details] Bug 1388687 - Split out inline functions into members of RecentBookmarksMenuUI to make testing/updates easier. https://reviewboard.mozilla.org/r/170792/#review176958 ::: browser/base/content/browser-places.js:1301 (Diff revision 2) > Services.prefs.setBoolPref(this.RECENTLY_BOOKMARKED_PREF, false); > }, > > - _initRecentBookmarks(aHeaderItem, aExtraCSSClass) { > - this._populateRecentBookmarks(aHeaderItem, aExtraCSSClass); > + observe(subject, topic, data) { > + if (topic == "nsPref:changed") { > + if (data == this.RECENTLY_BOOKMARKED_PREF) { nit: may use && ::: browser/base/content/browser-places.js:1431 (Diff revision 2) > + showItem.parentNode.appendChild(showItem); > + hideItem.parentNode.appendChild(hideItem); > + } > + }, > + > + onItemRemoved() { we should likely define all the methods for nsINavBookmarkObserver, IIRC XPConnect complains if it can't find a method, even if we don't care about it.
Attachment #8899514 - Flags: review?(mak77) → review+
Comment on attachment 8899515 [details] Bug 1388687 - Change the recent bookmarks object to only update the menu items when really necessary, and only do a database query when the recent bookmarks need updating. https://reviewboard.mozilla.org/r/170794/#review176960 Just a few things that probably could be improved. ::: browser/base/content/browser-places.js:1386 (Diff revision 2) > + _populateRecentBookmarks(shouldShow, guidRemoved) { > + if (!shouldShow) { > + // Only clear the existing items if we were previously showing them. > + if (this.shouldShow) { > + this.shouldShow = false; > + this._clearExistingItems(); The code may be simpler is shouldShow would be a setter and would internally call _clearExistingItems and _insertRecentMenuItems. Probably you could also avoid the this.shouldshow check and do it internally. ::: browser/base/content/browser-places.js:1402 (Diff revision 2) > + return; > + } > + > + if (this.recentURIs.size == 0 || > + (guidRemoved && this.recentURIs.has(guidRemoved))) { > + this._clearExistingItems(); I don't like much this indirection: passing the guid from onItemRemoved to here. I'd rather put the code in onItemRemoved... Probably also on a timer, so that multiple onItemRemoved in a short time can be handled just once and so that we don't block onItemRemoved. Shouldn't _clearExistingItems also clear this.recentURIs? Looks like we just keep adding. ::: browser/base/content/browser-places.js:1464 (Diff revision 2) > item.setAttribute("image", icon); > item.setAttribute("loadingprincipal", loadingPrincipal); > } > item._placesNode = node; > fragment.appendChild(item); > + this.recentURIs.add(node.bookmarkGuid); should it be named recentGuids instead? recentURIs sounds confusing.
Attachment #8899515 - Flags: review?(mak77) → review-
(In reply to Marco Bonardo [::mak] from comment #13) > (In reply to nivtwig from comment #12) > > If a batch operation is performed like removing many bookmarks, the recent > > bookmarks list should be updated just once after all the items are removed > > (after the end of the batch or transaction). > > It's all good from a theoretical point of view, unfortunately the reality of > the code we have to work with and that we are trying to improve doesn't > allow to easily do that. It's the usual case of ideal behavior vs reality. What in the code doesn't allow to easily do that ? Can you give an example ?
(In reply to nivtwig from comment #16) > (In reply to Marco Bonardo [::mak] from comment #13) > > (In reply to nivtwig from comment #12) > > > If a batch operation is performed like removing many bookmarks, the recent > > > bookmarks list should be updated just once after all the items are removed > > > (after the end of the batch or transaction). > > > > It's all good from a theoretical point of view, unfortunately the reality of > > the code we have to work with and that we are trying to improve doesn't > > allow to easily do that. It's the usual case of ideal behavior vs reality. > > What in the code doesn't allow to easily do that ? > Can you give an example ? We don't currently have a reasonable way of block notifications of changes in an async world, and one of the things we want to do sometime is to rework the notifications system to support this. That's possible, but it is likely a project that is going to take a bit of time to do, and we haven't got time to do that at the moment. Hence we're improving the performance of the hot-spots that we can easily change to get the async code to a reasonable level (and the patches here have improvements that are good to have anyway) - once we've done that, we can move towards redoing the notifications. Unfortunately we can't do everything at once.
Comment on attachment 8899512 [details] Bug 1388687 - Separate out the recent bookmarks menu code into its own object. https://reviewboard.mozilla.org/r/170788/#review176896 > Should probably lose the underscore prefix. > > I'd also evaluate to rename it to just init(). > The same could be done for show() and hide() since RecentBookmarksMenuUI.show() is clear enough. Done. The show/hide are already changed in the third patch, so I'll leave those there.
Comment on attachment 8899513 [details] Bug 1388687 - Add tests for the recently bookmarked menu items. https://reviewboard.mozilla.org/r/170790/#review176912 > is all of this loader code really necessary? Can it get a comment about what it is for? I thought I had tested importing sinon via a script tag and it didn't work, so I found this and c&p it. However... it looks like importing via a script tag does work, so I've changed it to that. I'll throw it at try server again once I put updates to make sure. Note: I've also moved including sinon to the last patch of the series as that is where it is really needed.
Priority: -- → P1
Whiteboard: [fxsearch]
Status: NEW → ASSIGNED
Comment on attachment 8899515 [details] Bug 1388687 - Change the recent bookmarks object to only update the menu items when really necessary, and only do a database query when the recent bookmarks need updating. https://reviewboard.mozilla.org/r/170794/#review177858 ::: browser/base/content/browser-places.js:1321 (Diff revision 3) > - hideRecentlyBookmarked() { > - Services.prefs.setBoolPref(this.RECENTLY_BOOKMARKED_PREF, false); > + /** > + * Show or hide the recently bookmarked menu items. > + * > + * @param {Boolean} show Set to true to show the menu items, false otherwise. > + */ > + setRecentlyBookmarkedVisible(show) { what about "setVisible"? Actually, now that I see this, I wonder if we could just rename shouldShow to visible, and then you would do RecentBookmarksMenuUI.visible = true/false and use this.visible. Sounds like even more readable.
Attachment #8899515 - Flags: review?(mak77) → review+
Comment on attachment 8899515 [details] Bug 1388687 - Change the recent bookmarks object to only update the menu items when really necessary, and only do a database query when the recent bookmarks need updating. https://reviewboard.mozilla.org/r/170794/#review177858 > what about "setVisible"? > > Actually, now that I see this, I wonder if we could just rename shouldShow to visible, and then you would do RecentBookmarksMenuUI.visible = true/false and use this.visible. Sounds like even more readable. I almost merged those functions, knew I should have! I definitely like the visible suggestion, much nicer.
Pushed by mbanner@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/69df008ce954 Separate out the recent bookmarks menu code into its own object. r=mak https://hg.mozilla.org/integration/autoland/rev/b13ca9af6718 Add tests for the recently bookmarked menu items. r=mak https://hg.mozilla.org/integration/autoland/rev/96a6c21df104 Split out inline functions into members of RecentBookmarksMenuUI to make testing/updates easier. r=mak https://hg.mozilla.org/integration/autoland/rev/83dcb598eb65 Change the recent bookmarks object to only update the menu items when really necessary, and only do a database query when the recent bookmarks need updating. r=mak
Depends on: 1397545
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: