Reduce places views performance overhead

VERIFIED FIXED in Firefox 3 beta1

Status

()

Firefox
Bookmarks & History
P2
normal
VERIFIED FIXED
10 years ago
7 years ago

People

(Reporter: mano, Assigned: mano)

Tracking

({perf})

Trunk
Firefox 3 beta1
Points:
---
Dependency tree / graph
Bug Flags:
blocking-firefox3 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 10 obsolete attachments)

135.16 KB, patch
Details | Diff | Splinter Review
It's entirely possible to attach a places-view-binding just to the topmost node of each hierarchy (history menu, bookmarks menu, toolbar), and by that speed up and reduce the memory overhead of long bookmark menus / toolbar folders. We could also avoid caching rarely-used node data as as attributes.
Flags: blocking-firefox3?
Keywords: perf
Priority: -- → P2
Target Milestone: --- → Firefox 3 M9
Created attachment 284796 [details] [diff] [review]
wip
Assignee: nobody → mano
Status: NEW → ASSIGNED
OS: Mac OS X → All
Hardware: PC → All
Version: unspecified → Trunk
Created attachment 285032 [details] [diff] [review]
more...
Attachment #284796 - Attachment is obsolete: true
Created attachment 285037 [details] [diff] [review]
more...
Attachment #285032 - Attachment is obsolete: true
Blocking, but not sure that we need it done for M9.
Flags: blocking-firefox3? → blocking-firefox3+
Target Milestone: Firefox 3 M9 → ---
Given the scale of this change, I would prefer not to land it post beta 1.
Target Milestone: --- → Firefox 3 M9
Created attachment 285166 [details] [diff] [review]
more...
Attachment #285037 - Attachment is obsolete: true
Created attachment 285633 [details] [diff] [review]
more..
Attachment #285166 - Attachment is obsolete: true
Created attachment 285825 [details] [diff] [review]
v1
Attachment #285633 - Attachment is obsolete: true
Attachment #285825 - Flags: review?(sspitzer)
Blocks: 400738
Created attachment 285915 [details] [diff] [review]
v1.1
Attachment #285825 - Attachment is obsolete: true
Attachment #285915 - Flags: review?(sspitzer)
Attachment #285825 - Flags: review?(sspitzer)
Created attachment 285917 [details] [diff] [review]
v1.2

Fix chevron sub-folders
Attachment #285915 - Attachment is obsolete: true
Attachment #285915 - Flags: review?(sspitzer)
Please ignore the @@ -780,17 +778,25 @@ var PlacesStarButton = { hunk.
still reviewing / testing, but a quick question:

Mano,

       <method name="removeItem">
         <parameter name="child"/>
         <body><![CDATA[
           if (PlacesUtils.nodeIsContainer(child.node)) {
             for (var i=0; i < this._containerNodesMap.length; i++) {
               if (this._containerNodesMap[i].resultNode == child.node)
                 this._containerNodesMap.splice(i, 1);
+              break;
             }
           }
 

Shouldn't that be:

             for (var i=0; i < this._containerNodesMap.length; i++) {
-              if (this._containerNodesMap[i].resultNode == child.node)
+              if (this._containerNodesMap[i].resultNode == child.node) {
                 this._containerNodesMap.splice(i, 1);
+                break;
+              }
             }

or am I misunderstanding why you added the break?

another question, why'd you decide to stop caching the string bundle?

   /**
    * String bundle helpers
    */
-  __bundle: null,
   get _bundle() {
-    if (!this.__bundle) {
-      const PLACES_STRING_BUNDLE_URI =
+    const PLACES_STRING_BUNDLE_URI =
         "chrome://browser/locale/places/places.properties";
-      this.__bundle = Cc["@mozilla.org/intl/stringbundle;1"].
-                      getService(Ci.nsIStringBundleService).
-                      createBundle(PLACES_STRING_BUNDLE_URI);
-    }
-    return this.__bundle;
+    return this._bundle = Cc["@mozilla.org/intl/stringbundle;1"].
+                          getService(Ci.nsIStringBundleService).
+                          createBundle(PLACES_STRING_BUNDLE_URI);
   },
re removeItem: yeah...
re _bundle: I didn't... I just forgot the "delete this._bundle", see other smart-getters in PlacesUtils.
Blocks: 397117, 399472
1)

>  re removeItem: yeah...

note, that problem appears twice in your patch.

2)

>  see other smart-getters in PlacesUtils.

after reading #399930 and bug #385809, I understand now, thanks.

>  I just forgot the "delete this._bundle"

but I don't understand why the delete is necessary.  can you elaborate?

3) another review nit:

