Closed Bug 337855 Opened 14 years ago Closed 13 years ago

Bookmarks menu painfully slow with Places build

Categories

(Firefox :: Bookmarks & History, defect, P2)

defect

Tracking

()

VERIFIED FIXED
Firefox 3 alpha6

People

(Reporter: bzbarsky, Assigned: mano)

References

Details

Attachments

(3 files, 14 obsolete files)

304.90 KB, application/bzip2
Details
48.17 KB, text/html
Details
49.38 KB, patch
Details | Diff | Splinter Review
STEPS TO REPRODUCE:

1)  Have about 300 bookmarks (I have 302).
2)  Start a Places build
3)  Open the bookmark menu.  Close it. Repeat.

ACTUAL RESULTS:  The menu takes over a second to open every single time.

EXPECTED RESULTS:  In a non-places build, the menu takes about 3/4 of a second to open the very first time it's opened.  After that it opens just as fast as all other menus.  I would expect similar performance characteristics here -- cache the info the first time, and only change it in the unlikely event of bookmarks getting edited.

ADDITIONAL INFORMATION:  Gavin tells me that we apparently rebuild the entire menu every single time; my profile confirms this as a likely possibility (lots of mutations, etc).  It's a little too big to attach to this bug, but at least 60% of the time (including dead time between succcessive openings of the menu) is spent under nsXULElement::AppendChild and nsXULElement::RemoveChild.
Well, the zip file is too big to attach, but this works...
Attachment #221924 - Attachment is patch: false
Attachment #221924 - Attachment mime type: text/plain → application/bzip2
Not in particular:

1  chrome://browser/content/places/menu.xml
_rebuild: 138-239, 16 call(s), 22277.7ms total, 1266.88ms min, 1933.71ms max, 1392.36ms avg, excluding calls: 20219.33ms total, 1174.59ms min, 1796.79ms max, 1263.71ms avg

and

2  chrome://browser/content/places/menu.xml
_cleanMenu: 93-131, 16 call(s), 1428.03ms total, 1.62ms min, 107.36ms max, 89.25ms avg, excluding calls: 1428.03ms total, 1.62ms min, 107.36ms max, 89.25ms avg
Flags: blocking1.9a2?
I put in a new view API so consumers of places results can be more efficient. This allows consumers to be notified of incremental changes and update their view dynamically. The old implementatin didn't allow this, so consumers had to re-build everything when something may have changed.

I thought there was already a bug for fixing the toolbar to do this (it rebuilds all buttons when anything in the toolbar changes, which is also really slow), but I couldn't find one. I filed bug 337868 for the toolbar.
Flags: blocking1.9a2?
Flags: blocking1.9?
Flags: blocking-firefox3?
Flags: blocking1.9?
Flags: blocking-firefox3? → blocking-firefox3+
Attached patch wip patch (obsolete) — Splinter Review
wip patch, using result view for incremental updating, only supports insert/removal of bookmark items atm.

mano, does this approach look ok?
Attachment #262281 - Flags: review?(mano)
Attached patch wip patch #2 (obsolete) — Splinter Review
Assignee: nobody → dietrich
Attachment #262281 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attached patch wip patch #2 (obsolete) — Splinter Review
w/o neil's toolbar patch included..
Attachment #262955 - Attachment is obsolete: true
taking to finish up dietrich's wip patch, per dietrich.

in related bug #337868, dietrich wrote:  "mano said this approach is fine".
Assignee: dietrich → sspitzer
Status: ASSIGNED → NEW
Attached patch wip patch #3 (obsolete) — Splinter Review
fixing some errors that prevent live bookmarks from showing child menu items.  preventing duplicate "Open <feed name>" menuitems.  still a wip.
Attachment #262956 - Attachment is obsolete: true
Attached patch wip patch #4 (obsolete) — Splinter Review
fix a problem with the history menu.  we need to heed the startMarker / endMarker values like we did before.
OS: Linux → All
Hardware: PC → All
Attached patch wip patch #5 (obsolete) — Splinter Review
update:

1) remove obsolete nosubmenus code (gone with the removal of places-popups)

2) add this code back, otherwise there are submenu problem on Mac:

