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

Status: RESOLVED FIXED

(Camino Graveyard :: Bookmarks, defect, P2)

Description

[Jasper]

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.

Comment 1

[Jasper]

Attachment: the huge bookmarks.plist file

Comment 2

[Mike Pinkerton (not reading bugmail)]

we need to profile this

Comment 3

[Greg K.]

Adding qawanted per comment 2. Mike, how does one profile? Is that something I
could do?

Comment 4

[Jasper]

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

Comment 5

[Mike Pinkerton (not reading bugmail)]

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

Comment 6

[Stuart Morgan]

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.

Comment 7

[Stuart Morgan]

Attachment: prevent notifications during load

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.

Comment 8

[Josh Aas]

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.

Comment 9

[Mike Pinkerton (not reading bugmail)]

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?

Comment 10

[Stuart Morgan]

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.

Comment 11

[Josh Aas]

Pushing to 1.0 since this is not a big problem any more and there are no plans
to do more work.

Comment 12

[Samuel Sidler (old account; do not CC)]

Is this still an issue in recently nightlies?

Comment 13

[ozjason_bugmail]

(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.

Comment 14

[Samuel Sidler (old account; do not CC)]

(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.

Comment 15

[Mike Pinkerton (not reading bugmail)]

i think simon's investigating this one.

Comment 16

[ozjason_bugmail]

(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.

Comment 17

[Simon Fraser [no longer active]]

Yup, working on it. I have that 45 seconds down to about 10 or so...

Comment 18

[Simon Fraser [no longer active]]

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)

Comment 19

[Simon Fraser [no longer active]]

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.

Comment 20

[Wevah (Nate Weaver)]

(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.

Comment 21

[Simon Fraser [no longer active]]

(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.

Comment 22

[Simon Fraser [no longer active]]

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.