Camino takes ages to load a 3.1mb bookmarks.plist file (huge bookmarks).

RESOLVED FIXED in Camino1.0

Status

P2
normal
RESOLVED FIXED
15 years ago
13 years ago

People

(Reporter: camino, Assigned: sfraser_bugs)

Tracking

({perf})

unspecified
Camino1.0
PowerPC
macOS

Details

Attachments

(1 attachment, 1 obsolete attachment)

157.76 KB, application/x-stuffit
Details
(Reporter)

Description

15 years ago
When using the bookmark.plist file attached in the next comment Camino will take
minutes if not tens of minutes to display bookmarks in the bookmarks bar,
bookmarks menu and bookmarks manager.

Property List editor opens it in a couple of seconds. The user claims the
Mozilla and Firefox are able to handle the original .xml file with out problems
and wishes Camino would to.

Please don't publisze the bookmarks file publically outside this bug. It's ment
for test purposes only.
(Reporter)

Comment 1

15 years ago
we need to profile this
Status: NEW → ASSIGNED
Target Milestone: --- → Camino0.9

Comment 3

15 years ago
Adding qawanted per comment 2. Mike, how does one profile? Is that something I
could do?
Keywords: qawanted
(Reporter)

Comment 4

15 years ago
*** Bug 243697 has been marked as a duplicate of this bug. ***
*** Bug 244628 has been marked as a duplicate of this bug. ***
Target Milestone: Camino0.9 → Camino0.8

Comment 6

15 years ago
Shark suggests that we are spending an inordinate amount of time in
__CFXNotificationPost while loading the large bookmark file. I checked bit, and
we are totally insane with posting notifications for bookmark updates during
loading (this is all per bookmark):
-Once when the title is set
-Once when the status is set
-Once when the number of visits is set
-Once when the icon is set (done when URL is set)
-Once when the status is set *again* (done when the URL is set)
-Possibly more that I didn't see at a glance.

So there are two things that would help. Simply skipping the setter functions,
or alternately using a flag to supress notifications during the setters then
posting one notification manually, would probably give us 5x or more improvement
in bookmark loading speed.

The better method might be to skip notifications entirely during the load
process, then do some sort of full refresh of everything that depends on those
changes all at once.

I'll try option 1 and see how much that helps.
Keywords: qawanted

Comment 7

15 years ago
Just supressing the redundant notifications only helped by a factor of about 2
(the icon notification is distinct, so it was 5->2 not 5->1). However, I found
out that there is already a system in place for all interested parties to learn
that the bookmark manager is loaded, and update toolbar/bmview/menu/whatever
appropriately.

This patch adds two things:

-One is a per-item way of temporarily suppressing updates then firing one off
when done. This is used in the redundant setting to make them roughly twice as
fast when using those functions after loading (importing bookmarks, etc.)

-The other is a bookmark-wide suppression of *all* NSNotification events that
would be sent, which is turned on during initial bookmark loading, then turned
off when finished.

My testing for just the bookmark loading function on the attached bookmark
file:
Before: 55-60 seconds
After: ~8 seconds

Unfortunatly, the subsequent filling of the bookmark menu takes 14-15 seconds,
so there's still a pretty sizable delay on loading. We should leave the bug
open for more work, but this is a substantial improvement.

Updated

15 years ago
Attachment #149544 - Flags: review?(josha)

Updated

15 years ago
Attachment #149544 - Flags: review?(qa-mozilla)

Comment 8

15 years ago
I looked over the patch and it looks great. However, it should be generated
using "diff -u" instead of whatever it is now. I'll test it more soon. Nice work.

Updated

15 years ago
Attachment #149544 - Flags: review?(josha) → review+
Attachment #149544 - Flags: review?(qa-mozilla) → superreview?(pinkerton)
Comment on attachment 149544 [details] [diff] [review]
prevent notifications during load

sr=pink

landed on trunk and branch. should this stay on the 0.8 list?
Attachment #149544 - Attachment is obsolete: true
Attachment #149544 - Flags: superreview?(pinkerton) → superreview+

Comment 10

15 years ago
Unfortunately, I don't see any obvious way to do a similar thing for the menu
constructions notifications, which is where we spend most of our time now.
|setMenuChangedMessagesEnabled| sounded promising, but doesn't seem to apply to
addItem notifications. Maybe it would be possible to create a custom
NSNotificationQueue that would do batching automatically?

Since nothing else is obvious, pushing to 0.9. Also lowering severity to normal,
since it's significantly less painful now.
Severity: major → normal
Target Milestone: Camino0.8 → Camino0.9

Comment 11

15 years ago
Pushing to 1.0 since this is not a big problem any more and there are no plans
to do more work.
Target Milestone: Camino0.9 → Camino1.0
Is this still an issue in recently nightlies?

Comment 13

