1188705 - Two image memory reporting tweaks

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Nicholas Nethercote [inactive]]

Just a couple of minor image memory reporting tweaks.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Remove gfxASurface::GetMemoryLocation()

It's unused.

Comment 2

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

Comment on attachment 8640258 [details] [diff] [review]
(part 1) - Remove gfxASurface::GetMemoryLocation()

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

LGTM.

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Remove gfxASurface::GetMemoryLocation()

It's unused.

Comment 4

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Remove unused SizeOfDecodedWithComputedFallbackIfHeap declaration

Comment 5

[Nicholas Nethercote [inactive]]

Attachment: (part 3) - Simplify imgFrame::SizeOfExcludingThis()

imgFrame::SizeOfExcludingThis() measures heap and non-heap memory in a very
complex way. This patch simplifies it and removes gfxMemoryLocation in the
process. (gfxMemoryLocation::OUT_OF_PROCESS was unused.)

Comment 6

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

Comment on attachment 8640270 [details] [diff] [review]
(part 1) - Remove gfxASurface::GetMemoryLocation()

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

(Assuming nothing changed here.)

Comment 7

[Nicholas Nethercote [inactive]]

In imgFrame::AddSizeOfExcludingThis() I'm rather skeptical about these lines:

>  if (mImageSurface) {
>    heapSize += aMallocSizeOf(mImageSurface);
>  }
>  if (mOptSurface) {
>    heapSize += aMallocSizeOf(mOptSurface);
>  }

Calling aMallocSizeOf directly on those fields seems suspicious. I would have expected SourceSurface and DataSourceSurface to have SizeOfIncludingThis() methods... AFAICT, SourceSurface contains a UserData, which contains a pointer to entries of some kind.

But that's beyond the scope of this bug, the patches for which don't change functionality in any way.

Comment 8

[Nicholas Nethercote [inactive]]

> (Assuming nothing changed here.)

Whoops, I think I just uploaded the same patch twice. Sorry.

Comment 9

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

Comment on attachment 8640272 [details] [diff] [review]
(part 3) - Simplify imgFrame::SizeOfExcludingThis()

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

r+ with the nits below.

::: image/Image.h
@@ +7,5 @@
>  #define mozilla_image_Image_h
>  
>  #include "mozilla/MemoryReporting.h"
>  #include "mozilla/TimeStamp.h"
> +#include "gfx2DGlue.h"

I guess we still need it for other stuff?

::: image/SurfaceCache.h
@@ +13,5 @@
>  
>  #include "mozilla/Maybe.h"           // for Maybe
>  #include "mozilla/MemoryReporting.h" // for MallocSizeOf
>  #include "mozilla/HashFunctions.h"   // for HashGeneric and AddToHash
> +#include "gfx2DGlue.h"

Here, too: still needed?

::: image/imgFrame.cpp
@@ +1116,5 @@
>  }
>  
> +void
> +imgFrame::AddSizeOfExcludingThis(MallocSizeOf aMallocSizeOf,
> +                                 size_t& heapSize, size_t& nonHeapSize) const

Probably should use |aHeapSizeOut| and |aNonHeapSizeOut| here. Or you can use |oHeapSize| and |oNonHeapSize| - I won't tell.

::: image/imgFrame.h
@@ +264,5 @@
>    already_AddRefed<SourceSurface> GetSurface();
>    already_AddRefed<DrawTarget> GetDrawTarget();
>  
> +  void AddSizeOfExcludingThis(MallocSizeOf aMallocSizeOf,
> +                              size_t& heapSize, size_t& nonHeapSize) const;

Should update the parameter names here too.

Comment 10

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

(In reply to Nicholas Nethercote [:njn] from comment #7)
> In imgFrame::AddSizeOfExcludingThis() I'm rather skeptical about these lines:
> 
> >  if (mImageSurface) {
> >    heapSize += aMallocSizeOf(mImageSurface);
> >  }
> >  if (mOptSurface) {
> >    heapSize += aMallocSizeOf(mOptSurface);
> >  }
> 
> Calling aMallocSizeOf directly on those fields seems suspicious. I would
> have expected SourceSurface and DataSourceSurface to have
> SizeOfIncludingThis() methods... AFAICT, SourceSurface contains a UserData,
> which contains a pointer to entries of some kind.
> 
> But that's beyond the scope of this bug, the patches for which don't change
> functionality in any way.

That does seem sketchy. Maybe a followup in Core::Graphics is in order?

Comment 11

[Nicholas Nethercote [inactive]]

> > +#include "gfx2DGlue.h"
> 
> I guess we still need it for other stuff?

Not sure. It's so hard to tell without pulling out include-what-you-use. I just removed the comment because it's clearly incorrect after my change.

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cda0eccc0f7d
https://hg.mozilla.org/integration/mozilla-inbound/rev/bef9528b79b6
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a11ad23d2c5

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/cda0eccc0f7d
https://hg.mozilla.org/mozilla-central/rev/bef9528b79b6
https://hg.mozilla.org/mozilla-central/rev/2a11ad23d2c5