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

RESOLVED FIXED

Status

()

defect
P1
normal
RESOLVED FIXED
11 years ago
6 years ago

People

(Reporter: joe, Assigned: joe)

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
bzbarsky
: review+
Details | Diff | Splinter Review
(Assignee)

Description

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

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

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

Comment 3

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

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

11 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

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

11 years ago
Depends on: 449543
(Assignee)

Comment 8

11 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

11 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 11

11 years ago
Attachment #332677 - Attachment is obsolete: true
Attachment #334534 - Flags: review?(pavlov)
Attachment #332677 - Flags: review?(pavlov)
(Assignee)

Comment 12

11 years ago
Posted patch Use PRInt64 correctly (obsolete) — Splinter Review
Attachment #334534 - Attachment is obsolete: true
Attachment #334538 - Flags: review?(pavlov)
Attachment #334534 - Flags: review?(pavlov)

Updated

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

Comment 13

11 years ago
<3 stuart
Keywords: checkin-needed

Comment 14

11 years ago
Pushed to mozilla-central: http://hg.mozilla.org/mozilla-central/index.cgi/rev/e63a23edb90c
Status: NEW → RESOLVED
Last Resolved: 11 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
(Assignee)

Comment 16

11 years ago
Posted 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)

Updated

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

Updated

11 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.
Posted patch More refcount logging (obsolete) — Splinter Review
You probably want to add this to the next patch iteration, by the way.
(Assignee)

Comment 19

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

Comment 20

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

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

Comment 21

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

Comment 24

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

11 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

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

Updated

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

Comment 27

11 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

11 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

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

Updated

11 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

Comment 31

11 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

11 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

11 years ago
Thanks, bug 450738 is WON'T FIX then.
Depends on: 466586

Updated

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