14 years ago
(In reply to comment #12)
> Is this still an issue in recently nightlies?

Still an issue. I tested this with an optimized build from today and it took
about 45 seconds (on a 1ghz G4) until I could do anything after launching.

I even see this with a much smaller bookmarks file, mine is only 340kb but there
is a period of 3-5 seconds after launch when I can't do anything with the
browser (it beachballs). If I delete most of the bookmarks I can use the browser
immediately on launch. Disabling favicons knocked a few seconds off the
beachballing.. it used to be 10 seconds +.

This was the first thing I noticed after switching over from Firefox.
(In reply to comment #13)
> Still an issue. I tested this with an optimized build from today and it took
> about 45 seconds (on a 1ghz G4) until I could do anything after launching.
> 

You need to test on a non-optimized build for us to recognize it as still an
issue. Thank you.
i think simon's investigating this one.
Assignee: pinkerton → sfraser_bugs
Status: ASSIGNED → NEW
(Assignee)

Updated

14 years ago
Status: NEW → ASSIGNED
Depends on: 279994
Priority: -- → P2

Comment 16

14 years ago
(In reply to comment #14)
> You need to test on a non-optimized build for us to recognize it as still an
> issue. Thank you.

OK. Re-tested with latest official nightly. I got the same result. Still an issue.
(Assignee)

Comment 17

14 years ago
Yup, working on it. I have that 45 seconds down to about 10 or so...
(Assignee)

Comment 18

14 years ago
In brief, the issues here are:
1. writing the spotlight metadata blocks the UI thread for ~12 seconds
2. when loading site icons, every bookmark gets notified of every site icon
   load (or failure to load) (this is O(N^2))
3. we save the bookmarks file every time you go Back/Forward (because visit
   counts change)
(Assignee)

Comment 19

14 years ago
also
4.We build the entire hierarchy of bookmarks menus up-front, which takes a few 
  seconds. We should build all but the main Bookmarks menu lazily, on click.
(In reply to comment #19)
> 2. when loading site icons, every bookmark gets notified of every site icon
>    load (or failure to load) (this is O(N^2))

We should probably have the BM manager do the listening and updating instead of
individual bookmarks (I think this was mentioned earlier).

> 3. we save the bookmarks file every time you go Back/Forward (because visit
>   counts change)

Maybe we should just be incrementing things in memory and only saving the file
periodically and on quit, like (IIRC) the user defaults database does?

> 4.We build the entire hierarchy of bookmarks menus up-front, which takes a few 
>   seconds. We should build all but the main Bookmarks menu lazily, on click.

I agree, but if you mean building the submenus lazily, we may have to wait until
we drop 10.2 support, as the Cocoa methods for lazy menu population don't exist
until 10.3.
(Assignee)

Comment 21

14 years ago
(In reply to comment #20)
> > 4.We build the entire hierarchy of bookmarks menus up-front, which takes a few 
> >   seconds. We should build all but the main Bookmarks menu lazily, on click.
> 
> I agree, but if you mean building the submenus lazily, we may have to wait until
> we drop 10.2 support, as the Cocoa methods for lazy menu population don't exist
> until 10.3.

We already have code to do this for history which works on 10.2.
(Assignee)

Comment 22

14 years ago
I just checked in a large set of performance improvements here. Checkin comment:

Optimize the performance of bookmarks loading in Camino so that we launch must
faster with large bookmarks files. Changes include:
* Change from having every bookmark register for page and favicon load
notifications to having the BookmarksManger watch for loads, and then notify the
relevant bookmarks. Requires the BookmarksManager to maintain two maps, one of
page url -> bookmark, and another of favicon url -> bookmark.
* Now we build the bookmarks menus lazily on display: BookmarkMenu is now an
NSMenu subclass.
* The writing of Spotlight Metadata for bookmarks now happens on a thread, after
we've set up the rest of the bookmarks.
* We no longer save some empty keys for bookmarks and folders to the bookmarks
plist file, which reduces its size (and saving cost). Also changed some
bookmarks members from being object types (NSNumber) to primitive types.
* Delay the loading of bookmark icons until requested (rather than when the
bookmarks url is set on bookmark loading).
* Bookmark changed notifications now go out with a set of change flags, so we
know what changed and can avoid saving bookmarks for trivial changes.
* We no longer save the bookmarks when you tab between fields in the Info panel,
if you didn't change anything.
* Sundry bookmarks code cleanup for readability and leg-cocking.
* We now save custom (i.e. <link>-based) favicon URLS with bookmarks, which
allows us to avoid hitting the Necko cache for every bookmark up-front as we try
to figure out the favicon url for a bookmark.
* The site icon provider code now provides info in the "loaded" notification
about whether the load came from the network.

Now with that 3+Mb file, starting is much faster, with no pizza wheel.
Status: ASSIGNED → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED

Updated

13 years ago
Summary: Camino takes ages to load a 3.1mb bookmarks.plist file. → Camino takes ages to load a 3.1mb bookmarks.plist file (huge bookmarks).
You need to log in before you can comment on or make changes to this bug.