430061 - Replace imglib's use of necko memory cache with an imglib-specific cache

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect, P1)

Description

[Joe Drew (not getting mail)]

Attachment: WIP patch to use a hash table instead of necko's memory cache

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.

Comment 1

[Joe Drew (not getting mail)]

Attachment: WIP patch; merge imgCache and imgLoader

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.

Comment 2

[Damon Sicore (:damons)]

P1.

Comment 3

[Joe Drew (not getting mail)]

Attachment: WIP patch; use a priority queue to monitor cache size

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.

Comment 4

[Joe Drew (not getting mail)]

Attachment: Fix the problems in the previous patch that killed Tp

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 5

[Joe Drew (not getting mail)]

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.

Comment 6

[Damon Sicore (:damons)]

GO JOE!

Comment 7

[Joe Drew (not getting mail)]

Attachment: Use nsExpirationTracker to expire cache elements by time

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 8

[Joe Drew (not getting mail)]

Comment on attachment 332677 [details] [diff] [review]
Use nsExpirationTracker to expire cache elements by time

I guess it's not actually a WIP patch!

Comment 9

[Mike Schroepfer]

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

Comment 10

[Joe Drew (not getting mail)]

https://build.mozilla.org/tryserver-builds/2008-08-06_23:[e-mail]-imglib-caching-wip-13/

Comment 11

[Joe Drew (not getting mail)]

Attachment: Small changes to address some of Stuart's (irc) comments

Comment 12

[Joe Drew (not getting mail)]

Attachment: Use PRInt64 correctly

Comment 13

[Joe Drew (not getting mail)]

<3 stuart

Comment 14

[Benjamin Smedberg]

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

Comment 15

[Dave Camp (:dcamp)]

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

Comment 16

[Joe Drew (not getting mail)]

Attachment: Fix refcount leaks

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

Comment 17

[Boris Zbarsky [:bzbarsky]]

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.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Attachment: More refcount logging

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

Comment 19

[Joe Drew (not getting mail)]

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

Comment 20

[Joe Drew (not getting mail)]

Attachment: Solve the last couple of leaks

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.

Comment 21

[Joe Drew (not getting mail)]

Attachment: Simplified interdiff

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 22

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 336929 [details] [diff] [review]
Solve the last couple of leaks

Looks good to me (based on interdiff)!

Comment 23

[Boris Zbarsky [:bzbarsky]]

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?

Comment 24

[Joe Drew (not getting mail)]

Attachment: address bz's comments

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

Comment 25

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 336954 [details] [diff] [review]
address bz's comments

Looks good.  Thanks!

Comment 26

[Joe Drew (not getting mail)]

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

Comment 27

[Jo Hermans]

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

Comment 28

[Joe Drew (not getting mail)]

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.

Comment 29

[Jo Hermans]

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

Comment 30

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 31

[Wladimir Palant]

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?

Comment 32

[Joe Drew (not getting mail)]

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.

Comment 33

[Wladimir Palant]

Thanks, bug 450738 is WON'T FIX then.