961441 - AudioQueueMemoryFunctor should use MallocSizeOf to measure its memory consumption

Status: RESOLVED FIXED

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

Description

[Nicholas Nethercote [inactive]]

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.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

I'll take this since I'm already in the code for bug 960873.

Comment 2

[Nicholas Nethercote [inactive]]

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.

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Attachment: audio-malloc-sizeof.patch

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

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

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 6

[Chris Pearce [:cpearce (Not reading bugmail)]]

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).

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

Attachment: audio-malloc-sizeof.patch

> 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

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8363240 [details] [diff] [review]
audio-malloc-sizeof.patch

Carry forward r+ from :cpearce.  Thanks for the review!

Comment 9

[Nicholas Nethercote [inactive]]

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.

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

Attachment: audio-malloc-sizeof.patch v3

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

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

Attachment: audio-malloc-sizeof.patch v4

Minor update to the last patch.  I missed completely removing the virtual keyword on SizeOfAudioQueue.

https://tbpl.mozilla.org/?tree=Try&rev=8dfdd4d81ec6

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

Attachment: audio-malloc-sizeof.patch v5

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

Comment 14

[Ben Kelly [:bkelly, not reviewing]]

https://tbpl.mozilla.org/?tree=Try&rev=bf8f54161435

Comment 15

[Chris Pearce [:cpearce (Not reading bugmail)]]

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.

Comment 16

[Nicholas Nethercote [inactive]]

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.

Comment 17

[Chris Pearce [:cpearce (Not reading bugmail)]]

(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.

Comment 18

[Ben Kelly [:bkelly, not reviewing]]

(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.

Comment 19

[Ben Kelly [:bkelly, not reviewing]]

https://hg.mozilla.org/integration/b2g-inbound/rev/76f23abe25fc

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/76f23abe25fc