1157954 - Report each surface in the ImageLib SurfaceCache individually in about:memory

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Seth Fowler [:seth] [:s2h]]

Right now we only report the total amount of memory used by an image in the SurfaceCache (though we separate the number into heap vs nonheap).

It'd be much better to report each surface individually. That would make debugging an issue like the one in bug 1157559 very straightforward.

Comment 1

[Seth Fowler [:seth] [:s2h]]

Attachment: Report each surface in the ImageLib SurfaceCache individually in about:memory

Wow, this turned into a pretty big patch for what seems like a relatively small
change.

The main idea here is that instead of recording all memory counters on an
ImageMemoryCounter, we add an intermediate type called a SurfaceMemoryCounter
that records the counters for each surface. ImageMemoryCounter then just records
totals. As with ImageMemoryCounter, SurfaceMemoryCounter also tracks a little
bit of metadata: for surfaces, we track their size, whether they're locked,
their animation time, and their decode flags. That allows us to report enough
information to diagnose memory issues involving the SurfaceCache, especially
those involving HQ scaling, downscale-during-decode, or animation.

We also track one more piece of information: the surface type. This only exists
to let us flag the compositing surfaces we use in FrameAnimator as something
special. (Those are also the only surfaces that we don't store in the
SurfaceCache.)

The about:memory output places "locked/" and "unlocked/" sections under each
image. Under each of those sections is a list of surfaces, identified by the
metadata mentioned above. The surfaces are then broken down individually into
"decoded-heap", "decoded-nonheap", and the rest as usual.

The common case is that images only have one surface, so this doesn't bloat the
output much. When images *do* have many surfaces, we of course end up reporting
a lot more information after this patch, but that's the point: this information
will help tracking down memory issues that currently require custom logging to
accurately diagnose. This should make life easier for me, and I think also for
MemShrink folks.

Comment 2

[Eric Rahm [:erahm]]

Comment on attachment 8597568 [details] [diff] [review]
Report each surface in the ImageLib SurfaceCache individually in about:memory

Review of attachment 8597568 [details] [diff] [review]:
-----------------------------------------------------------------

As discussed in IRC, I think we want to keep 'SizeOf' somewhere in the function names of things that use |MallocSizeOf|.

Seth, is it possible for you to paste a snippet of the new about:memory output as well?