+          // Container should always be open while rebuilding.
+          var wasOpen = this._resultNode.containerOpen;
+          this._resultNode.containerOpen = true;

+          // Reset the container to the same state it was in before the function was called.
+          this._resultNode.containerOpen = wasOpen;

see bug #327036 for details.

3)  added this back:

           if (PlacesUtils.nodeIsContainer(this._resultNode)) {
             this._resultNode.QueryInterface(Ci.nsINavHistoryContainerResultNode);
-            this._resultNode.containerOpen = false;
           }

dietrich, do you remember why you removed this from popuphiding in your original wip patches?
Attachment #263682 - Attachment is obsolete: true
todo:

need to figure out why when I add a new folder in the bm organizer, they don't show up in the bm menu.  I may need to ensure that for the resultNode the containerOpen is true in itemInserted().  (and for itemRemoved())  
Status: NEW → ASSIGNED
update:

> dietrich, do you remember why you removed this from popuphiding in your
> original wip patches?

I've answered this question myself, which is that once opened, we don't want to close and then re-open containers.  Otherwise, we won't get notified of item removed / opened and the menus will be out of sync.

> need to figure out why when I add a new folder in the bm organizer, they don't
> show up in the bm menu.  

This is a similar issue, and I've got a fix for this particular issue.  I'm also working on figuring out how to delay opening the container for the bm menu until the user first opens it.

updated wip coming today.
this morning I chatted with dietrich, and we came up with a slightly different approach.  I believe this approach is simpler, yet still solves the bug that boris reported.

to avoid rebuilding the menus every time, we build them once and then keep a boolean flag ("_built") to decide if the menu has been changed.  if it has been changed, we'll pay the price and rebuild it.

to determine if the menu has changed, we have a simple results view (nsINavHistoryResultViewer).  everything *but* visiting a bookmarked page will invalidate the view, but this means that we won't rebuild the menus if you just open and close them, or select a bookmark and load it (unless the title, url, or favicon change, that will invalidate the menu.)

note, unlike the previous work-in-progress patch that use nsINavHistoryResultViewer, we aren't rebuilding menus as items are inserted, removed or changed.  we just invalidate and rebuild the next time you open the menu.

also note that once we open a container and build a menupopup, we keep it open so that we'll get notifications about changes.

finally, note that for the menupopups that are built from the personal toolbar, we always rebuild the menuitems each time.  to avoid doing that, I think we would have to fix the _viewer in toolbar.xml.

additionally, this patch contains some minor code cleanup.
Comment on attachment 264330 [details] [diff] [review]
patch, a slightly different approach (per conversation with dietrich)

I need to update to use itemId (instead of bookmarkId), as a result of bug #379211

working on that now.
Attachment #264330 - Attachment is obsolete: true
Attachment #264330 - Flags: review?(mano)
Attached patch updated patch (obsolete) — Splinter Review
Attachment #264430 - Flags: review?(mano)
Comment on attachment 264430 [details] [diff] [review]
updated patch

Per discussion on /msg sspitzerMsgMe CZ tab.
Attachment #264430 - Flags: review?(mano) → review-
Comment on attachment 264823 [details] [diff] [review]
updated to trunk (folderId / asFolder() -> itemId changes)

