711901 - Use mallocSizeOf in the source image memory reporters

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Nicholas Nethercote [inactive]]

Attachment: patch

This patch reworks the source image memory reporters in the new style (https://wiki.mozilla.org/Platform/Memory_Reporting).  Because the old measuring functions were used both for memory reporting and managing the image cache, I've added a second measuring path that's just for memory reporters, and it uses mallocSizeOf.

Lots of names of functions changed to use the new conventions, and the old functions all now start with "ComputedSize".

The newly added imgFrame::SizeOfExcludingThis() doesn't yet pass mallocSizeOf to gfxASurface::KnownMemoryUsed, which means I haven't changed the decoded image reporters yet.  That's a harder change than this one because of all the platform-specific stuff, and I want to do it in a follow-up.

Comment 1

[Jeff Muizelaar [:jrmuizel]]

(In reply to Nicholas Nethercote [:njn] from comment #0)
> Created attachment 582736 [details] [diff] [review]
> patch
> 
> This patch reworks the source image memory reporters in the new style
> (https://wiki.mozilla.org/Platform/Memory_Reporting).  Because the old
> measuring functions were used both for memory reporting and managing the
> image cache, I've added a second measuring path that's just for memory
> reporters, and it uses mallocSizeOf.

If I understand correctly, the new measuring path gives numbers that are strictly larger than the old path because of slop bytes. Personally, I don't mind using the new version for the image cache. Since, the image cache is trying to optimize for memory used, the new versions seem technically more correct.

I'd like to hear what Joe thinks, but he's on vacation till January. In the mean time I think it might be worth doing a version that just uses the new path. That's the approach I would prefer.

Comment 2

[Nicholas Nethercote [inactive]]

> If I understand correctly, the new measuring path gives numbers that are
> strictly larger than the old path because of slop bytes.

That's right.  (Except for platforms where moz_malloc_usable_size always returns zero, and then we fall back to the computed sizes.)

> Personally, I don't
> mind using the new version for the image cache. Since, the image cache is
> trying to optimize for memory used, the new versions seem technically more
> correct.
> 
> I'd like to hear what Joe thinks, but he's on vacation till January. In the
> mean time I think it might be worth doing a version that just uses the new
> path. That's the approach I would prefer.

Ok.  My only concern is that I'm not sure how expensive moz_malloc_usable_size is.  Looking at jemalloc's code, for huge (1MB+) allocations it involves a lock.  For smaller allocations it looks fairly fast, just a bunch of memory lookups and bit operations.

How often are these functions called for the image cache?

Comment 3

[Nicholas Nethercote [inactive]]

jrmuizel, how should I move forward with this bug?  Is Joe back from vacation?

Comment 4

[Joe Drew (not getting mail)]

Comment on attachment 582736 [details] [diff] [review]
patch

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

::: image/src/RasterImage.cpp
@@ -393,2 @@
>    if (nextFrameIndex == 0) {
> -    frameToUse = nextFrame;

hunk is unrelated?

::: image/src/imgFrame.cpp
@@ +761,2 @@
>  {
> +  PRUint32 n = 0;

this name change seems unnecessary, though I don't feel strongly about it

::: image/src/imgRequest.cpp
@@ +482,5 @@
>  
>  void imgRequest::UpdateCacheEntrySize()
>  {
>    if (mCacheEntry) {
> +    mCacheEntry->SetDataSize(mImage->ComputedSizeOfData());

This is the only place that ComputedSizeOfData would be called, and it's then cached during the actual expiry calculation. This is only called after an image is finished downloading or (re-)decoding, which is not frequent. Thus, its cost is not an issue.

THEREFORE

please only have one way of computing the size of this data, and make it correct. :)

Comment 5

[Nicholas Nethercote [inactive]]

Attachment: patch v2

This patch unifies the two measurement paths.  There's a wart, which is that moz_malloc_usable_size doesn't work on some obscure platforms and configurations (e.g. Solaris or BSD if --disable-jemalloc or --trace-malloc is enabled) and so always returns 0.  I figured returning 0 would not be good for the cache, so I had to do some ugly stuff like imgFrame::SizeOfExcludingThisWithComputedFallbackIfHeap().  But there is less duplication than in the previous patch.

The one thing I was uncertain about was the single-pixel case in SizeOfExcludingThisWithComputedFallbackIfHeap().  I couldn't determine any heap allocations associated with it, so I just removed it.


> ::: image/src/RasterImage.cpp
> @@ -393,2 @@
> >    if (nextFrameIndex == 0) {
> > -    frameToUse = nextFrame;
> 
> hunk is unrelated?

|frameToUse| is a dead variable.  This change fixes a compiler warning, I figured I'd do it while I was in the neighbourhood.

Comment 6

[Justin Lebar (not reading bugmail)]

> imgFrame::SizeOfExcludingThisWithComputedFallbackIfHeap().

Wow, when I advocated for the longer method name "SizeOfExcludingThis", I did not think it would come to this!

Comment 7

[Joe Drew (not getting mail)]

Comment on attachment 594073 [details] [diff] [review]
patch v2

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

::: image/src/Image.h
@@ +105,5 @@
> +  virtual size_t
> +    HeapSizeOfSourceWithComputedFallback(nsMallocSizeOfFun aMallocSizeOf)
> +    const = 0;
> +  virtual size_t
> +    HeapSizeOfDecodedWithComputedFallback(nsMallocSizeOfFun aMallocSizeOf)

I don't think it's necessary to say "WithComputedFallback" here. Long name is long.

::: image/src/RasterImage.cpp
@@ +981,3 @@
>  {
> +  size_t n = mSourceData.SizeOfExcludingThis(aMallocSizeOf);
> +  if (n == 0) {

What happens if we have a (invalid, but they exist all over) 0-byte image?

@@ +988,5 @@
> +  return n;
> +}
> +
> +static size_t
> +SizeOfDecodedWithComputedFallbackIfHeap(

"SizeOfDecoded" is a nicer name.

@@ +989,5 @@
> +}
> +
> +static size_t
> +SizeOfDecodedWithComputedFallbackIfHeap(
> +  const nsTArray<imgFrame *> &aFrames, gfxASurface::MemoryLocation aLocation,

& and * should go with the type, not the name, fwiw

@@ +997,5 @@
>    for (PRUint32 i = 0; i < aFrames.Length(); ++i) {
>      imgFrame *frame = aFrames.SafeElementAt(i, nsnull);
>      NS_ABORT_IF_FALSE(frame, "Null frame in frame array!");
> +    n += frame->SizeOfExcludingThisWithComputedFallbackIfHeap(
> +                  aLocation, aMallocSizeOf);

You don't need to adhere so strongly to 80-column files. I suspect this will become less of an issue when you rename these functions, but I'd much prefer one of the following:

foo->Bar(asdf,
         23,
         0.0);
or

foo->Bar(asdf, 23, 0.0);

::: image/src/imgFrame.cpp
@@ +764,5 @@
>  {
> +  NS_ABORT_IF_FALSE(
> +    (aLocation == gfxASurface::MEMORY_IN_PROCESS_HEAP &&  aMallocSizeOf) ||
> +    (aLocation != gfxASurface::MEMORY_IN_PROCESS_HEAP && !aMallocSizeOf),
> +    "mismatch between aLocation and aMallocSizeOf");

A comment would be handy here.

@@ +769,2 @@
>  
> +  size_t n = 0;

Why rename size to n?

::: image/src/imgFrame.h
@@ +129,5 @@
>  #endif
>      return mImageSurface;
>    }
>  
> +  size_t SizeOfExcludingThisWithComputedFallbackIfHeap(

SizeOfExcludingThis would be a better name, with documentation that it has a computed fallback.

Comment 8

[Nicholas Nethercote [inactive]]

( > +    HeapSizeOfDecodedWithComputedFallback(nsMallocSizeOfFun aMallocSizeOf)
> 
> I don't think it's necessary to say "WithComputedFallback" here. Long name
> is long.

I respectfully disagree.  Names of the form |SizeOfFoo| are used in memory reporters all over the codebase and they have a standard behaviour -- they can always return 0 on some platforms.  This function has a subtly, crucially different behaviour, and so merits a different name.  Sure it's long, but this function (and the similar ones) only occur in a handful of places.

Comment 9

[Nicholas Nethercote [inactive]]

Attachment: patch, v3

I kept the long names for the reasons explained in comment 8.


> ::: image/src/RasterImage.cpp
> @@ +981,3 @@
> >  {
> > +  size_t n = mSourceData.SizeOfExcludingThis(aMallocSizeOf);
> > +  if (n == 0) {
> 
> What happens if we have a (invalid, but they exist all over) 0-byte image?

The fallback case works fine.  I've added a comment explaining this.


> @@ +989,5 @@
> > +}
> > +
> > +static size_t
> > +SizeOfDecodedWithComputedFallbackIfHeap(
> > +  const nsTArray<imgFrame *> &aFrames, gfxASurface::MemoryLocation aLocation,
> 
> & and * should go with the type, not the name, fwiw

I changed them, but note that this file uses an almost perfect mix of the two styles :/


> ::: image/src/imgFrame.cpp
> @@ +764,5 @@
> >  {
> > +  NS_ABORT_IF_FALSE(
> > +    (aLocation == gfxASurface::MEMORY_IN_PROCESS_HEAP &&  aMallocSizeOf) ||
> > +    (aLocation != gfxASurface::MEMORY_IN_PROCESS_HEAP && !aMallocSizeOf),
> > +    "mismatch between aLocation and aMallocSizeOf");
> 
> A comment would be handy here.

Added.

 
> @@ +769,2 @@
> >  
> > +  size_t n = 0;
> 
> Why rename size to n?

This patch is all about converting the memory reporter to the prevailing style, and the prevailing style for memory reporters is to use |n|.

Comment 10

[Joe Drew (not getting mail)]

Comment on attachment 598121 [details] [diff] [review]
patch, v3

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

I would still like the long function calls that wrap to either not wrap or wrap at each argument, as I mentioned in comment 7.

Otherwise, looks good. I grudgingly accept these incredibly long function names.

Comment 11

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5cc8a9432812

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/5cc8a9432812

Comment 13

[Nicholas Nethercote [inactive]]

Attachment: When possible, measure, don't compute, the size of imgFrame::mImageSurface.

This uses mallocSizeOf() for imgFrame::mImageSurface, because I'm pretty sure
it's always heap-allocated.  This covers the case that DMD complains about the
most.

Ones like mWinSurface and mQuartzSurface are more difficult because they are 
certainly sometimes heap-allocated, but possibly not always?  I can't tell and
I'm no gfx expert.  Maybe later.

Comment 14

[Joe Drew (not getting mail)]

Comment on attachment 685002 [details] [diff] [review]
When possible, measure, don't compute, the size of imgFrame::mImageSurface.

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

mOptSurface, mWinSurface, mQuartzSurface are all always heap-allocated, fwiw.

::: gfx/thebes/gfxASurface.cpp
@@ +652,5 @@
> +size_t
> +gfxASurface::SizeOfExcludingThis(nsMallocSizeOfFun aMallocSizeOf) const
> +{
> +    // njn: |cairo_surface_t mSurface?|  (try renaming it in order to find all
> +    // the places that access it)

?

Comment 15

[Nicholas Nethercote [inactive]]

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

(I removed the bogus "njn:" comment, which represented a line of inquiry that didn't go anywhere useful.)

Comment 16

[Nicholas Nethercote [inactive]]

Hmm, I just realized that the "When possible..." patch above was meant to be handled in bug 723827.  But since it was submitted and reviewed here and I've now landed it with this bug's number in the commit message, I guess we can just pretend that's what was intended all along.  I'll close bug 723827.

Comment 18

[Ed Morley [:emorley]]

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