::: image/src/Image.h
@@ +53,5 @@
> +  MemoryCounter& operator-=(const MemoryCounter& aOther)
> +  {
> +    mSource += aOther.mSource;
> +    mDecodedHeap += aOther.mDecodedHeap;
> +    mDecodedNonHeap += aOther.mDecodedNonHeap;

Looks like a copy and paste failure here.

::: image/src/imgLoader.cpp
@@ +230,5 @@
> +                                 const ImageMemoryCounter& aCounter)
> +  {
> +    nsresult rv;
> +
> +    for (const SurfaceMemoryCounter counter : aCounter.Surfaces()) {

const SurfaceMemoryCounter& ?

Comment 3

[Daniel Holbert [:dholbert]]

Comment on attachment 8597568 [details] [diff] [review]
Report each surface in the ImageLib SurfaceCache individually in about:memory

Review of attachment 8597568 [details] [diff] [review]:
-----------------------------------------------------------------

::: image/src/DynamicImage.cpp
@@ +48,2 @@
>  {
> +  // We can't do anything useful because gfxDrawable doesn't expose this

Maybe s/do/report/?
(i.e. "We can't report anything useful because...")

::: image/src/FrameAnimator.cpp
@@ +345,5 @@
> +}
> +
> +void
> +FrameAnimator::
> +  ReportCompositingSurfaceMemory(nsTArray<SurfaceMemoryCounter>& aCounters,

Stylistic nit: this particular break-and-deindent strategy (which I have myself used in the past) is actually an antipattern, because it breaks searchability.  If you're trying to find this funtion-impl in MXR, you'd rightfully expect to be able to search for "FrameAnimator::ReportCompositingSurfaceMemory", but that won't work if you break after the ::.

So, better to break-and-deindent after the open-paren, like so:

void
FrameAnimator::ReportCompositingSurfaceMemory(
  nsTArray<SurfaceMemoryCounter>& aCounters,
  MallocSizeOf aMallocSizeOf) const
{

(Mats pointed this out to me during a review a little while back, and I agree with him.)

::: image/src/Image.h
@@ +91,5 @@
> +private:
> +  const SurfaceKey mKey;
> +  MemoryCounter mValues;
> +  SurfaceMemoryCounterType mType;
> +  bool mIsLocked;

Looks like mType & mIsLocked could be declared as const. (Their values are assigned in the init list, and they're not modified after that.)

(It doesn't matter too much, since this is just a simple struct; I'm only mentioning it because you've got one thing already labeled as const here, which sort of implies that the rest of the variables are modified -- but really only one of them is.)

@@ +129,5 @@
> +  nsCString mURI;
> +  nsTArray<SurfaceMemoryCounter> mSurfaces;
> +  uint16_t mType;
> +  MemoryCounter mValues;
> +  bool mIsUsed : 1;

Nit: seems likely that this will pack better if you move "uint16_t mType" down by one line, to make it adjacent to the bitfield bools.

::: image/src/SurfaceCache.cpp
@@ +164,5 @@
>    Lifetime GetLifetime() const { return mLifetime; }
>    bool IsDecoded() const { return mSurface->IsImageComplete(); }
>  
> +  // A helper type used by SurfaceCacheImpl::ReportSurfaceMemory.
> +  struct SurfaceMemoryReport

Add MOZ_STACK_CLASS after "struct" here.

(This is only ever used on the stack, as a helper-object for use during hashtable enumeration.  And this is important to enforce, because this object retains a (C++) reference to a nsTArray whose lifetime it doesn't control.  So, this object had *better* be stack-only, or it might accidentally outlive that nsTArray and trigger a use-after-free bug.)

::: image/src/imgLoader.cpp
@@ +228,5 @@
> +                                 nsISupports* aData,
> +                                 const nsACString& aPathPrefix,
> +                                 const ImageMemoryCounter& aCounter)
> +  {
> +    nsresult rv;

You only use this variable inside the 'for' loop. Just declare it at the line where it's used.

@@ +390,5 @@
>      nsAutoCString spec;
>      imageURL->GetSpec(spec);
>  
>      ImageMemoryCounter counter(image->GetType(), spec, aIsUsed);
> +    counter.Init(image, ImagesMallocSizeOf);

(I'd suggest collapsing "Init()" into the constructor.  Generally, I think Init() methods make sense when there's a potential for something to fail, or when there's some processing we need to do between construction & initialization.  But neither of those things are true here, so I don't see any reason for the division.  Note that combining these would let us get rid of the mInitialized bool, which is a small complexity win.

This is a minor point; I'm OK keeping it as-is, too, if you like having them split up for some reason.)

Comment 4

[Seth Fowler [:seth] [:s2h]]

Thanks for the feedback and review comments, everyone! I'll post an updated version of the patch shortly.

In addition to addressing the comments here, it'll address a few other issues I noticed, and will tweak the output format a bit.

(In reply to Eric Rahm [:erahm] from comment #2)
> Seth, is it possible for you to paste a snippet of the new about:memory
> output as well?

Sure. Here's a snippet of the new output format for arstechnica.com:

├────9.11 MB (04.61%) -- images
│    ├──8.03 MB (04.06%) -- content/raster/used
│    │  ├──0.55 MB (00.28%) ++ image(480x300, moz-page-thumb://thumbnail?url=http%3A%2F%2Fwww.reddit.com%2Fr%2Fgif)/unlocked/surface(480x300@0)
│    │  ├──0.25 MB (00.13%) ++ image(728x90, http://s1.2mdn.net/viewad/2993648/CIROC_PINEAPPLE_728X90_v4.jpg)/locked/surface(728x90@0)
│    │  ├──0.17 MB (00.09%) ++ image(300x150, http://cdn.arstechnica.net/wp-content/uploads/2015/04/konami2-300x150.jpg)/locked/surface(300x150@0)
│    │  ├──0.12 MB (00.06%) ++ image(300x100, http://cdn.arstechnica.net/wp-content/uploads/2015/04/triviacrack-300x100.png)/unlocked/surface(300x100@0)

(It's not present in this example, but images *can* have both locked and unlocked surfaces. Downscale-during-decode makes that much more common.)

Here's another example, for some animated GIF I found on reddit, because some aspects of the output are only relevant for animated images:

│   │  │  └──10.97 MB (04.06%) -- image(400x300, http://i.imgur.com/SBFjnCU.gif)/locked
│   │  │     ├───0.46 MB (00.17%) ++ surface(400x300, compositing frame)
│   │  │     ├───0.46 MB (00.17%) ++ surface(400x300@0)
│   │  │     ├───0.12 MB (00.04%) ── surface(400x300@1)/decoded-heap
│   │  │     ├───0.12 MB (00.04%) ── surface(400x300@149)/decoded-heap
│   │  │     ├───0.09 MB (00.03%) ── surface(299x298 subframe of 400x300@69)/decoded-heap
│   │  │     ├───0.09 MB (00.03%) ── surface(299x299 subframe of 400x300@132)/decoded-heap

In "400x300@132", the "132" is a time value. (Meaning, for raster images, a frame number.)

Comment 5

[Seth Fowler [:seth] [:s2h]]

Hmm. It didn't occur to me that bugzilla would linkify those URIs. I apologize to the people running those sites for any additional traffic. =\

Comment 6

[Seth Fowler [:seth] [:s2h]]

Attachment: Report each surface in the ImageLib SurfaceCache individualy in about:memory

Here's the updated version of the patch. It should address all of the comments
so far.

This version of the patch also includes a few changes to formatting and other
similar small nits.

Most importantly, this version changes the output format to the one shown in
comment 4. It should convey all of the same information as the original patch,
while being a bit easier to read and a bit less verbose in the common case. It
also reports one new piece of information: when animated images frames do not
cover the complete area of the image, it reports the subframe size. I included
this because it's important for understanding memory usage.

Comment 7

[Seth Fowler [:seth] [:s2h]]

Oh, I just realized that I also added reporting of each image's intrinsic size in the new version of the patch. Again, you can see an example in comment 4.

Comment 8

[Daniel Holbert [:dholbert]]

Comment on attachment 8598360 [details] [diff] [review]
Report each surface in the ImageLib SurfaceCache individualy in about:memory

One nit on the new patch:

>+++ b/image/src/imgLoader.cpp
>@@ -408,26 +401,19 @@ private:
>     if (!image) {
>       return;
>     }
> 
>     nsRefPtr<ImageURL> imageURL(image->GetURI());
>     nsAutoCString spec;
>     imageURL->GetSpec(spec);
> 
>-    ImageMemoryCounter counter(image->GetType(), spec, aIsUsed);
[...]
>+    ImageMemoryCounter counter(image, ImagesMallocSizeOf, aIsUsed);
>+
>+    aArray->AppendElement(Move(counter));
>   }

Looks like the variables "imageURL" and "spec" here are unused now, & should be removed. (You look them up inside of the ImageMemoryCounter constructor now, so they're not needed here anymore.)

Comment 9

[Seth Fowler [:seth] [:s2h]]

Attachment: Report each surface in the ImageLib SurfaceCache individually in about:memory

Thanks for taking another look, Daniel! I've removed those lines. Here's the
final version of the patch.

Comment 10

[Seth Fowler [:seth] [:s2h]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0d3cd85c3132

Comment 11

[Seth Fowler [:seth] [:s2h]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8fdcb497c551

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8fdcb497c551