107900 - window open time is significantly dependent on "size" of bookmarks.html

Status: VERIFIED FIXED

(SeaMonkey :: Bookmarks & History, defect, P1)

Description

[John Morrison]

As pchen and I noticed, a large bookmarks file will significantly slow down 
new window time (as much as 3/4 sec. on 500MHz win2k). I also confirmed with 
claudius that even a relatively small (32K, but larger than the default 
bookmarks.html [12K]) can add ~0.2 sec. on 500MHz OS X. Yoiks.

Comment 1

[John Morrison]

-> pchen

Comment 2

[Paul Chen]

marking p1 and mozilla0.9.7

Comment 3

[John Morrison]

Attachment: minimal bookmarks file; four folders in personal toolbar; no bookmarks in any folder

Comment 4

[John Morrison]

Attachment: bookmarks file; four folders in personal toolbar; 100 bookmarks in each of first two folders

Comment 5

[John Morrison]

I've attached two test bookmarks file. Each is a vanilla file which has four
folders in the Personal Toolbar Folder; the difference between them is that
the second attachment has 100 bookmarks in each of its first two folders,
while the first attachment has no bookmarks.

The first attachment (no bookmarks inside personal toolbar folders) takes
about 710 msec [in a simple test at <http://jrgm/bugs/107900/test.html>],
while the bookmarks file that has bookmarks in the toolbar folders took about
860 msec. [on win2k/500MHz/128MB; times for Mac OS 9/450MHz/256MB are
respectively 942 and 1124]

So, it seems that we are somehow enumerating the contents of these folders
when opening each and every window.

Comment 6

[Chris Waterson]

Well, in theory the only thing that should matter is the size of the personal
toolbar folder (unless you're doing this with the sidebar open to the bookmarks
panel, I guess). The bookmarks menu should not be constructed until somebody
opens it (mouses over it, to be more precise), and the same should be true of
the bookmarks button on the personal toolbar. If either of these are being built
eagerly, reassign the bug to me and I'll look at it.

Comment 7

[John Morrison]

To be sure that the sidebar was not a factor I removed all the sidebar tabs as
well as hiding the sidebar entirely when opening windows; the results were
still the same as the results above (win2k).

> Well, in theory the only thing that should matter is the size of the
> personal toolbar folder

I'm not sure what you mean by 'size of personal toolbar': the number of 
top-level folders or the total number of items contained in them?
We shouldn't be eager in constructing the contents of each PT folder.
Are we doing that?

Comment 8

[Chris Waterson]

Yeah, I meant the number of top-line items in the personal toolbar that would
need to be constructed immediately.

Comment 9

[Paul Chen]

Hyatt came to me today and said that he believes the fix would be to add toolbar
button to the list of items we never treat as open (currently menu and menubutton):
http://lxr.mozilla.org/seamonkey/source/content/xul/templates/src/nsXULContentBuilder.cpp#1402

Comment 10

[Paul Chen]

Attachment: use new return boolean from ReadBookmarks to determine whether or not to rebuild personal toolbar

Comment 11

[Paul Chen]

Attachment: return a boolean which tells the caller if we actually loaded bookmarks or not

Comment 12

[Paul Chen]

Attachment: change ReadBookmarks to return boolean

Comment 13

[Paul Chen]

Attachment: add toolbarbutton to xul atom list

Comment 14

[Paul Chen]

Attachment: add nsXULAtom::toolbarbutton and nsXULAtom::button to list of openable elements in IsLazyElement and IsOpen

Comment 15

[Paul Chen]

Ok just attached the fix plus a fix for performance regression when I did 
delayed loading of bookmarks. The fix for this particular bug is to check for 
toolbar button and button as "container" elements and don't build their content 
like we already do for menu and menubutton. This fixes the loading folders on 
the personal toolbar when building it up. That fix is contained in attachments 
57156 and 57157.

The other fix is to have ReadBookmarks return a boolean that says whether or 
not it has done the work of loading the bookmarks file. If this is true, then 
we need to rebuild the personal toolbar folder. If not, obviously, we don't 
need to rebuild the personal toolbar. This saves an extra personal toolbar 
rebuild on all navigator windows after the first one. I am such a doofus. So 
far, there aren't any other callers of ReadBookmarks() (in the mozilla 
sourcebase, that is), so as much as I hate to change the API, I think we should 
do it to get the performance win. This fix is in attachments 57152, 57153, and 
57154

Comment 16

[Paul Chen]

cc-ing jag for review

Comment 17

[Paul Chen]

Sorry, more spam, moving up to mozilla0.9.6, would really like to get this in 
for mozilla0.9.6

Comment 18

[jag (Peter Annema)]

Comment on attachment 57152 [details] [diff] [review]
use new return boolean from ReadBookmarks to determine whether or not to rebuild personal toolbar

r=jag

Comment 19

[jag (Peter Annema)]

Comment on attachment 57153 [details] [diff] [review]
return a boolean which tells the caller if we actually loaded bookmarks or not

r=jag

Comment 20

[jag (Peter Annema)]

Comment on attachment 57154 [details] [diff] [review]
change ReadBookmarks to return boolean

r=jag

Comment 21

[jag (Peter Annema)]

Comment on attachment 57156 [details] [diff] [review]
add toolbarbutton to xul atom list

r=jag

Comment 22

[jag (Peter Annema)]

Comment on attachment 57157 [details] [diff] [review]
add nsXULAtom::toolbarbutton and nsXULAtom::button to list of openable elements in IsLazyElement and IsOpen

r=jag

Comment 23

[Ben Goodger (use ben at mozilla dot org for email)]

sr=[e-mail] for the last five patches. 

A question: do we need to be concerned about updating such code whenever a
element tag name changes? I thought hyatt had some kind of XBL method called
ResolveTag that would figure out what an element /really/ was regardless of what
the derived binding said it was. Maybe that doesn't apply here. 

Comment 24

[Paul Chen]

fix checked into trunk, waiting for a= to check onto 0.9.6 branch

Comment 25

[Robert John Churchill]

> So far, there aren't any other callers of ReadBookmarks() (in the mozilla 
sourcebase, that is), so as much as I hate to change the API, I think we should 
do it to get the performance win.


Definitely a good change for the performance win.  :)  Changing ReadBookmarks()
from a void to returning a boolean is AOK anyway... it wouldn't break any JS
callsites even if there were any.  :)

Comment 26

[Keyser Sose]

*** Bug 62907 has been marked as a duplicate of this bug. ***

Comment 27

[Paul Chen]

This bug has not been approved for 0.9.6. Since I've already checked this into
the trunk, I'd say this bug is fixed. ;-)

Comment 28

[sairuh (rarely reading bugmail)]

mass-verifying claudius' Fixed bugs which haven't changed since 2001.12.31.

if you think this particular bug is not fixed, please make sure of the following
before reopening:

a. retest with a *recent* trunk build.
b. query bugzilla to see if there's an existing, open bug (new, reopened,
assigned) that covers your issue.
c. if this does need to be reopened, make sure there are specific steps to
reproduce (unless already provided and up-to-date).

thanks!

[set your search string in mail to "AmbassadorKoshNaranek" to filter out these
messages.]