Closed Bug 1188705 Opened 4 years ago Closed 4 years ago

Two image memory reporting tweaks

Categories

(Core :: ImageLib, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: njn, Assigned: njn)

Details

Attachments

(3 files, 1 obsolete file)

Just a couple of minor image memory reporting tweaks.
It's unused.
Attachment #8640258 - Flags: review?(seth)
Comment on attachment 8640258 [details] [diff] [review]
(part 1) - Remove gfxASurface::GetMemoryLocation()

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

LGTM.
Attachment #8640258 - Flags: review?(seth) → review+
It's unused.
Attachment #8640270 - Flags: review?(seth)
Attachment #8640258 - Attachment is obsolete: true
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.)
Attachment #8640272 - Flags: review?(seth)
Comment on attachment 8640270 [details] [diff] [review]
(part 1) - Remove gfxASurface::GetMemoryLocation()

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

(Assuming nothing changed here.)
Attachment #8640270 - Flags: review?(seth) → review+
Attachment #8640271 - Flags: review?(seth) → review+
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.
> (Assuming nothing changed here.)

Whoops, I think I just uploaded the same patch twice. Sorry.
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.
Attachment #8640272 - Flags: review?(seth) → review+
(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?
> > +#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.
You need to log in before you can comment on or make changes to this bug.