please add back the "// No context menus on menus on Mac" comment here:

+#ifndef XP_MACOSX
+        popup.setAttribute("context", "placesContext");
+#endif

4)  missing changes for browser/themes/winstripe/browser/browser.css

Created attachment 285940 [details] [diff] [review]
v1.3
Attachment #285917 - Attachment is obsolete: true
Attachment #285940 - Flags: review?(sspitzer)
"delete" removes the getter itself.
Created attachment 285946 [details] [diff] [review]
v1.4
Attachment #285940 - Attachment is obsolete: true
Attachment #285946 - Flags: review?(sspitzer)
Attachment #285940 - Flags: review?(sspitzer)
while testing, seeing a few things:

1) I have a folder on my personal toolbar with no items.  when I make the window small enough to force it to the overflow chevron, I don't get the "(Empty)" item.

2) when opening live bookmarks in the overflow chevron, I don't see the same number of elements that I see when opening livebookmarks from the personal toolbar folder (not in the overflow chevron).

the smaller the browser window, the more items I see (in the livemark folder under the overflow chevron.)

on the console, I'm getting:

JavaScript strict warning: chrome://browser/content/places/toolbar.xml, line 202
: reference to undefined property this.childNodes[i]
JavaScript error: chrome://browser/content/places/toolbar.xml, line 202: this.ch
ildNodes[i] has no properties

3)  I don't seem to have the livemark feed icon anywhere on windows (menus, toolbar, trees.)
more details:

> 2) when opening live bookmarks in the overflow chevron, I don't see the same
> number of elements that I see when opening livebookmarks from the personal
> toolbar folder (not in the overflow chevron).
>
> the smaller the browser window, the more items I see (in the livemark folder
> under the overflow chevron.)

a livebookmark to http://rss.slashdot.org/Slashdot/slashdot exhibits that problem for me (I tried with a second profile)

> JavaScript strict warning: chrome://browser/content/places/toolbar.xml, line
> 202: reference to undefined property this.childNodes[i]
> JavaScript error: chrome://browser/content/places/toolbar.xml, line 202:
> this.childNodes[i] has no properties

a livebookmark to http://planet.mozilla.org/atom.xml on the personal toolbar exhibits that problem for me (I tried with a second profile)

> 3)  I don't seem to have the livemark feed icon anywhere on windows (menus,
> toolbar, trees.)

That appears to happen only after the livemark is created, but in new windows or after restart, the icons show up.  This appears to be an existing bug, and I'll log it (or see if it is logged already)

over irc, mano gave me a fix for issues #1 and #2 in comment #19

<Mano> the fix is:
<Mano>       <method name="chevronPopupShowing">
<Mano>         <parameter name="aEvent"/>
<Mano>         <body><![CDATA[
<Mano>           var popup = aEvent.target;
<Mano>           if (popup != this._chevron.firstChild)
<Mano>             return;
<Mano> should fix both the empty/missing items issue

I've verified that it works.  thanks mano!
On this issue:

> 3)  I don't seem to have the livemark feed icon anywhere on windows (menus,
> toolbar, trees.)

That appears to happen only after the livemark is created, but in new windows
or after restart, the icons show up.  This appears to be an existing bug, and
I'll log it (or see if it is logged already)

---

This is a known issue. I've searched for it within the last day. They do appear on reload.
> This is a known issue. I've searched for it within the last day. They do appear
> on reload.

thanks for confirming, all.