note, this includes the fix for chevron updating (see bug #380230)
Attachment #264823 - Flags: review?(mano)
note, this also includes the fix for bug #380702
Comment on attachment 264823 [details] [diff] [review]
updated to trunk (folderId / asFolder() -> itemId changes)

As discussed, the id-based submenus array isn't going to work for query submenus.
Attachment #264823 - Flags: review?(mano) → review-
Target Milestone: --- → Firefox 3 alpha6
Attached patch another approach - wip (obsolete) — Splinter Review
Assignee: sspitzer → mano
Attachment #264823 - Attachment is obsolete: true
Attached patch more... (obsolete) — Splinter Review
Attachment #268050 - Attachment is obsolete: true
Attached patch usable (obsolete) — Splinter Review
Attachment #268076 - Attachment is obsolete: true
Attachment #268153 - Flags: review?(dietrich)
Comment on attachment 268153 [details] [diff] [review]
usable

saw this after closing the bookmarks menu:

************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "'[JavaScript Error: "XULNS is not defined" {file: "chrome://browser/content/places/menu.xml" line: 263}]' when calling method: [nsINavHistoryResultViewer::containerOpened]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "JS frame :: chrome://browser/content/places/menu.xml :: onPopupShowing :: line 49"  data: yes]
************************************************************

Index: browser/components/places/content/toolbar.xml
>===================================================================
>RCS file: /cvsroot/mozilla/browser/components/places/content/toolbar.xml,v
>retrieving revision 1.89
>diff -u -p -8 -r1.89 toolbar.xml
>--- browser/components/places/content/toolbar.xml	5 Jun 2007 23:26:27 -0000	1.89
>+++ browser/components/places/content/toolbar.xml	12 Jun 2007 23:25:43 -0000
>@@ -208,25 +208,23 @@
>           var popup = this._chevron.firstChild;
>           popup.setAttribute("type", "places");
>           // This is set here and not in the XBL constructor for the menu because
>           // it doesn't get initialized properly in the constructor.
> #ifndef XP_MACOSX
>           // No context menus on menuitems on Mac
>           popup.setAttribute("context", "placesContext");
> #endif
>-          popup._result = this._result;
>-          popup._resultNode = this._result.root;
>+          popup.place = this.place;
>           var t = this;
>-          popup.popupShowingCallback = function() {t.chevronPopupShowing();};
>+          popup.popupShowingCallback = function() { t.chevronPopupShowing();};

nit: space before end bracket as well, as below?
> 
>           // This needs to be in a timeout to make sure our boxObject has time
>           // to get its proper size
>-          var self = this;
>-          setTimeout(function() { self.updateChevron(); }, 0);
>+          setTimeout(function() { t.updateChevron(); }, 0);
>         ]]></body>
>       </method>
> 

>-        itemInserted: function MV_V_itemInserted(aParentNode, aNode, aIndex) {
>+
>+        _forwardToChildView:
>+        function TV_V__forwardToChildView(aNode, aFunction, aArguments) {
>+          for (var i=0; i < this._self._containerNodesMap.length; i++) {
>+            if (this._self._containerNodesMap[i].resultNode == aNode) {
>+              var childView = this._self._containerNodesMap[i].domNode._viewer;
>+              childView[aFunction].apply(childView, aArguments);
>+            }

is there a case in which the same container node might be in multiple places in the same tree?

also, should warn if aNode is not in the map?

>+        itemChanged: function TV_V_itemChanged(aNode) {
>           // this check can be removed once we fix bug #382397
>-          if (!aNode.parent)
>+          var parentNode = aNode.parent;
>+          if (!parentNode)
>             return;
> 

this breaks title changes to the root node. er, actually this has parity with the current bug :)

>-          // for the toolbar,
>-          // we only care if children of the root are changing  
>-          if (!PlacesUtils.nodeIsFolder(aNode.parent) ||
>-              aNode.parent != this._self.getResult().root)
>+          if (parentNode != this._self.getResult().root) {
>+            this._forwardToChildView(parentNode, "itemChanged", arguments);
>             return;
>+          }
>+
>+        containerClosed: function TV_V_containerClosed(aNode) {
>+          // if (aParentNode != this._self.getResult().root)
>+          //  this._forwardToChildView(aNode, "containerClosed", arguments);
>+          //

should either remove, or add a comment.

>@@ -175,243 +177,406 @@
>               this.removeChild(this.childNodes[i]);
>               if (this._endMarker > 0)
>                 --this._endMarker;
>             }
>           }
>           //LOG("KIDS = " + this.childNodes.length);
>         ]]></body>
>       </method>
>-      
>+
>+      <!-- Map for containerNodes<->domNodes. There's only one map per
>+           result/viewer, the field is initialized once for the root menu,
>+           for sub menus it is set to the root's map (see insertNewItem) -->
>+      <field name="_containerNodesMap">null</field>

can you add this comment to the same field in toolbar.xml?

>+          NS_ASSERT(PlacesUtils.nodeIsContainer(this._resultNode),
>+                    "Result-node of a places popup has to a container node");
>+

nit: s/to a/to be a/

