Use mallocSizeOf in the source image memory reporters

RESOLVED FIXED in mozilla13

Status

()

Core
ImageLib
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

unspecified
mozilla13
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

5 years ago
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.

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.
Attachment #582736 - Flags: review?(jmuizelaar)
(Assignee)

Updated

5 years ago
Summary: Use malloCSizeOf in the source image memory reporters → Use mallocSizeOf in the source image memory reporters
(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.
(Assignee)

Comment 2

5 years ago
> 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?
(Assignee)

Comment 3

5 years ago
jrmuizel, how should I move forward with this bug?  Is Joe back from vacation?
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. :)
Attachment #582736 - Flags: review?(jmuizelaar) → review-
(Assignee)

Updated

5 years ago
Blocks: 723827
(Assignee)

Comment 5

5 years ago
Created attachment 594073 [details] [diff] [review]
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.
Assignee: nobody → n.nethercote
Attachment #582736 - Attachment is obsolete: true
Attachment #594073 - Flags: review?(joe)
> imgFrame::SizeOfExcludingThisWithComputedFallbackIfHeap().

Wow, when I advocated for the longer method name "SizeOfExcludingThis", I did not think it would come to this!
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.
Attachment #594073 - Flags: review?(joe) → review-
(Assignee)

Comment 8

5 years ago
( > +    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.
(Assignee)

Comment 9

5 years ago
Created attachment 598121 [details] [diff] [review]
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|.
Attachment #594073 - Attachment is obsolete: true
Attachment #598121 - Flags: review?(joe)
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.
Attachment #598121 - Flags: review?(joe) → review+
(Assignee)

Comment 11

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/5cc8a9432812
https://hg.mozilla.org/mozilla-central/rev/5cc8a9432812
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
Blocks: 723088
(Assignee)

Comment 13

5 years ago
Created attachment 685002 [details] [diff] [review]
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.
Attachment #685002 - Flags: review?(joe)
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)

?
Attachment #685002 - Flags: review?(joe) → review+
(Assignee)

Comment 15

5 years ago
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.)
(Assignee)

Comment 16

5 years ago
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.
(Assignee)

Updated

5 years ago
Duplicate of this bug: 723827
https://hg.mozilla.org/mozilla-central/rev/8f83993bd902
You need to log in before you can comment on or make changes to this bug.