Closed Bug 430061 Opened 13 years ago Closed 12 years ago

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

Categories

(Core :: ImageLib, defect, P1)

defect

Tracking

()

RESOLVED FIXED

People

(Reporter: joe, Assigned: joe)

References

Details

Attachments

(1 file, 11 obsolete files)

75.41 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
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.
Attachment #316814 - Attachment is obsolete: true
Flags: wanted1.9.1?
P1.
Priority: -- → P1
Flags: wanted1.9.1? → wanted1.9.1+
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.
Attachment #322299 - Attachment is obsolete: true
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.
Attachment #328575 - Attachment is obsolete: true
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.
Attachment #329733 - Flags: review?(pavlov)
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.
Attachment #329733 - Attachment is obsolete: true
Attachment #332677 - Flags: review?
Attachment #329733 - Flags: review?(pavlov)
Depends on: 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!
Attachment #332677 - Attachment description: WIP patch: use nsExpirationTracker to expire cache elements by time → Use nsExpirationTracker to expire cache elements by time
Attachment #332677 - Flags: review? → review?(pavlov)
Joe if you kickoff a mac try server build and hand it to me I'll re-run the memory tests on it...
Attachment #332677 - Attachment is obsolete: true
Attachment #334534 - Flags: review?(pavlov)
Attachment #332677 - Flags: review?(pavlov)
Attached patch Use PRInt64 correctly (obsolete) — Splinter Review
Attachment #334534 - Attachment is obsolete: true
Attachment #334538 - Flags: review?(pavlov)
Attachment #334534 - Flags: review?(pavlov)
Attachment #334538 - Flags: review?(pavlov) → review+
<3 stuart
Keywords: checkin-needed
Pushed to mozilla-central: http://hg.mozilla.org/mozilla-central/index.cgi/rev/e63a23edb90c
Status: NEW → RESOLVED
Closed: 12 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Reverted due to Rlk regression:
http://hg.mozilla.org/mozilla-central/index.cgi/rev/2c020a9f03c1
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
No longer blocks: 451266
Depends on: 451266
Attached patch Fix refcount leaks (obsolete) — Splinter Review
When this patch was initially landed, leaks were found by the various tests. This updated patch fixes those leaks.
Attachment #334538 - Attachment is obsolete: true
Attachment #336050 - Flags: review?(pavlov)
Attachment #336050 - Flags: review?(pavlov) → review+
Keywords: checkin-needed
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.
Attached patch More refcount logging (obsolete) — Splinter Review
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.
Attached patch Solve the last couple of leaks (obsolete) — Splinter Review
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.
Attachment #336050 - Attachment is obsolete: true
Attachment #336872 - Attachment is obsolete: true
Attachment #336929 - Flags: superreview?
Attachment #336929 - Flags: review?
Attachment #336929 - Flags: superreview?(pavlov)
Attachment #336929 - Flags: superreview?
Attachment #336929 - Flags: review?(bzbarsky)
Attachment #336929 - Flags: review?
Attached patch Simplified interdiff (obsolete) — Splinter Review
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.
Attachment #336929 - Flags: superreview?(pavlov)
Attachment #336929 - Flags: superreview+
Attachment #336929 - Flags: review?(bzbarsky)
Attachment #336929 - Flags: review+
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.
Attachment #336929 - Attachment is obsolete: true
Attachment #336930 - Attachment is obsolete: true
Attachment #336954 - Flags: review?(vladimir)
Attachment #336954 - Flags: review?(bzbarsky)
Attachment #336954 - Flags: review?(bzbarsky) → review+
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
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
Attachment #336954 - Flags: review?(vladimir)
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 :-)
Depends on: 454376
I think either this or bug 453765 cause significant tp regression on linux-fast03.
http://graphs.mozilla.org/graph.html#show=911694
Depends on: 455606
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.
Depends on: 466586
No longer depends on: 466586
Depends on: 497665
You need to log in before you can comment on or make changes to this bug.