Last Comment Bug 399729 - Reduce places views performance overhead
: Reduce places views performance overhead
Status: VERIFIED FIXED
: perf
Product: Firefox
Classification: Client Software
Component: Bookmarks & History (show other bugs)
: Trunk
: All All
: P2 normal (vote)
: Firefox 3 beta1
Assigned To: Mano (::mano, needinfo? for any questions; not reading general bugmail)
:
Mentors:
Depends on: 401950 401075 401092 401211 401247 401259 405776 413977
Blocks: 397117 399472 399476 400738
  Show dependency treegraph
 
Reported: 2007-10-13 16:58 PDT by Mano (::mano, needinfo? for any questions; not reading general bugmail)
Modified: 2010-12-17 06:21 PST (History)
10 users (show)
mbeltzner: blocking‑firefox3+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
wip (30.93 KB, patch)
2007-10-13 17:06 PDT, Mano (::mano, needinfo? for any questions; not reading general bugmail)
no flags Details | Diff | Review
more... (40.68 KB, patch)
2007-10-15 19:52 PDT, Mano (::mano, needinfo? for any questions; not reading general bugmail)
no flags Details | Diff | Review
more... (48.24 KB, patch)
2007-10-15 21:05 PDT, Mano (::mano, needinfo? for any questions; not reading general bugmail)
no flags Details | Diff | Review
more... (72.97 KB, patch)
2007-10-16 16:08 PDT, Mano (::mano, needinfo? for any questions; not reading general bugmail)
no flags Details | Diff | Review
more.. (105.77 KB, patch)
2007-10-20 19:11 PDT, Mano (::mano, needinfo? for any questions; not reading general bugmail)
no flags Details | Diff | Review
v1 (115.71 KB, patch)
2007-10-22 18:50 PDT, Mano (::mano, needinfo? for any questions; not reading general bugmail)
no flags Details | Diff | Review
v1.1 (134.53 KB, patch)
2007-10-23 13:31 PDT, Mano (::mano, needinfo? for any questions; not reading general bugmail)
no flags Details | Diff | Review
v1.2 (134.80 KB, patch)
2007-10-23 13:51 PDT, Mano (::mano, needinfo? for any questions; not reading general bugmail)
no flags Details | Diff | Review
v1.3 (135.33 KB, patch)
2007-10-23 16:29 PDT, Mano (::mano, needinfo? for any questions; not reading general bugmail)
no flags Details | Diff | Review
v1.4 (135.05 KB, patch)
2007-10-23 16:57 PDT, Mano (::mano, needinfo? for any questions; not reading general bugmail)
moco: review+
mbeltzner: approvalM9+
mbeltzner: approval1.9+
Details | Diff | Review
as checked in (135.16 KB, patch)
2007-10-24 19:04 PDT, Mano (::mano, needinfo? for any questions; not reading general bugmail)
no flags Details | Diff | Review

Description Mano (::mano, needinfo? for any questions; not reading general bugmail) 2007-10-13 16:58:13 PDT
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.
Comment 1 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2007-10-13 17:06:02 PDT
Created attachment 284796 [details] [diff] [review]
wip
Comment 2 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2007-10-15 19:52:33 PDT
Created attachment 285032 [details] [diff] [review]
more...
Comment 3 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2007-10-15 21:05:50 PDT
Created attachment 285037 [details] [diff] [review]
more...
Comment 4 Mike Beltzner [:beltzner, not reading bugmail] 2007-10-16 13:19:02 PDT
Blocking, but not sure that we need it done for M9.
Comment 5 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2007-10-16 13:21:59 PDT
Given the scale of this change, I would prefer not to land it post beta 1.
Comment 6 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2007-10-16 16:08:35 PDT
Created attachment 285166 [details] [diff] [review]
more...
Comment 7 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2007-10-20 19:11:23 PDT
Created attachment 285633 [details] [diff] [review]
more..
Comment 8 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2007-10-22 18:50:36 PDT
Created attachment 285825 [details] [diff] [review]
v1
Comment 9 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2007-10-23 13:31:41 PDT
Created attachment 285915 [details] [diff] [review]
v1.1
Comment 10 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2007-10-23 13:51:04 PDT
Created attachment 285917 [details] [diff] [review]
v1.2

Fix chevron sub-folders
Comment 11 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2007-10-23 13:52:36 PDT
Please ignore the @@ -780,17 +778,25 @@ var PlacesStarButton = { hunk.
Comment 12 (not reading, please use seth@sspitzer.org instead) 2007-10-23 15:08:09 PDT
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?

Comment 13 (not reading, please use seth@sspitzer.org instead) 2007-10-23 15:25:33 PDT
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);
   },
Comment 14 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2007-10-23 16:06:48 PDT
re removeItem: yeah...
re _bundle: I didn't... I just forgot the "delete this._bundle", see other smart-getters in PlacesUtils.
Comment 15 (not reading, please use seth@sspitzer.org instead) 2007-10-23 16:26:55 PDT
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

Comment 16 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2007-10-23 16:29:29 PDT
Created attachment 285940 [details] [diff] [review]
v1.3
Comment 17 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2007-10-23 16:32:17 PDT
"delete" removes the getter itself.
Comment 18 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2007-10-23 16:57:12 PDT
Created attachment 285946 [details] [diff] [review]
v1.4
Comment 19 (not reading, please use seth@sspitzer.org instead) 2007-10-24 09:01:16 PDT
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.)
Comment 20 (not reading, please use seth@sspitzer.org instead) 2007-10-24 11:43:30 PDT
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)

Comment 21 (not reading, please use seth@sspitzer.org instead) 2007-10-24 14:46:09 PDT
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!
Comment 22 Al Billings [:abillings] 2007-10-24 14:51:46 PDT
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.
Comment 23 (not reading, please use seth@sspitzer.org instead) 2007-10-24 14:57:48 PDT
> 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.
Comment 24 (not reading, please use seth@sspitzer.org instead) 2007-10-24 15:02:36 PDT
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 25 (not reading, please use seth@sspitzer.org instead) 2007-10-24 15:12:56 PDT
Comment on attachment 285946 [details] [diff] [review]
v1.4

r=sspitzer, assuming that mano also checks in the change he gave me over irc.
Comment 26 Mike Beltzner [:beltzner, not reading bugmail] 2007-10-24 15:22:50 PDT
Comment on attachment 285946 [details] [diff] [review]
v1.4

18:19 <@Mano> beltzner: Connor sorta-kinda approved this yesterday
Comment 27 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2007-10-24 15:29:15 PDT
<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)
Comment 28 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2007-10-24 15:30:43 PDT
[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.
Comment 29 Mike Beltzner [:beltzner, not reading bugmail] 2007-10-24 18:50:03 PDT
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
Comment 30 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2007-10-24 19:04:41 PDT
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
Comment 31 Al Billings [:abillings] 2007-10-25 13:33:05 PDT
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?
Comment 32 lucano 2007-10-26 14:05:29 PDT
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. 
Comment 33 Ronny Perinke 2007-12-07 16:41:06 PST
@@ -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)
Comment 34 Tracy Walker [:tracy] 2008-04-24 12:58:07 PDT
plenty of bake time
Comment 35 Gervase Markham [:gerv] 2009-11-26 06:58:45 PST
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

Note You need to log in before you can comment on or make changes to this bug.