>+
>+        itemChanged: function PMV_itemChanged(aNode) {
>+          // this check can be removed once we fix bug #382397
>+          var parentNode = aNode.parent;
>+          if (!parentNode)
>+            return;

same comment as above re: changing the root node title.

>+
>+        containerClosed: function PMV_containerClosed(aNode) {
>+          // if (aNode != this._self.getResultNode())
>+          //  this._forwardToChildView(aNode, "containerClosed", arguments);
>+        },

same comment as in toolbar.xml

>+        if (event.target != this)
>+          return;
>+
>+        // UI performace: keep the resultnode open so we don't rebuild its
>+        // contents whenever the popup is reopened.

s/performace/performance/

r=me given the above fixes. hrm, we really need a system for unit-testing the front-end.
Attachment #268153 - Flags: review?(dietrich) → review+
I presume this is what this bug is addressing -- simply opening and closing the Bookmarks menu is -dramatically- slower with the new Places/SQLlite system than with the old bookmarks.html. I have a lot of bookmarks organized in many folders, subfolders, and even at the top level. With pre-Places builds of Firefox/Minefield, the Bookmarks menu used to snap open essentially instantaneously, even on my 6-year-old machine. Now, there's just enough of a delay (roughly 1-1.5s) that I get irritated and aggravated every time I have to use the Bookmarks menu -- which is a -lot-. It might seem like a minor usability issue, but when you do it dozens (or hundreds) of times a day it really starts to grate on your nerves. It looks like the new patch is intended to address this, since there's really no reason the Bookmarks menu should open any slower than the old one if nothing about it has changed and it can be completely cached in memory...
> is there a case in which the same container node might be in multiple places in
> the same tree?

In complex queries, maybe. These are not yet supported by this view.
mozilla/browser/components/places/content/menu.xml 1.72
mozilla/browser/components/places/content/toolbar.xml 1.92
mozilla/browser/base/content/browser-places.js 1.37
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Depends on: 384690
Depends on: 384763
No longer depends on: 384763
to track the performance issue of the first time the menu is opened, I've logged bug #385036
Could this bug have caused a serious performance problem while scrolling?
With a certain sequence of events (go to Gmail - read a mail - click on History menu - load a long site in a tab and scroll down, then the CPU goes to 100% and the scrollbar does not follow my movements anymore for a few seconds. Can only reproduce this in my default profile.
I'd file another bug for that, Ria. (You could try only copying places.sqlite into a newly created profile and seeing if you can reproduce then.)
After a while you get also problems (delay) while typing in the locationbar without any plugins in the neighbourhood.
I can't reproduce the problem enough clear without extensions.
(In reply to comment #30)
> Created an attachment (id=268568) [details]
> as checked in
> 

(In reply to comment #31)
> mozilla/browser/components/places/content/menu.xml 1.72
> mozilla/browser/components/places/content/toolbar.xml 1.92
> mozilla/browser/base/content/browser-places.js 1.37
> 

I'm getting an ancaught exception, when a submenu of a places-generated menu should be opend.

Error: uncaught exception: [Exception... "Cannot modify properties of a WrappedNative"  nsresult: "0x80570034 (NS_ERROR_XPC_CANT_MODIFY_PROP_ON_WN)"  location: "JS frame :: chrome://browser/content/places/menu.xml :: onPopupShowing :: line 49"  data: no]

that's

        if (!this._resultNode.containerOpen)
          this._resultNode.containerOpen = true;


XUL-Code of the menu:
<menu label="History Menu">
	<menupopup type="places" asyncinit="true" place="place:group=0&amp;sort=1">
	</menupopup>
</menu>

This sorts the history by day. The menu that contains the folders for the day comes up, but when you open on of this folders and the history items should be shown, the exception is thrown.


I'll try to make a testcase to reproduce this problem because I'm using this menu as part of an extension.
Builds up to 15-06-2007 are working fine.
filled bug #337855
(In reply to comment #38)
> filled bug #337855
> 
bug #388148

sorry ;(
verified with: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a8pre) Gecko/2007080805 Minefield/3.0a8pre
Status: RESOLVED → VERIFIED
Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
You need to log in before you can comment on or make changes to this bug.