SurfaceCache should report its memory usage, but it actually stores DrawTargets, and there's no way to determine the actual memory usage of those DrawTargets right now. In particular, there's no way to determine if they are stored in GPU or CPU memory. Bug 923298 will add memory reporting support for DrawTargets. Once this is fixed, it's straightforward to add memory reporting for SurfaceCache.

[Removing "used by VectorImage" from summary, since SurfaceCache is used by more than just VectorImage, as of bug 1060869 and maybe another bug or two]

Since SurfaceCache stores imgFrame objects now and not SourceSurface objects (thanks to bug 1054079), this has become much easier to implement. I've gone ahead and done so. Some nice stuff (in particular better handling of the distinction between raster and vector images) will need to wait for bug 921300 or one of its blockers, because things are changing fast in imagelib and I think the necessary refactoring should wait until they settle down a little more. I still think we get major benefits here, though. Everything in the surface cache is reported explicitly now. That's particularly critical since now all surfaces in imagelib that aren't part of an animation are stored in the surface cache! (And animations will end up in there, too.)

Comment on attachment 8486929 [details] [diff] [review] Add explicit memory reporting for SurfaceCache Review of attachment 8486929 [details] [diff] [review]: ----------------------------------------------------------------- r=me with the changes made below. You should probably also run this by somebody who knows the image surface cache. ::: image/src/RasterImage.cpp @@ +1059,5 @@ > size_t > RasterImage::SizeOfDecodedWithComputedFallbackIfHeap(gfxMemoryLocation aLocation, > MallocSizeOf aMallocSizeOf) const > { > + size_t surfSize = SurfaceCache::SizeOfSurfaces(ImageKey(this), aLocation, This function has a fallback size computation if |aMallocSizeOf| isn't supported on this platform. It's important that this be clear, so this should be renamed SurfaceCache::SizeOfSurfacesWithComputedFallbackIfHeap(). I know it's a mouthful, but clarity trumps brevity. @@ +1065,5 @@ > + size_t animSize = mFrameBlender > + ? mFrameBlender->SizeOfDecodedWithComputedFallbackIfHeap(aLocation, > + aMallocSizeOf) > + : 0; > + return surfSize + animSize; Nit: The usual idiom for SizeOf* functions that measure multiple things is this: > size_t n = 0; > n += ... > n += ... > return n; ::: image/src/SurfaceCache.cpp @@ +554,4 @@ > { > + nsRefPtr<ImageSurfaceCache> cache = GetImageCache(aImageKey); > + if (!cache) > + return 0; Nit: brace one-line |if| statements. @@ +615,5 @@ > private: > virtual ~MemoryPressureObserver() { } > }; > > + struct SizeOfSurfacesSum As above, this should have a "WithComputedFallbackIfHeap" suffix.

This version of the patch addresses Nicholas's review comments above, with the except of the "WithComputedFallbackIfHeap" stuff, which we have agreed via IRC will be handled (by being made unnecessary) by bug 1065818, which should land very soon after this one. Requesting review from Daniel to get the perspective of someone with more SurfaceCache expertise.

Comment on attachment 8497652 [details] [diff] [review] Add explicit memory reporting for SurfaceCache (FWIW: this patch doesn't apply cleanly on current m-c. e.g. the first chunk, for RasterImage::SizeOfDecodedWithComputedFallbackIfHeap, is rejected because the ternary-condition on "mFrameBlender" doesn't exist in that function, in current m-c. This bug probably wants to be marked as depending on whichever bug's still-to-be-landed patch adds that check.)

Ah, looks like that bug (which adds the mFrameBlender check) is bug 1060869. This is currently blocking bug 1060869, but it seems like the relationship is really the other way around, at least in terms of what-lands-first?

Comment on attachment 8497652 [details] [diff] [review] Add explicit memory reporting for SurfaceCache Review of attachment 8497652 [details] [diff] [review]: ----------------------------------------------------------------- r=me, with comments addressed: ::: image/src/RasterImage.cpp @@ +1010,5 @@ > RasterImage::SizeOfDecodedWithComputedFallbackIfHeap(gfxMemoryLocation aLocation, > MallocSizeOf aMallocSizeOf) const > { > + size_t n = SurfaceCache::SizeOfSurfaces(ImageKey(this), aLocation, > + aMallocSizeOf); aMallocSizeOf looks mis-indented there. Also, per comment 4, I think njn wanted this to be more like: size_t n = 0; n += SurfaceCache::SizeOfSurfaces(...) (I don't know how strictly we stick to the " = 0" part of the idiom that's missing here; maybe check with njn to see if that part really matters, if you haven't already.) ::: image/src/SurfaceCache.cpp @@ +155,5 @@ > ImageKey GetImageKey() const { return mImageKey; } > SurfaceKey GetSurfaceKey() const { return mSurfaceKey; } > CostEntry GetCostEntry() { return image::CostEntry(this, mCost); } > nsExpirationState* GetExpirationState() { return &mExpirationState; } > + imgFrame* GetRawSurface() const { return mSurface.get(); } Do we really want to expose a GetRawSurface() method? I'm a little uneasy about it, particularly given that it's returning a raw pointer to a refcounted object (and someone might accidentally add a caller down the line that do unsafe things, directly or indirectly, with that raw pointer -- holding onto it without a nsRefPtr, basically). If we only need this accessor for the benefit of SizeOfSurfacesSum (which I think is the case), I'd lean towards keeping the imgFrame abstracted away, and just giving SizeOfSurfacesSum() access to CachedSurface's private data, by defining SizeOfSurfacesSum inside of "class CachedSurface { ... };". (Currently, you're adding it in "class SurfaceCacheImpl { ... }", but since it's measuring private data from CachedSurface, it seems better-suited to living in CachedSurface.) Alternately, we could declare it as a friend class to give it private access. @@ +526,5 @@ > bool aAnonymize) > { > + // We have explicit memory reporting for the surface cache which is more > + // accurate than what we report here, but this information is still useful > + // since these numbers are what actually control the cache's behavior. Nit: the second half of this comment is a bit ambiguous -- in particular, I wasn't sure which piece of data you were referring to with "this information" & "these numbers". (I initially thought they referred to the explicit memory reporting (since that's what you're talking about in the first part of this comment), but I later realized they're talking about the cost metrics reported in this function.) Let's tweak this to make it less ambiguous, e.g. something like: // We have explicit memory reporting for the surface cache which is more // accurate than the cost metrics that we report here, but these metrics // are still useful to report, since they control the cache's behavior. @@ +569,3 @@ > { > + auto sum = static_cast<SizeOfSurfacesSum*>(aSum); > + sum->Add(aSurface->GetRawSurface()); (If we drop GetRawSurface as I suggest above, then we'd just want to pass aSurface directly to Add() here, and let the SizeOfSurfacesSum object poke at aSurface's internal data.)

(If you feel like adding the MOZ_OVERRIDEs mentioned in bug 1075986 as part of this patch, too, I'm happy to proactively r+ that addition)

> Also, per comment 4, I think njn wanted this to be more like: > size_t n = 0; > n += SurfaceCache::SizeOfSurfaces(...) > > (I don't know how strictly we stick to the " = 0" part of the idiom that's > missing here; maybe check with njn to see if that part really matters, if > you haven't already.) It's how I usually do it but I wouldn't r- if it wasn't done that way :)

(Cool -- I don't have a preference; just wanted to make sure that hadn't slipped through the cracks. Sounds like that part is fine either way, then.)

(In reply to Daniel Holbert [:dholbert] from comment #7) > Ah, looks like that bug (which adds the mFrameBlender check) is bug 1060869. > This is currently blocking bug 1060869, but it seems like the relationship > is really the other way around, at least in terms of what-lands-first? Yep, that's correct. I originally had them in the other order in my patch queue but it turned out to be easier this way; I'll fix things up. I'm still going to land them in the same push, though, because I want to make sure that we don't lose high quality memory reporting even temporarily when bug 1060869 lands.

Thanks for the quick review! (In reply to Daniel Holbert [:dholbert] from comment #8) > Do we really want to expose a GetRawSurface() method? > ... > If we only need this accessor for the benefit of SizeOfSurfacesSum (which I > think is the case), I'd lean towards keeping the imgFrame abstracted away, > and just giving SizeOfSurfacesSum() access to CachedSurface's private data, > by defining SizeOfSurfacesSum inside of "class CachedSurface { ... };". Yeah, I agree with your concerns here. I'll take another look and see if this can be done in a cleaner way. > (If you feel like adding the MOZ_OVERRIDEs mentioned in bug 1075986 as part > of this patch, too, I'm happy to proactively r+ that addition) Yeah sure, that seems easy enough.

This version of the patch addresses the review comments and integrates the changes from bug 1075986.

Comment on attachment 8498550 [details] [diff] [review] Add explicit memory reporting for SurfaceCache >+ // A helper type used by SurfaceCacheImpl::SizeOfSurfacesSum. >+ struct SizeOfSurfacesSum [...] >+ void Add(CachedSurface* aCachedSurface) >+ { >+ if (!aCachedSurface || !aCachedSurface->mSurface) { >+ return; >+ } >+ >+ mSum += aCachedSurface->mSurface-> >+ SizeOfExcludingThisWithComputedFallbackIfHeap(mLocation, mMallocSizeOf); >+ } [...] >+ static PLDHashOperator DoSizeOfSurfacesSum(const SurfaceKey&, >+ CachedSurface* aSurface, >+ void* aSum) > { >- return mLockedCost; >+ auto sum = static_cast<CachedSurface::SizeOfSurfacesSum*>(aSum); >+ sum->Add(aSurface); >+ return PL_DHASH_NEXT; Is it possible for us to get a null CachedSurface* passed to SizeOfSurfacesSum::Add()? I don't think so, and the previous version of the patch suggested that this shouldn't happen, in the spot where it dereferenced the CachedSurface (in DoSizeOfSurfacesSum). If we did have a null pointer passed to Add, that would imply that we have a null CachedSurface stored as an actual value in our hash table, and I don't think that's possible, given the assertion in ImageSurfaceCache::Insert(). If I'm correct on this, could you add an assertion to Add() that aCachedSurface is non-null? (and probably also remove it from the early-return, though I'm also OK with keeping that check for robustness, as long as we have the assertion to document what our actual expectations are)

Pushed: https://hg.mozilla.org/integration/mozilla-inbound/rev/a3e9214bfcce

(In reply to Daniel Holbert [:dholbert] from comment #15) > If I'm correct on this, could you add an assertion to Add() that > aCachedSurface is non-null? (and probably also remove it from the > early-return, though I'm also OK with keeping that check for robustness, as > long as we have the assertion to document what our actual expectations are) Sure, but it'll have to be a followup.

... or not. Backed out: https://hg.mozilla.org/integration/mozilla-inbound/rev/e388678a9211

This version of the patch addresses comment 15 and adds Daniel to the list of reviewers in the commit message.

Pushed: https://hg.mozilla.org/integration/mozilla-inbound/rev/2a95a3663cc2

sorry seth had to back this out in https://treeherder.mozilla.org/ui/#/jobs?repo=mozilla-inbound&revision=e04692266f17 since one of this changes caused test failures like https://treeherder.mozilla.org/ui/logviewer.html#?job_id=4191914&repo=mozilla-inbound Seth i noticed this failure also on https://tbpl.mozilla.org/?tree=Try&rev=90124f3bc2a0 and did a retrigger, maybe this helps

Repushed since that issue should be dealt with now: remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/503b80845952

sorry had to back this out again in https://treeherder.mozilla.org/ui/#/jobs?repo=mozilla-inbound&revision=b5dda6f01770 since one of this changes caused https://treeherder.mozilla.org/ui/logviewer.html#?job_id=4220043&repo=mozilla-inbound

Repushed: remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/7ee7e774e19f