226140 - Loading bookmarks cached site icons at startup is very slow

Status: RESOLVED FIXED

(Camino Graveyard :: Bookmarks, defect)

Description

[S Woodside]

Loading of site icons from the cache at startup time is very slow.

Comment 1

[S Woodside]

... could it be faster?

Here is the code that I think handles this same task in mozilla:
http://lxr.mozilla.org/mozilla/source/xpfe/components/bookmarks/src/nsBookmarksService.cpp#4129

I believe there are some differences. For example, have a look at
RemoteDataProvider.mm RemoteURILoadManager::RequestURILoad :

  rv = NS_NewStreamLoader(getter_AddRefs(streamLoader), channel, this,
loaderContext) ; // , mLoadGroup, nsnull, loadFlags);


Why is loadFlags commented out? That is (I think) where LOAD_ONLY_FROM_CACHE
would be set, so it's maybe not being set after all. Anothing thing to notice is
that NS_NewStreamLoader (in mozilla/netwerk/base/public) isn't used anywhere
else much in Mozilla which seems odd to me.

Comment 2

[S Woodside]

It says:
http://bonsai.mozilla.org/cvslog.cgi?file=mozilla/chimera/SiteIconProvider.mm&root=/cvsroot
"Site icons that are loaded to into the necko cache. We also cache data for
missing site icons, to avoid continual refetches."

What does "missing" mean? Shouldn't it be caching data for site icons that have
already been loaded?

Comment 3

[Simon Fraser [no longer active]]

"missing" here means sites that don't have a site icon. You don't want to keep
pinging foo.com every time if it doesn't have favicon.ico; you want to remember
that it doesn't have one.

Comment 4

[S Woodside]

OK, I get it (negative cache).

Is this right? In Bookmark.mm, -(id) init  we have:
    [nc addObserver:self selector:@selector(siteIconCheck:)
name:SiteIconLoadNotificationName object:nil];

I think that every bookmark gets every notification about every site icon load
because of that (leading to O(N^2) performance). Could that be a performance hit?

Comment 5

[S Woodside]

re comment #3 : is there a cache for site icons that HAVE been loaded before?

Comment 6

[S Woodside]

Attachment: add BookmarkIconChangedNotification

BookmarkItemChangedNotification was being fired for each site icon load at
startup time. Each of those notifications caused the entire bookmarks file to
be written to disk (so hundreds of time maybe) This fixes that, and seems to be
much much faster.

Note, to test, add to your user.js, this line, and then load lots of site
icons.
user_pref("browser.chrome.favicons", true);

Comment 7

[Simon Fraser [no longer active]]

Comment on attachment 135964 [details] [diff] [review]
add BookmarkIconChangedNotification

Nice catch.

Comment 8

[S Woodside]

Comment on attachment 135964 [details] [diff] [review]
add BookmarkIconChangedNotification

setting second r?

Comment 9

[Ludovic Hirlimann [:Usul]]

Comment on attachment 135964 [details] [diff] [review]
add BookmarkIconChangedNotification


>+  //  if( [[self url] isEqualToString:[[note userInfo] objectForKey:SiteIconLoadUserDataKey]] ) {
>+      //      NSLog(@"siteIconCheck for: %@ myURL=%@", iconString, [self url]);

R+ without those two lines (and yes I know I'm nitpicking).

Comment 10

[David Haas (gone, not reading mail)]

Hi.  This patch does not fix the problem it claims to fix (slow icon load at
startup) - in fact, it will likely make the problem worse.    

It does prevent the bookmark file from being written when site icons are loaded.
 That might be not a bad fix.  Get rid of all the changes to the files
"Bookmark.mm" and "BookmarkManager.mm" and it's ready to go.  

Here's why:

comment 4: Yes, every bookmark gets every notificiation about every load. 
That's how bookmarks know to get a new icon if you a) type in an address
manually b) have more than 1 bookmark pointing at the same site (ie, multiple
Yahoo!, or Slashdot bookmarks).  I doubt it significantly affects performance
for even large bookmark files - although I'd be happy to see any sampler data
stating otherwise.

comment 6: NO, each of those changes were NOT causing the bookmark file to be
written to disk.  All bookmark changes occurring in the same pass through the
run-loop get grouped together.  Only 1 write-to-disk is triggered.  

Actually, in this case, 4 writes-to-disk were being triggered.  1 after the
toolbar icons were loaded, 1 happening ~3 seconds later when bookmarks in the
Bookmark Menu were loaded, 1 happening ~10 seconds later when the Address Book
icons were loaded, and 1 happening ~12 seconds later when the Rendezvous icons
were loaded.  

I do not believe the write-to-disk is causing the slowdown in startup
performance.  I believe it's checking the necko-cache for all the bookmark icons
and then loading them from disk.  This patch doesn't help that at all.  In fact,
it hurts it - now everything's loaded on the first pass through the run-loop
after startup, rather than spread out over ~12 seconds or so.  

If you want to make startup performance better, iterate through the BookmarkMenu
folder and do a refreshIcon on each of its subfolders at progressively longer
delay times.  That's likely to be the folder with the most items in it, and it's
the folder loaded after the shortest delay.  

Comment 11

[David Haas (gone, not reading mail)]

Attachment: different patch

I took part of Simon's patch and added in slower icon loading.	On a very very
very large bookmark file with lots of site icons, I never got an unexpected
pizza wheel of doom. (loading 12 tabs pizza wheels, but that's normal).  

I'm not going to obsolete the above patch, but I think this is a better
replacement.

Comment 12

[S Woodside]

> comment 6: NO, each of those changes were NOT causing the bookmark file 
> to be written to disk.  All bookmark changes occurring in the same pass 
> through the run-loop get grouped together.  Only 1 write-to-disk is triggered. 

What? 

Every BookmarkItemChangedNotification causes it to post a
WriteBookmarkNotification. Then it executes this code (once for each bookmark):
[dict writeToFile:[pathToFile stringByStandardizingPath] atomically:YES]

That happens once for each bookmark that changes.

OK, though, I agree that my patch didn't seem to completely fix the problem
(although I think it helped). I'll try your patch.

Comment 13

[David Haas (gone, not reading mail)]

comment 12: 

>Every BookmarkItemChangedNotification causes it to post a
>WriteBookmarkNotification. Then it executes this code (once for each bookmark):
>[dict writeToFile:[pathToFile stringByStandardizingPath] atomically:YES]

Nope.  NSNotificationQueue != NSNotificationCenter.  The difference is rather
important here.  

Comment 14

[S Woodside]

Dave your patch is a bit messed up, I had to delete part of the path prefix on
the first file to get it to apply (the prefixes are different for different files).

Comment 15

[S Woodside]

Attachment: different patch (fixed)

Dave's patch with fixed patch headers.

Comment 16

[S Woodside]

Comment on attachment 136688 [details] [diff] [review]
different patch (fixed)

I like this patch. It fixes the actual problem. Also it also avoid the
excessive unecessarry writes (which I'm not convinced are batched, Dave)

Comment 17

[Mike Pinkerton (not reading bugmail)]

last patch landed.