I couldn't find the bug, so I logged one, see bug #401018 – upon initial creation, livemark folder looks like a regular folders in the menus and personal toolbar.

please dup if necessary.
another comment about the patch is that we're hard coding "chrome://browser/skin/places/livemarkItem.png".

but we're already doing this in other places.  there's a bug logged about this issue, see bug #397211, so we can spin off the discussion over there (and it does not need to block this bug.)
Comment on attachment 285946 [details] [diff] [review]
v1.4

r=sspitzer, assuming that mano also checks in the change he gave me over irc.
Attachment #285946 - Flags: review?(sspitzer) → review+
Target Milestone: Firefox 3 M9 → Firefox 3 M10
Comment on attachment 285946 [details] [diff] [review]
v1.4

18:19 <@Mano> beltzner: Connor sorta-kinda approved this yesterday
Attachment #285946 - Flags: approvalM9?
Attachment #285946 - Flags: approval1.9?
<Mano> beltzner: It's a rewrite of both the menu and toolbar bindings, and i'm not willing to widely test current code side-effects (and slowness)
[00:28]	<Mano>	beltzner: i could paste either the explanation i gave you here and/or mention that you called me a jerk ;)
[00:28]	* Mano	finds the later more productive

Also note the dependencies.
mconnor confirmed that he and dietrich had wanted this to be a M9 blocker, and they'd forgotten to mark it as such ... --> M9, a=endgame drivers for M9
Target Milestone: Firefox 3 M10 → Firefox 3 M9
Attachment #285946 - Flags: approvalM9?
Attachment #285946 - Flags: approvalM9+
Attachment #285946 - Flags: approval1.9?
Attachment #285946 - Flags: approval1.9+
Created attachment 286106 [details] [diff] [review]
as checked in

mozilla/browser/base/content/browser-places.js 1.57
mozilla/browser/components/places/content/controller.js 1.183
mozilla/browser/components/places/content/menu.xml 1.90
mozilla/browser/components/places/content/toolbar.xml 1.108
mozilla/browser/components/places/content/treeView.js 1.22
mozilla/browser/components/places/content/utils.js 1.75
mozilla/toolkit/components/places/public/nsINavHistoryService.idl  1.70
mozilla/toolkit/components/places/src/nsNavHistoryResult.cpp 1.121
mozilla/toolkit/components/places/src/nsNavHistoryResult.h 1.49
Attachment #285946 - Attachment is obsolete: true
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Depends on: 401075
Depends on: 401092
So, this is fixed now and it came up within QA. Is there a way to verify that this is fixed since it isn't something that can be simply tested?
Depends on: 401211

Updated

10 years ago
Depends on: 401259

Comment 32

10 years ago
Since this landed, menu behavior has changed - previously a bookmark menu longer than one screen tall would retain its position after selection, so that when I reopened the menu it would be at the position I had scrolled down to before. Now bookmark menus always return to top when opened. 

Updated

10 years ago
Depends on: 401247

Updated

10 years ago
Depends on: 401950
Depends on: 405776

Comment 33

10 years ago
@@ -650,16 +489,17 @@
           if (this.hasSelection) {
             if (PlacesUtils.nodeIsFolder(this.selectedNode)) {
               // If there is a folder selected, the insertion point is the
               // end of the folder.
               folderId = this.selectedNode.itemId;
             } else {
               // If there is another type of node selected, the insertion point
               // is after that node.
+              folderId = this.selectedNode.parent.itemId;
               index = PlacesUtils.getIndexOfNode(this.selectedNode)
             }
           }

The new line crashes when selectedNode is a bookmark in the root bookmarks folder. In such case selectedNode.parent is null, resulting in an uncaught exception (e.g. when calling PlacesController.isCommandEnabled during the check for cmd_paste).

I suggest to add a small if-not-null check

               // is after that node.
-              folderId = this.selectedNode.parent.itemId;
+              if (this.selectedNode.parent)
+                folderId = this.selectedNode.parent.itemId;
               index = PlacesUtils.getIndexOfNode(this.selectedNode)
Depends on: 413977
plenty of bake time
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.