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

VERIFIED FIXED in mozilla0.9.6

Status

defect
P1
normal
VERIFIED FIXED
18 years ago
15 years ago

People

(Reporter: jrgmorrison, Assigned: paulkchen)

Tracking

({perf, topperf})

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(7 attachments)

(Reporter)

Description

18 years ago
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.
(Reporter)

Comment 1

18 years ago
-> pchen

Assignee: ben → pchen
(Assignee)

Comment 2

18 years ago
marking p1 and mozilla0.9.7
Status: NEW → ASSIGNED
Priority: -- → P1
Target Milestone: --- → mozilla0.9.7
(Reporter)

Comment 5

18 years ago
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

18 years ago
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.
(Reporter)

Comment 7

18 years ago
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

18 years ago
Yeah, I meant the number of top-line items in the personal toolbar that would
need to be constructed immediately.
(Assignee)

Comment 9

18 years ago
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
(Assignee)

Comment 13

18 years ago
(Assignee)

Comment 15

18 years ago
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
(Assignee)

Comment 16

18 years ago
cc-ing jag for review
(Assignee)

Comment 17

18 years ago
Sorry, more spam, moving up to mozilla0.9.6, would really like to get this in 
for mozilla0.9.6
Target Milestone: mozilla0.9.7 → mozilla0.9.6
Comment on attachment 57152 [details] [diff] [review]
use new return boolean from ReadBookmarks to determine whether or not to rebuild personal toolbar

r=jag
Attachment #57152 - Flags: review+
Comment on attachment 57153 [details] [diff] [review]
return a boolean which tells the caller if we actually loaded bookmarks or not

r=jag
Attachment #57153 - Flags: review+
Comment on attachment 57154 [details] [diff] [review]
change ReadBookmarks to return boolean

r=jag
Attachment #57154 - Flags: review+
Comment on attachment 57156 [details] [diff] [review]
add toolbarbutton to xul atom list

r=jag
Attachment #57156 - Flags: review+
Comment on attachment 57157 [details] [diff] [review]
add nsXULAtom::toolbarbutton and nsXULAtom::button to list of openable elements in IsLazyElement and IsOpen

r=jag
Attachment #57157 - Flags: review+
sr=ben@netscape.com 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. 
(Assignee)

Updated

18 years ago
Attachment #57152 - Flags: superreview+
(Assignee)

Updated

18 years ago
Attachment #57153 - Flags: superreview+
(Assignee)

Updated

18 years ago
Attachment #57154 - Flags: superreview+
(Assignee)

Updated

18 years ago
Attachment #57156 - Flags: superreview+
(Assignee)

Updated

18 years ago
Attachment #57157 - Flags: superreview+
(Assignee)

Comment 24

18 years ago
fix checked into trunk, waiting for a= to check onto 0.9.6 branch
> 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

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

Updated

18 years ago
Blocks: 49141
(Assignee)

Comment 27

18 years ago
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. ;-)
Status: ASSIGNED → RESOLVED
Last Resolved: 18 years ago
Resolution: --- → FIXED
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.]
Status: RESOLVED → VERIFIED
Product: Browser → Seamonkey
You need to log in before you can comment on or make changes to this bug.