We want to be able to tune the memory characteristics of the imglib cache more carefully than necko's in-memory cache lets us. This bug will be tracking that work. This is a WIP patch that compiles and runs, but isn't complete by any means.

It turns out that, in order to make imgCache smart enough to do what we want, it's necessary to make it know too much about what's going on with the request. Instead, take imgCache and smash it together with imgLoader in a particle accelerator, and see what we're left with. This patch, as the last, compiles and works minimally. Next up: expiring things from the cache.

P1.

This is an almost-ready-for-prime-time patch. Previous iterations seriously regressed Tp; this one is at least even with current CVS trunk (which is the only thing I can test against because of the TryServer), and might be a slight benefit (~10 ms on one platform). Still remaining to do before asking for review is (potentially) splitting out some of the caching-specific clases into their own file (for cleanliness), and adding prefs for cache size and size/date weighting.

Various problems, most notably an enormous leak (we never freed any cache entries ever), caused the previous patch to regress Tp -- first by about 5x, and then by 2x. This patch should fix the issues, and is currently cycling through the try server. If all goes well, I'll ask for first review shortly.

Comment on attachment 329733 [details] [diff] [review] Fix the problems in the previous patch that killed Tp Hooray! Code complete. On Tp, it saves 35ms on Linux (and is basically even on OS X and Windows), and saves several megabytes of memory.

GO JOE!

This patch should be the final one, barring any changes needed for review. It uses nsExpirationTracker to ensure that we free up caches if they haven't been used in a little while (10 seconds). It also exposes a bug in nsExpirationTracker, bug 449543.

Comment on attachment 332677 [details] [diff] [review] Use nsExpirationTracker to expire cache elements by time I guess it's not actually a WIP patch!

Joe if you kickoff a mac try server build and hand it to me I'll re-run the memory tests on it...

<3 stuart

Pushed to mozilla-central: http://hg.mozilla.org/mozilla-central/index.cgi/rev/e63a23edb90c

Reverted due to Rlk regression: http://hg.mozilla.org/mozilla-central/index.cgi/rev/2c020a9f03c1

When this patch was initially landed, leaks were found by the various tests. This updated patch fixes those leaks.

This is still causing shutdown leaks. In particular, we leak an expiration tracker's timer, and thence various other stuff. Stack for the timer to the place where we stick it into its comptr: nsCOMPtr<nsITimer>::assign_assuming_AddRef(nsITimer*) (/usr/include/libkern/i386/_OSByteOrder.h:) nsCOMPtr<nsITimer>::assign_from_helper(nsCOMPtr_helper const&, nsID const&) (/usr/include/libkern/i386/_OSByteOrder.h:) nsCOMPtr<nsITimer>::operator=(nsCOMPtr_helper const&) (/usr/include/libkern/i386/_OSByteOrder.h:) nsExpirationTracker<imgCacheEntry, 3u>::CheckStartTimer() (/usr/include/libkern/i386/_OSByteOrder.h:) nsExpirationTracker<imgCacheEntry, 3u>::AddObject(imgCacheEntry*) (/usr/include/libkern/i386/_OSByteOrder.h:) imgLoader::PutIntoCache(nsIURI*, imgCacheEntry*) (/usr/include/libkern/i386/_OSByteOrder.h:) I suspect that we need to clear out the cache at xpcom shutodnw or earlier (check with bsmedberg on the right notification) and need to get rid of the expiration tracker at that point.

You probably want to add this to the next patch iteration, by the way.

(In reply to comment #18) > Created an attachment (id=336872) [details] > More refcount logging > > You probably want to add this to the next patch iteration, by the way. I'll add it, but somehow I did get refcount logging for imgCacheEntry without it. I presume it was through nsTraceRefcnt.

This patch makes Firefox run refcount-tracing clean. (It seems that Shutdown() is called at an appropriate time.) When I made my last patch, I didn't know that we were 100% clean on refcount tracing, and since I didn't see any of my classes in that list, I assumed I was good. (I was wrong.) Also, I now use NS_IMPL_ADDREF/RELEASE(). I don't know the reason I implemented it by hand before.

OK, so I switched from mq to a committed patch, and interdiff was *very* unhappy. This should be the sum total of the interdiff between the previous version and my latest patch.

Comment on attachment 336929 [details] [diff] [review] Solve the last couple of leaks Looks good to me (based on interdiff)!

Comment on attachment 336930 [details] [diff] [review] Simplified interdiff >+ delete gCacheObserver; But... it's a reference-counted object. Shouldn't this be a NS_IF_RELEASE? As far as that goes, shouldn't you addref it before you start passing it around when you create it? The code as written now is just asking for deleted-object-reference crashes. >+ delete gCacheTracker; This looks fine if you also null it out and all the consumers are OK with it possibly being null, or if you're guaranteed that no consumers will try to access it after this point. The former is more likely to be provably correct, I suspect, so I would lean towards doing that. >+++ b/modules/libpr0n/src/imgLoader.h >+ NS_IMPL_ADDREF(imgCacheEntry) >+ NS_IMPL_RELEASE(imgCacheEntry) I'm not sure that's desirable (putting the fully-qualified names here, etc). I'm also not sure you want the weird calling-convention stuff that comes with those. You probably want to just go back to manually implementing it; we don't have good macros for doing this for non-XPCOM classes. Maybe a followup bug on that?

This patch restores the implementation of AddRef and Release, and addresses Boris' other requests.

Comment on attachment 336954 [details] [diff] [review] address bz's comments Looks good. Thanks!

Pushed with a crappy commit message: http://hg.mozilla.org/mozilla-central/rev/44853b2a5fc2

Is there any follow up bug to fix about:cache ? - the new imglib cache is not displayed - the memory cache is way to large (18MB in my case), since it's not used for images anymore

Jo, can you please file those two followup bugs? Assign the imglib cache not being in about:cache to me, and leave the other unassigned.

bug 453989 / bug 454001 I waited, but missed 454000 :-)

I think either this or bug 453765 cause significant tp regression on linux-fast03. http://graphs.mozilla.org/graph.html#show=911694

This patch made sure that images loaded from file:/// are always removed after 10 seconds (they don't have an expiration time). Was that intentional?

yes, that was intentional (it's true for all images). However, images should only be removed if they haven't been used in the past ten seconds. If that isn't the case (ie, images are being removed even though they've been used), please file a new bug and assign it to me.

Thanks, bug 450738 is WON'T FIX then.