Closed Bug 961441 Opened 6 years ago Closed 6 years ago

AudioQueueMemoryFunctor should use MallocSizeOf to measure its memory consumption

Categories

(Core :: Audio/Video, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: njn, Assigned: bkelly)

References

Details

(Keywords: perf, Whiteboard: [c=memory p=2 s= u=] [MemShrink:P2])

Attachments

(1 file, 4 obsolete files)

AudioQueueMemoryFunctor computes its memory consumption analytically. It should instead measure it via SizeOfMalloc, so that (a) slop bytes from jemalloc are included, and (b) it integrates with DMD. (See https://wiki.mozilla.org/Memory_Reporting for more details.)

This is relevant to bug 960873, where large amounts of slop were falling into the "heap-unclassified" bucket.
OS: Gonk (Firefox OS) → All
Priority: P1 → P3
Hardware: ARM → All
Summary: AudioQueueMemoryFunctor should use SizeOfMalloc to measure its memory consumption → AudioQueueMemoryFunctor should use MallocSizeOf to measure its memory consumption
I'll take this since I'm already in the code for bug 960873.
Assignee: nobody → bkelly
Status: NEW → ASSIGNED
Thanks, bkelly!

You'll want to start by adding this inside class AudioQueueMemoryFunctor:

    MOZ_DEFINE_MALLOC_SIZE_OF(MallocSizeOf)

You can then call MallocSizeOf() on each heap block to get its size. I assume/hope the existing code measures each block exactly once.

I'd like to review the patch once you're done; having a review from an audio expert would also be good.
Whiteboard: [MemShrink] → [MemShrink][c=memory p=2 s= u=]
Keywords: perf
Whiteboard: [MemShrink][c=memory p=2 s= u=] → [c=memory p=2 s= u=] [MemShrink]
Blocks: 682195
It appears this also effects the decoded-video reporter.  I wrote up bug 962154 to track that.  I also noticed that this is covered by the older bug 682195.  I decided to leave the two child bugs for now since the fixes are likely to be a bit different given how video uses PlanarYCbCrImage.
Attached patch audio-malloc-sizeof.patch (obsolete) — Splinter Review
This is a minimal change to use MallocSizeOf() for the AudioData buffer.  It does not include any of the memory used in the AudioData object itself for the buffer meta data.

  https://tbpl.mozilla.org/?tree=Try&rev=9fd5cc97fb92
Attachment #8363091 - Flags: review?(n.nethercote)
Attachment #8363091 - Flags: review?(cpearce)
Oh, I also verified poppit memory looks suitably horrific with this patch applied:

70.10 MB (100.0%) -- explicit
├──28.55 MB (40.73%) -- media
│  ├──28.55 MB (40.73%) ── decoded-audio
Comment on attachment 8363091 [details] [diff] [review]
audio-malloc-sizeof.patch

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

::: content/media/MediaDecoderReader.h
@@ +590,3 @@
>      virtual void* operator()(void* anObject) {
>        const AudioData* audioData = static_cast<const AudioData*>(anObject);
> +      mResult += MallocSizeOf(audioData->mAudioData);

Do we also need to add in audioData->mBuffer->Data(). That's used on android (I think).
Attachment #8363091 - Flags: review?(cpearce) → review+
Attached patch audio-malloc-sizeof.patch (obsolete) — Splinter Review
> Do we also need to add in audioData->mBuffer->Data(). That's used on android
> (I think).

Good point!  Here is an updated patch that factors in audioData->mAudioBuffer.

Note, I think we want MallocSizeOf(audioData->mAudioBuffer) without the Data() since the SharedBuffer uses an in-place constructor on top of the buffer allocation.  Data() just returns |this+1|.  So the allocation pointer is really the |this| pointer of the mAudioBuffer.

I also verified that its safe to pass |nullptr| to MallocSizeOf() as it occurred to me I wasn't checking here.

https://tbpl.mozilla.org/?tree=Try&rev=065b13b05f60
Attachment #8363091 - Attachment is obsolete: true
Attachment #8363091 - Flags: review?(n.nethercote)
Attachment #8363240 - Flags: review?(n.nethercote)
Comment on attachment 8363240 [details] [diff] [review]
audio-malloc-sizeof.patch

Carry forward r+ from :cpearce.  Thanks for the review!
Attachment #8363240 - Flags: review+
Whiteboard: [c=memory p=2 s= u=] [MemShrink] → [c=memory p=2 s= u=] [MemShrink:P2]
Comment on attachment 8363240 [details] [diff] [review]
audio-malloc-sizeof.patch

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

You're definitely heading in the right direction, but I'd like another look before landing. Some of the comments below pre-date your patch, but we might as well fix them while you're here.

BTW, https://wiki.mozilla.org/Memory_Reporting might be an interesting read for you.

::: content/media/MediaDecoderReader.h
@@ +582,5 @@
>    }
>  
>    class AudioQueueMemoryFunctor : public nsDequeFunctor {
>    public:
>      AudioQueueMemoryFunctor() : mResult(0) {}

Is mSize a better name?

@@ +590,4 @@
>      virtual void* operator()(void* anObject) {
>        const AudioData* audioData = static_cast<const AudioData*>(anObject);
> +      mResult += MallocSizeOf(audioData->mAudioData);
> +      mResult += MallocSizeOf(audioData->mAudioBuffer);

Generally it's better for classes to measure their own memory consumption. Can you please add a function like this to class SharedBuffer:

  size_t SizeOfIncludingThis(MallocSizeOf aMallocSizeOf) const {
    return aMallocSizeOf(this);
  }

and then call this function on audioData->mAudioData.

You could also do likewise with AudioDataValue, but that class is trivial, so the direct MallocSizeOf() call is probably ok.

@@ +594,4 @@
>        return nullptr;
>      }
>  
>      int64_t mResult;

Can you change this to |size_t|? Memory reporters take an int64_t amount, but we usually measure using size_t and then convert to int64_t at the last minute.

@@ +596,5 @@
>  
>      int64_t mResult;
>    };
>  
>    virtual int64_t AudioQueueMemoryInUse() {

While you're here, can you rename this SizeOfAudioQueue()? It's a convention that these measuring functions be named with a SizeOf prefix.
Attachment #8363240 - Flags: review?(n.nethercote) → feedback+
Duplicate of this bug: 682195
No longer blocks: 682195
Attached patch audio-malloc-sizeof.patch v3 (obsolete) — Splinter Review
Updated patch based on review feedback.  Chris, I'm asking for re-review since there are more extensive changes now.

(In reply to Nicholas Nethercote [:njn] from comment #9)
> @@ +596,5 @@
> >  
> >      int64_t mResult;
> >    };
> >  
> >    virtual int64_t AudioQueueMemoryInUse() {
> 
> While you're here, can you rename this SizeOfAudioQueue()? It's a convention
> that these measuring functions be named with a SizeOf prefix.

Done and I also propagated the change to the methods named the same in MediaDecoder and MediaDecoderStateMachine.

Note, I also dropped the |virtual| keyword from MediaDecoderReader::AudioQueueMemoryInUse().  It looks like its a holdover from dash which has since been removed.

The video memory reporter has similar issues, but I resisted the urge to make the changes here.  I figure that can be done in bug 962154.

https://tbpl.mozilla.org/?tree=Try&rev=54e55aedc34f
Attachment #8363240 - Attachment is obsolete: true
Attachment #8363711 - Flags: review?(n.nethercote)
Attachment #8363711 - Flags: review?(cpearce)
Attached patch audio-malloc-sizeof.patch v4 (obsolete) — Splinter Review
Minor update to the last patch.  I missed completely removing the virtual keyword on SizeOfAudioQueue.

https://tbpl.mozilla.org/?tree=Try&rev=8dfdd4d81ec6
Attachment #8363711 - Attachment is obsolete: true
Attachment #8363711 - Flags: review?(n.nethercote)
Attachment #8363711 - Flags: review?(cpearce)
Attachment #8363716 - Flags: review?(n.nethercote)
Attachment #8363716 - Flags: review?(cpearce)
Sorry for the bug spam, but here's yet another minor update.  I missed converting some int64_t return types to size_t on the various SizeOfAudioQueue methods.

https://tbpl.mozilla.org/?tree=Try&rev=bfafefcfae86
Attachment #8363716 - Attachment is obsolete: true
Attachment #8363716 - Flags: review?(n.nethercote)
Attachment #8363716 - Flags: review?(cpearce)
Attachment #8363775 - Flags: review?(n.nethercote)
Attachment #8363775 - Flags: review?(cpearce)
Comment on attachment 8363775 [details] [diff] [review]
audio-malloc-sizeof.patch v5

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

c=cpearce with the changes suggested below.

::: content/media/MediaDecoder.h
@@ +640,5 @@
>    virtual nsresult GetBuffered(dom::TimeRanges* aBuffered);
>  
>    // Returns the size, in bytes, of the heap memory used by the currently
>    // queued decoded video and audio data.
>    virtual int64_t VideoQueueMemoryInUse();

Does VideoQueueMemoryInUse() need to be changed to a size_t too? In another bug/patch would be logical place to do that.

::: content/media/MediaDecoderReader.h
@@ +159,5 @@
> +    return aMallocSizeOf(this) + SizeOfExcludingThis(aMallocSizeOf);
> +  }
> +
> +  size_t SizeOfExcludingThis(MallocSizeOf aMallocSizeOf) const {
> +    size_t size = aMallocSizeOf(mAudioData);

Can't you just fold these into one function:

size_t SizeOf(MallocSizeOf aMallocSizeOf) const {
  return aMallocSizeOf(this) +
         aMallocSizeOf(mAudioData) +
         aMallocSizeOf(mAudioBuffer);
}

Since SharedBuffer::SizeOfIncludingThis() just returns aMallocSizeof(this), it's no different than calling aMallocSizeOf(mAudioBuffer), right?

Then there's only one SizeOf() function which always does The Right Thing, and the caller doesn't have to reason about which one they should call.

I assume that aMallocSizeOf(nullptr) cheaply evaluates to 0? If not, then it should!

::: content/media/SharedBuffer.h
@@ +42,5 @@
>                   "SharedBuffers should be at least 4-byte aligned");
>      return p.forget();
>    }
>  
> +  size_t SizeOfIncludingThis(MallocSizeOf aMallocSizeOf) const {

You don't need this if you make the change to a single SizeOf() function in MediaDecoderReader.
Attachment #8363775 - Flags: review?(cpearce) → review+
Comment on attachment 8363775 [details] [diff] [review]
audio-malloc-sizeof.patch v5

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

::: content/media/MediaDecoderReader.h
@@ +159,5 @@
> +    return aMallocSizeOf(this) + SizeOfExcludingThis(aMallocSizeOf);
> +  }
> +
> +  size_t SizeOfExcludingThis(MallocSizeOf aMallocSizeOf) const {
> +    size_t size = aMallocSizeOf(mAudioData);

Some classes need both the Including and Excluding forms, but this one doesn't, so folding them together is fine. But please call the folded form SizeOfIncludingThis() rather than just SizeOf().

And aMallocSizeOf(nullptr) does cheaply return 0.

::: content/media/SharedBuffer.h
@@ +42,5 @@
>                   "SharedBuffers should be at least 4-byte aligned");
>      return p.forget();
>    }
>  
> +  size_t SizeOfIncludingThis(MallocSizeOf aMallocSizeOf) const {

I asked bkelly to add this. In general, it's better if classes measure they're own memory consumption, rather than other classes doing it for them. Encapsulation and all that.
Attachment #8363775 - Flags: review?(n.nethercote) → review+
(In reply to Nicholas Nethercote [:njn] from comment #16)
> Comment on attachment 8363775 [details] [diff] [review]
> audio-malloc-sizeof.patch v5
> 
> Review of attachment 8363775 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/media/MediaDecoderReader.h
> @@ +159,5 @@
> > +    return aMallocSizeOf(this) + SizeOfExcludingThis(aMallocSizeOf);
> > +  }
> > +
> > +  size_t SizeOfExcludingThis(MallocSizeOf aMallocSizeOf) const {
> > +    size_t size = aMallocSizeOf(mAudioData);
> 
> Some classes need both the Including and Excluding forms, but this one
> doesn't, so folding them together is fine. But please call the folded form
> SizeOfIncludingThis() rather than just SizeOf().
> 
> And aMallocSizeOf(nullptr) does cheaply return 0.
 
Sounds good.

> ::: content/media/SharedBuffer.h
> @@ +42,5 @@
> >                   "SharedBuffers should be at least 4-byte aligned");
> >      return p.forget();
> >    }
> >  
> > +  size_t SizeOfIncludingThis(MallocSizeOf aMallocSizeOf) const {
> 
> I asked bkelly to add this. In general, it's better if classes measure
> they're own memory consumption, rather than other classes doing it for them.
> Encapsulation and all that.

Fair enough. I approve of encapsulation.
(In reply to Chris Pearce (:cpearce) from comment #15)
> Does VideoQueueMemoryInUse() need to be changed to a size_t too? In another
> bug/patch would be logical place to do that.

Yes, I was thinking bug 962154 would be the best place to do that.

Thanks for the review!  I'll fold those two methods in AudioData together and get this landed.
https://hg.mozilla.org/mozilla-central/rev/76f23abe25fc
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
You need to log in before you can comment on or make changes to this bug.