Closed Bug 386159 Opened 13 years ago Closed 13 years ago

Menu-view performance improvements

Categories

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

defect

Tracking

()

RESOLVED FIXED
Firefox 3 alpha6

People

(Reporter: mano, Assigned: mano)

References

Details

Attachments

(1 file)

* We should do full rebuilds only once the menu is opened
 * We should not live complex-queries containers open (unlike folder container which are cheap)
Attached patch patchSplinter Review
Assignee: nobody → mano
Status: NEW → ASSIGNED
Attachment #270158 - Flags: review?(dietrich)
Flags: blocking-firefox3?
Priority: -- → P2
Target Milestone: --- → Firefox 3 alpha6
Summary: Menu-view performace improvements → Menu-view performance improvements
Comment on attachment 270158 [details] [diff] [review]
patch

>       <method name="onPopupShowing">
>         <body><![CDATA[
>         if (!this._resultNode)
>           this._init();
> 
>         if (!this._resultNode.containerOpen)
>-          this._resultNode.containerOpen = true;
>+          this._resultNode.containerOpen = true;  

nit: whitespace

>@@ -892,18 +912,22 @@
>         if (event.target == this)
>           this.onPopupShowing();
>       </handler>
> 
>       <handler event="popuphidden">
>         if (event.target != this)
>           return;
> 
>-        // UI performance: keep the resultnode open so we don't rebuild its
>-        // contents whenever the popup is reopened.
>+        // UI performance: folder queries are cheap, keep the resultnode open
>+        // so we don't rebuild its contents whenever the popup is reopened.
>+        if (!PlacesUtils.nodeIsFolder(this._resultNode)) {
>+          this._resultNode.containerOpen = false;
>+          this._built = false;
>+        }

won't this change make the history menu load slower as it has to do a full rebuild every time?

>Index: browser/components/places/content/toolbar.xml
>===================================================================
>RCS file: /cvsroot/mozilla/browser/components/places/content/toolbar.xml,v
>retrieving revision 1.95
>diff -u -p -8 -r1.95 toolbar.xml
>--- browser/components/places/content/toolbar.xml	28 Jun 2007 06:50:46 -0000	1.95
>+++ browser/components/places/content/toolbar.xml	28 Jun 2007 09:46:14 -0000
>@@ -624,29 +624,29 @@
>             while (parent) {
>               if (parent == container)
>                 return true;
>               parent = parent.parent;
>             }
>             return false;
>           }
> 
>-          var viewerToRebuild = null;
>+          var menuToRebuild = null;
>           for (var i=0; i < this._self._containerNodesMap.length; i++) {
>             var node = this._self._containerNodesMap[i].resultNode;
>             
>             if (node == aContainer)
>-              viewerToRebuild = this._self._containerNodesMap[i].domNode._viewer;
>+              menuToRebuild = this._self._containerNodesMap[i].domNode._viewer;
>             if (isChildOf(node, aContainer))
>               this._self._containerNodesMap.splice(i,1);
>           }
>-        
>+
>           if (aContainer.containerOpen) {
>-            if (viewerToRebuild)
>-              viewerToRebuild._self._rebuild();
>+            if (menuToRebuild)
>+              menuToRebuild._self._built = false;
>             else
>               this._self._rebuild();
>           }
>         },
> 
>         invalidateAll: function TV_V_invalidateAll() {
>           this._self._containerNodesMap.splice(0);
>           this._self._rebuild();

why this change? it's the viewer, not the menu element that the variable is referring to, right? also, why change this in the toolbar and not in the menu?
Comment on attachment 270158 [details] [diff] [review]
patch

ok, cleared up the history menu perf question w/ mano over irc. r=me, assuming the toolbar.xml naming change is removed or explained.
Attachment #270158 - Flags: review?(dietrich) → review+
dietrich>	i'm reviewing the menu change now
[19:22]	<dietrich>	why shouldn't we live-update menus for query nodes?
[19:24]	<Mano>	dietrich: expensive
[19:25]	<Mano>	dietrich: folder updates are cheap
[19:25]	<dietrich>	expensive how? high volume of viewer activity? more expensive than rebuilding?
[19:25]	<Mano>	for queries we run sql queries very often
[19:25]	<Mano>	dietrich: high sql usage.
[19:26]	<Mano>	most queries re-query on every single visit
[19:27]	<dietrich>	ugh, ok that would explain it
[19:27]	<Mano>	dietrich: given that our longest query is ten items long....
[19:28]	<dietrich>	ok, can you put that info in the bug for reference?
[19:31]	|<--	stevewon has left moznet (Quit: Leaving)
> won't this change make the history menu load slower as it has to do a full
> rebuild every time?

We can make that particular menu really fast, see bug #385397.

See also https://bugzilla.mozilla.org/show_bug.cgi?id=385397#c6 for more info about the re-query on every single visit problem.
Flags: blocking-firefox3? → blocking-firefox3+
checked in just the menu.xml parts

Checking in browser/components/places/content/menu.xml;
/cvsroot/mozilla/browser/components/places/content/menu.xml,v  <--  menu.xml
new revision: 1.79; previous revision: 1.78
done
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Flags: blocking-firefox3+ → blocking-firefox3?
Resolution: --- → FIXED
1.80 is the backout.
Status: RESOLVED → VERIFIED
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
Checking in browser/components/places/content/menu.xml;
/cvsroot/mozilla/browser/components/places/content/menu.xml,v  <--  menu.xml
new revision: 1.81; previous revision: 1.80
done
Checking in browser/components/places/content/toolbar.xml;
/cvsroot/mozilla/browser/components/places/content/toolbar.xml,v  <--  toolbar.xml
new revision: 1.96; previous revision: 1.95
done
Status: REOPENED → RESOLVED
Closed: 13 years ago13 years ago
Resolution: --- → FIXED
Flags: blocking-firefox3?
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.