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

RESOLVED FIXED

Status

()

Core
ImageLib
P1
normal
RESOLVED FIXED
9 years ago
4 years ago

People

(Reporter: Joe Drew (not getting mail), Assigned: Joe Drew (not getting mail))

Tracking

Trunk
Points:
---
Dependency tree / graph
Bug Flags:
wanted1.9.1 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 11 obsolete attachments)

75.41 KB, patch
Details | Diff | Splinter Review
(Assignee)

Description

9 years ago
Created attachment 316814 [details] [diff] [review]
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.
(Assignee)

Comment 1

9 years ago
Created attachment 322299 [details] [diff] [review]
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.
Attachment #316814 - Attachment is obsolete: true
(Assignee)

Updated

9 years ago
Flags: wanted1.9.1?
P1.
Priority: -- → P1
Flags: wanted1.9.1? → wanted1.9.1+
(Assignee)

Comment 3

9 years ago
Created attachment 328575 [details] [diff] [review]
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.
Attachment #322299 - Attachment is obsolete: true
(Assignee)

Comment 4

9 years ago
Created attachment 329733 [details] [diff] [review]
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.
Attachment #328575 - Attachment is obsolete: true
(Assignee)

Comment 5

9 years ago
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!
(Assignee)

Comment 7

9 years ago
Created attachment 332677 [details] [diff] [review]
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.
Attachment #329733 - Attachment is obsolete: true
Attachment #332677 - Flags: review?
Attachment #329733 - Flags: review?(pavlov)
(Assignee)

Updated

9 years ago
Depends on: 449543
(Assignee)

Comment 8

9 years ago
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)

Comment 9

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

Comment 10

9 years ago
https://build.mozilla.org/tryserver-builds/2008-08-06_23:06-jdrew@mozilla.com-imglib-caching-wip-13/
(Assignee)

Comment 11

9 years ago
Created attachment 334534 [details] [diff] [review]
Small changes to address some of Stuart's (irc) comments
Attachment #332677 - Attachment is obsolete: true
Attachment #334534 - Flags: review?(pavlov)
Attachment #332677 - Flags: review?(pavlov)
(Assignee)

Comment 12

9 years ago
Created attachment 334538 [details] [diff] [review]
Use PRInt64 correctly
Attachment #334534 - Attachment is obsolete: true
Attachment #334538 - Flags: review?(pavlov)
Attachment #334534 - Flags: review?(pavlov)

Updated

9 years ago
Attachment #334538 - Flags: review?(pavlov) → review+
(Assignee)

Comment 13

9 years ago
<3 stuart
Keywords: checkin-needed
Pushed to mozilla-central: http://hg.mozilla.org/mozilla-central/index.cgi/rev/e63a23edb90c
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED

Comment 15

9 years ago
Reverted due to Rlk regression:
http://hg.mozilla.org/mozilla-central/index.cgi/rev/2c020a9f03c1
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Blocks: 451266
No longer blocks: 451266
Depends on: 451266
(Assignee)

Comment 16

9 years ago
Created attachment 336050 [details] [diff] [review]
Fix refcount leaks

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)

Updated

9 years ago
Attachment #336050 - Flags: review?(pavlov) → review+
(Assignee)

Updated

9 years ago
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.
Created attachment 336872 [details] [diff] [review]
More refcount logging

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

Comment 19

9 years ago
(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.
Keywords: checkin-needed
(Assignee)

Comment 20

9 years ago
Created attachment 336929 [details] [diff] [review]
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.
Attachment #336050 - Attachment is obsolete: true
Attachment #336872 - Attachment is obsolete: true
Attachment #336929 - Flags: superreview?
Attachment #336929 - Flags: review?
(Assignee)

Updated

9 years ago
Attachment #336929 - Flags: superreview?(pavlov)
Attachment #336929 - Flags: superreview?
Attachment #336929 - Flags: review?(bzbarsky)
Attachment #336929 - Flags: review?
(Assignee)

Comment 21

9 years ago
Created attachment 336930 [details] [diff] [review]
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.
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?
(Assignee)

Comment 24

9 years ago
Created attachment 336954 [details] [diff] [review]
address bz's comments

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)
(Assignee)

Updated

9 years ago
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!
(Assignee)

Comment 26

9 years ago
Pushed with a crappy commit message: http://hg.mozilla.org/mozilla-central/rev/44853b2a5fc2
Status: REOPENED → RESOLVED
Last Resolved: 9 years ago9 years ago
Resolution: --- → FIXED
(Assignee)

Updated

9 years ago
Attachment #336954 - Flags: review?(vladimir)

Comment 27

9 years ago
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
(Assignee)

Comment 28

9 years ago
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

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

Updated

9 years ago
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
Blocks: 455508
Depends on: 455606

Comment 31

9 years ago
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?
(Assignee)

Comment 32

9 years ago
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

9 years ago
Thanks, bug 450738 is WON'T FIX then.

Updated

9 years ago
Depends on: 466586

Updated

9 years ago
No longer depends on: 466586
Depends on: 472727
Depends on: 497665
You need to log in before you can comment on or make changes to this bug.