Closed Bug 967817 Opened 6 years ago Closed 6 years ago

Finish memory reporters for Web Audio

Categories

(Core :: Web Audio, defect, P1)

x86
macOS
defect

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: njn, Assigned: erahm)

References

(Blocks 1 open bug)

Details

(Whiteboard: [MemShrink:P2])

Attachments

(1 file, 3 obsolete files)

Bug 884368 added a memory reporter for web audio that measures part of its memory consumption. It needs to be completed. Nodes are the main thing that's missing.
WIP - Started descending the AudioNode tree to see what all needs to be accounted for, traversal is not complete.
Assignee: nobody → erahm
Status: NEW → ASSIGNED
After speaking with Ehsan we have settled on the following proposal for implementation and presentation.

Implementation:
- Add a memory reporter for each MediaStreamGraph which reports web audio memory usage
- Initiate a request to calculate audio memory usage on the MSG thread
- On the MSG thread filter the list of MediaStreams to just AudioNodeStreams, use those to report the memory used by each WebAudio node (AudioNode, AudioNodeEngine, AudioNodeStream)

Presentation:
- Report the DOM node, stream, and engine components for each AudioNode type as follows:

├───17.13 MB (06.85%) -- webaudio
│   ├──17.13 MB (06.85%) -- audionode
│   │  ├──15.38 MB (06.15%) -- ConvolverNode
│   │  │  ├──15.01 MB (06.00%) ── Engine
│   │  │  └───0.37 MB (00.15%) -- (2 tiny)
│   │  │      ├──0.37 MB (00.15%) ── DOMNode
│   │  │      └──0.00 MB (00.00%) ── Stream
│   │  └───1.76 MB (00.70%) -- (4 tiny)
│   │      ├──1.68 MB (00.67%) -- AudioBufferSourceNode
│   │      │  ├──1.68 MB (00.67%) ── DOMNode [12]
│   │      │  ├──0.01 MB (00.00%) ── Stream [12]
│   │      │  └──0.00 MB (00.00%) ── Engine [12]
│   │      ├──0.06 MB (00.02%) -- AnalyserNode
│   │      │  ├──0.05 MB (00.02%) ── DOMNode
│   │      │  ├──0.00 MB (00.00%) ── Stream
│   │      │  └──0.00 MB (00.00%) ── Engine
│   │      ├──0.01 MB (00.00%) -- GainNode
│   │      │  ├──0.01 MB (00.00%) ── Stream [12]
│   │      │  ├──0.00 MB (00.00%) ── DOMNode [12]
│   │      │  └──0.00 MB (00.00%) ── Engine [12]
│   │      └──0.00 MB (00.00%) -- AudioDestinationNode
│   │         ├──0.00 MB (00.00%) ── Stream [3]
│   │         ├──0.00 MB (00.00%) ── DOMNode [3]
│   │         └──0.00 MB (00.00%) ── Engine [3]
│   └───0.00 MB (00.00%) ── audiocontext [2]
> - Initiate a request to calculate audio memory usage on the MSG thread

How does this result get communicated to the main thread?
(In reply to Nicholas Nethercote [:njn] from comment #3)
> > - Initiate a request to calculate audio memory usage on the MSG thread
> 
> How does this result get communicated to the main thread?

With standard thread synchronization techniques and a member variable. Something like:

<main_thread>
collect_reports:
   monitor.lock();
   needs_reports = true;
   monitor.wait();
   foreach (report in mReports) report_it

<MSG_thread>
    monitor.lock();
    if (needs_report) get_the_reports(mReports)
    monitor.notify();
This patch adds memory reporting for WebAudio nodes and their associated objects. I apologize for the size, but I couldn't think of a reasonable way to break this up and still have it make sense.
Attachment #8406415 - Flags: review?(roc)
Attachment #8406415 - Flags: review?(n.nethercote)
Attachment #8406415 - Flags: review?(ehsan)
Attachment #8397366 - Attachment is obsolete: true
Comment on attachment 8406415 [details] [diff] [review]
Finish memory reporters for Web Audio

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

Mostly looks great!

::: content/media/AudioNodeEngine.h
@@ +45,5 @@
>      }
>      ~Storage() { free(mDataToFree); }
> +    size_t SizeOfExcludingThis(MallocSizeOf aMallocSizeOf) const
> +    {
> +      // NB: mSampleData might not be owned, if it is it just points to

"if it just"

::: content/media/MediaStreamGraphImpl.h
@@ +598,5 @@
> +   */
> +  Monitor mMemoryReportMonitor;
> +  bool mNeedsMemoryReport;
> +  size_t mAudioMemoryUsage;
> +  nsTArray<AudioNodeMemoryUsage> mAudioNodeMemoryReports;

Reorder these so that mAudioNodeMemoryReports is after Monitor, followed by the size_t and the bool.

::: content/media/SharedBuffer.h
@@ +20,5 @@
>  class ThreadSharedObject {
>  public:
>    NS_INLINE_DECL_THREADSAFE_REFCOUNTING(ThreadSharedObject)
>  
> +  bool isShared() { return mRefCnt.get() > 1; }

IsShared

::: content/media/webaudio/AudioBufferSourceNode.h
@@ +109,5 @@
>    virtual void NotifyMainThreadStateChanged() MOZ_OVERRIDE;
>  
> +  virtual const char* NodeType() const
> +  {
> +    return "AudioBufferSourceNode";

Can't we get these using WebIDL somehow instead of having to add a new virtual method?
Attachment #8406415 - Flags: review?(roc) → review-
Comment on attachment 8406415 [details] [diff] [review]
Finish memory reporters for Web Audio

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

Thanks for the quick review! I hold off on changes until I hear from njn and/or Ehsan.

::: content/media/AudioNodeEngine.h
@@ +45,5 @@
>      }
>      ~Storage() { free(mDataToFree); }
> +    size_t SizeOfExcludingThis(MallocSizeOf aMallocSizeOf) const
> +    {
> +      // NB: mSampleData might not be owned, if it is it just points to

I'm not sure of a better way to state this. Basically mSampleData could be external memory that's not owned, or it points to some offset in mDataToFree and is already accounted for. So maybe:

"NB: mSampleData is either not owned or points to memory already accounted for in mDataToFree"

::: content/media/webaudio/AudioBufferSourceNode.h
@@ +109,5 @@
>    virtual void NotifyMainThreadStateChanged() MOZ_OVERRIDE;
>  
> +  virtual const char* NodeType() const
> +  {
> +    return "AudioBufferSourceNode";

If there's a reasonable way to do this from the scope of AudioNode I'm all for it. I'm unfamiliar with introspection in WebIDL, do you have any idea of how that would work?
(In reply to Eric Rahm [:erahm] from comment #7)
> ::: content/media/webaudio/AudioBufferSourceNode.h
> @@ +109,5 @@
> >    virtual void NotifyMainThreadStateChanged() MOZ_OVERRIDE;
> >  
> > +  virtual const char* NodeType() const
> > +  {
> > +    return "AudioBufferSourceNode";
> 
> If there's a reasonable way to do this from the scope of AudioNode I'm all
> for it. I'm unfamiliar with introspection in WebIDL, do you have any idea of
> how that would work?

Boris, is this possible?
Flags: needinfo?(bzbarsky)
Comment on attachment 8406415 [details] [diff] [review]
Finish memory reporters for Web Audio

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

roc's review here is enough as far as I'm concerned :-)
Attachment #8406415 - Flags: review?(ehsan)
(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #8)
> (In reply to Eric Rahm [:erahm] from comment #7)
> > ::: content/media/webaudio/AudioBufferSourceNode.h
> > @@ +109,5 @@
> > >    virtual void NotifyMainThreadStateChanged() MOZ_OVERRIDE;
> > >  
> > > +  virtual const char* NodeType() const
> > > +  {
> > > +    return "AudioBufferSourceNode";
> > 
> > If there's a reasonable way to do this from the scope of AudioNode I'm all
> > for it. I'm unfamiliar with introspection in WebIDL, do you have any idea of
> > how that would work?
> 
> Boris, is this possible?

Looks like there is no great way of using the bindings layer for this.
Flags: needinfo?(bzbarsky)
To summarize our IRC discussion:

If there is a JS wrapper, you can get the WebIDL interface name from it with js::GetObjectClass(obj)->name.

But if not, then you have to get it from the C++ object, which is really just an rtti problem...  You can do it via virtual calls, via storing the name in a member of the superclass, or via rtti (which we turn off).
We could create a wrapper and ask for its name, but that's obviously stupid, so OK.
Comment on attachment 8406415 [details] [diff] [review]
Finish memory reporters for Web Audio

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

Sorry, I misread the AudioNodeEngine comment and it's fine. So r+ with the comments addressed except for the AudioNodeEngine comment and the WebIDL comment.
Attachment #8406415 - Flags: review- → review+
Comment on attachment 8406415 [details] [diff] [review]
Finish memory reporters for Web Audio

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

Looks pretty good! f+ for now because I'd like to see it after addressing the comment below.

There are a bunch of TODO and TODO(ER) comments. Are you planning to fix these before landing? It would be nice if it's possible.

::: content/media/AudioNodeEngine.h
@@ +352,5 @@
> +    return aMallocSizeOf(this) + SizeOfExcludingThis(aMallocSizeOf);
> +  }
> +
> +  size_t SizeOfIncludingThis(MallocSizeOf aMallocSizeOf,
> +                             AudioNodeMemoryUsage& aUsage) const

This is an unusual pattern. Normally SizeOf functions either return a single size_t, or return void and fill in a struct. But not both...

@@ +358,5 @@
> +    aUsage.mEngineMemory = SizeOfIncludingThis(aMallocSizeOf);
> +    aUsage.mDomNodeMemory = mNode->SizeOfIncludingThis(aMallocSizeOf);
> +    aUsage.mNodeType = mNode->NodeType();
> +
> +    // Return the cumulative memory usage.

Is "cumulative" the wrong word? That suggests to me the notion of adding up sizes over time, which I don't think is happening here. Would "combined" or "total" be better? Or can this just be removed?

::: content/media/AudioNodeStream.cpp
@@ +56,5 @@
> +AudioNodeStream::SizeOfIncludingThis(MallocSizeOf aMallocSizeOf,
> +                                     AudioNodeMemoryUsage& aUsage) const
> +{
> +  size_t amount = aMallocSizeOf(this);
> +  amount += ProcessedMediaStream::SizeOfExcludingThis(aMallocSizeOf);

I'm confused by the overlap between this function and AudioNodeStream::SizeOfExcludingThis(). Is only one needed?

::: content/media/AudioNodeStream.h
@@ +157,5 @@
>  
> +  virtual size_t SizeOfExcludingThis(MallocSizeOf aMallocSizeOf) const MOZ_OVERRIDE;
> +  virtual size_t SizeOfIncludingThis(MallocSizeOf aMallocSizeOf) const MOZ_OVERRIDE;
> +
> +  size_t SizeOfIncludingThis(MallocSizeOf aMallocSizeOf, AudioNodeMemoryUsage& aUsage) const;

And I'm not sure why this one isn't virtual. But maybe it'll be merged with the other one above?

::: content/media/MediaDecoder.cpp
@@ +1769,5 @@
>    REPORT("explicit/media/resources", resources,
>           "Memory used by media resources including streaming buffers, caches, "
>           "etc.");
>  
> +#undef REPORT

Mmm, nice.

::: content/media/MediaStreamGraph.cpp
@@ +2672,5 @@
> +void
> +MediaStreamGraphImpl::Destroy()
> +{
> +  // NB: Unregistering will cause |this| to be deleted. Don't do anything after
> +  //     this call.

There's an idiom called "kungFuDeathGrip" which makes this nicer. You have a local nsRefPtr<>, assign |this| to it, and that keeps it alive until the end of the method. There are other examples in this file you can look at.

@@ +2673,5 @@
> +MediaStreamGraphImpl::Destroy()
> +{
> +  // NB: Unregistering will cause |this| to be deleted. Don't do anything after
> +  //     this call.
> +  UnregisterStrongMemoryReporter(this);

I've avoided creating UnregisterStrongMemoryReporter(), because my idea was for things to be simple:
- Use a weak pointer if the reporter manages its own lifetime
- Use a strong pointer if the reporter lives until shutdown

Adding UnregisterStrongMemoryReporter() muddies this.

Could MSGImpl hold an nsRefPtr<> to itself? Then you could use RegisterWeakMemoryReporter() in the constructor, and Destroy() would call UnregisterWeakMemoryReporter(this) and then null out the self-reference, triggering the destructor (unless you add a kungFuDeathGrip, in which case it would trigger at the end of the method).

I *think* this would work. Does it sound reasonable? (Ah! There's a SelfReference class you could use :)

@@ +2791,5 @@
> +    const AudioNodeMemoryUsage& usage = mAudioNodeMemoryReports[i];
> +    const char* const node_type =  usage.mNodeType.get();
> +
> +    nsPrintfCString dom_node_path("explicit/webaudio/audionode/%s/DOMNode",
> +                                  node_type);

Memory reporter paths use lower case and hyphens, so that last segment should be "dom-node", or perhaps "dom-nodes".

And I'd use "audio-node" instead of "audionode".

Also, use camelCaps for dom_node_path, and the similar cases below.

@@ +2795,5 @@
> +                                  node_type);
> +    REPORT(dom_node_path, usage.mDomNodeMemory,
> +           "Memory used by AudioNode DOM objects (Web Audio).");
> +
> +    nsPrintfCString engine_path("explicit/webaudio/audionode/%s/Engine",

Change "Engine" to "engine", or perhaps "engine-objects".

@@ +2800,5 @@
> +                                node_type);
> +    REPORT(engine_path, usage.mEngineMemory,
> +           "Memory used by AudioNode engine objects (Web Audio).");
> +
> +    nsPrintfCString stream_path("explicit/webaudio/audionode/%s/Stream",

Change "Stream" to "stream", or perhaps "stream-objects".

::: content/media/MediaStreamGraph.h
@@ +232,5 @@
> +struct AudioNodeMemoryUsage
> +{
> +  size_t mDomNodeMemory;
> +  size_t mStreamMemory;
> +  size_t mEngineMemory;

Nit: structs like these are usually called |FooSizes|, and the members within don't need a common suffix, so

::: content/media/MediaStreamGraphImpl.h
@@ +123,5 @@
>    explicit MediaStreamGraphImpl(bool aRealtime);
>    virtual ~MediaStreamGraphImpl();
>  
> +  /**
> +   * Unregisteres memory reporting and deletes this instance. This should be

"Unregisters".

@@ +597,5 @@
> +   * Used to signal that a memory report has been requested.
> +   */
> +  Monitor mMemoryReportMonitor;
> +  bool mNeedsMemoryReport;
> +  size_t mAudioMemoryUsage;

AFAICT, this member is never used in a meaningful way. Seems like you can remove it. That might allow other things to be simplified, e.g. you wouldn't need to have SizeOf functions that fill in |usage| *and* return a size_t?

@@ +598,5 @@
> +   */
> +  Monitor mMemoryReportMonitor;
> +  bool mNeedsMemoryReport;
> +  size_t mAudioMemoryUsage;
> +  nsTArray<AudioNodeMemoryUsage> mAudioNodeMemoryReports;

I'd call this |mStreamSizes|, which makes it clear that there is one per Stream.

::: content/media/StreamBuffer.h
@@ +213,5 @@
> +    amount += mTracks.SizeOfExcludingThis(aMallocSizeOf);
> +    for (size_t i = 0; i < mTracks.Length(); i++) {
> +      amount += mTracks[i]->SizeOfIncludingThis(aMallocSizeOf);
> +    }
> +    return 0;

Return |amount|!

::: content/media/webaudio/AudioNode.cpp
@@ +89,5 @@
> +  for (size_t i = 0; i < mInputNodes.Length(); i++) {
> +    amount += mInputNodes[i].SizeOfExcludingThis(aMallocSizeOf);
> +  }
> +
> +  // Just measure the array, we don't want to double-count the elements.

Add a comment about where they are measured?

::: content/media/webaudio/BiquadFilterNode.cpp
@@ +265,5 @@
> +  size_t amount = AudioNode::SizeOfExcludingThis(aMallocSizeOf);
> +
> +  if (mFrequency) {
> +    amount += mFrequency->SizeOfIncludingThis(aMallocSizeOf);
> +  }

Using |amount += x ? x->SizeOfIncludingThis(aMallocSizeOf) : 0| would make functions like this one a lot more concise.

::: content/media/webaudio/ChannelMergerNode.h
@@ +32,5 @@
> +  }
> +
> +  virtual size_t SizeOfIncludingThis(MallocSizeOf aMallocSizeOf) const MOZ_OVERRIDE
> +  {
> +    return aMallocSizeOf(this) + SizeOfExcludingThis(aMallocSizeOf);

There's no SizeOfExcludingThis() function for this class. I guess it inherits it. Is this deliberate?

::: content/media/webaudio/ChannelSplitterNode.h
@@ +32,5 @@
> +  }
> +
> +  virtual size_t SizeOfIncludingThis(MallocSizeOf aMallocSizeOf) const MOZ_OVERRIDE
> +  {
> +    return aMallocSizeOf(this) + SizeOfExcludingThis(aMallocSizeOf);

Ditto.

::: content/media/webaudio/blink/HRTFPanner.cpp
@@ +69,5 @@
>  {
>      MOZ_COUNT_DTOR(HRTFPanner);
>  }
>  
> +size_t HRTFPanner::sizeOfIncludingThis(mozilla::MallocSizeOf aMallocSizeOf)const

Nit: space before |const|.
Attachment #8406415 - Flags: review?(n.nethercote) → feedback+
The SelfReference class in an abomination that I introduced.  Please try not to use it if at all possible.  ;-)
(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #15)
> The SelfReference class in an abomination that I introduced.  Please try not
> to use it if at all possible.  ;-)

RE: RefCounting shenanigans

So given that:
a) The intent of this bug is not to rewrite MSG's life-cycle
b) Memory reporting forces us to ref count something we don't want to ref count
c) Registering as a strong memory reporter actually potentially messes with the lifecycle in a dangerous way (think crash on shutdown)

I'd like to not register as a strong memory reporter (as njn suggested), but I'd also like to not do SelfReferencing (as ehsan pointed out is an abomination).

So my proposal which is certainly janky, but I see it as less janky is to just lie and pretend we're ref counting when registering the memory reporter:

create_MSGi:
    MSGi msg = new MSGi;
    // NB: The memory reporter requires msg to have a ref when registering,
    //     we'll add that ref for now to satisfy this requirement.
    RefPtr<MSGi> fake_ref = MSGi;
    RegisterWeakMemoryReporter(fake_ref);
    unused << fake_ref.forget();

Or we could make RegisterWeakMemoryReporter not care if the thing being registered already has a ref.
Comment on attachment 8406415 [details] [diff] [review]
Finish memory reporters for Web Audio

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

Thanks for the review! I think the only contentious point is how to deal w/ referencing (I posted a separate comment for that). I'll clean up as suggested and clear out the remaining TODOs and get a new review up with changes from both reviews.

::: content/media/AudioNodeEngine.h
@@ +352,5 @@
> +    return aMallocSizeOf(this) + SizeOfExcludingThis(aMallocSizeOf);
> +  }
> +
> +  size_t SizeOfIncludingThis(MallocSizeOf aMallocSizeOf,
> +                             AudioNodeMemoryUsage& aUsage) const

I will switch to void for these instances.

@@ +358,5 @@
> +    aUsage.mEngineMemory = SizeOfIncludingThis(aMallocSizeOf);
> +    aUsage.mDomNodeMemory = mNode->SizeOfIncludingThis(aMallocSizeOf);
> +    aUsage.mNodeType = mNode->NodeType();
> +
> +    // Return the cumulative memory usage.

This will go away.

::: content/media/AudioNodeStream.cpp
@@ +56,5 @@
> +AudioNodeStream::SizeOfIncludingThis(MallocSizeOf aMallocSizeOf,
> +                                     AudioNodeMemoryUsage& aUsage) const
> +{
> +  size_t amount = aMallocSizeOf(this);
> +  amount += ProcessedMediaStream::SizeOfExcludingThis(aMallocSizeOf);

The other one's are dead code, I'll remove them.

::: content/media/AudioNodeStream.h
@@ +157,5 @@
>  
> +  virtual size_t SizeOfExcludingThis(MallocSizeOf aMallocSizeOf) const MOZ_OVERRIDE;
> +  virtual size_t SizeOfIncludingThis(MallocSizeOf aMallocSizeOf) const MOZ_OVERRIDE;
> +
> +  size_t SizeOfIncludingThis(MallocSizeOf aMallocSizeOf, AudioNodeMemoryUsage& aUsage) const;

The other two will be removed, we currently only report memory usage for audio related streams so there's no need to be virtual.

::: content/media/MediaStreamGraph.cpp
@@ +2791,5 @@
> +    const AudioNodeMemoryUsage& usage = mAudioNodeMemoryReports[i];
> +    const char* const node_type =  usage.mNodeType.get();
> +
> +    nsPrintfCString dom_node_path("explicit/webaudio/audionode/%s/DOMNode",
> +                                  node_type);

Sounds good, I'll change them to the preferred style. Perhaps we need a memory reporting style guide?

::: content/media/MediaStreamGraph.h
@@ +232,5 @@
> +struct AudioNodeMemoryUsage
> +{
> +  size_t mDomNodeMemory;
> +  size_t mStreamMemory;
> +  size_t mEngineMemory;

That sounds clearer, I'll update it.

::: content/media/MediaStreamGraphImpl.h
@@ +597,5 @@
> +   * Used to signal that a memory report has been requested.
> +   */
> +  Monitor mMemoryReportMonitor;
> +  bool mNeedsMemoryReport;
> +  size_t mAudioMemoryUsage;

Yes this can go.

@@ +598,5 @@
> +   */
> +  Monitor mMemoryReportMonitor;
> +  bool mNeedsMemoryReport;
> +  size_t mAudioMemoryUsage;
> +  nsTArray<AudioNodeMemoryUsage> mAudioNodeMemoryReports;

This seems reasonable, although I'd call it |mAudioStreamSizes| as it only measures web audio related data.

::: content/media/StreamBuffer.h
@@ +213,5 @@
> +    amount += mTracks.SizeOfExcludingThis(aMallocSizeOf);
> +    for (size_t i = 0; i < mTracks.Length(); i++) {
> +      amount += mTracks[i]->SizeOfIncludingThis(aMallocSizeOf);
> +    }
> +    return 0;

Ooooh nice find! Stubbing out functions fails again :(

::: content/media/webaudio/AudioNode.cpp
@@ +89,5 @@
> +  for (size_t i = 0; i < mInputNodes.Length(); i++) {
> +    amount += mInputNodes[i].SizeOfExcludingThis(aMallocSizeOf);
> +  }
> +
> +  // Just measure the array, we don't want to double-count the elements.

They're measured right here ;) I'll add a note that the whole graph is measured in MSG via the streams.

::: content/media/webaudio/BiquadFilterNode.cpp
@@ +265,5 @@
> +  size_t amount = AudioNode::SizeOfExcludingThis(aMallocSizeOf);
> +
> +  if (mFrequency) {
> +    amount += mFrequency->SizeOfIncludingThis(aMallocSizeOf);
> +  }

I find that to be less readable (more dense, why are we adding zero here?), but to each his own.

::: content/media/webaudio/ChannelMergerNode.h
@@ +32,5 @@
> +  }
> +
> +  virtual size_t SizeOfIncludingThis(MallocSizeOf aMallocSizeOf) const MOZ_OVERRIDE
> +  {
> +    return aMallocSizeOf(this) + SizeOfExcludingThis(aMallocSizeOf);

Yes, there's nothing of interest to add in |SizeOfExcludingThis|

::: content/media/webaudio/ChannelSplitterNode.h
@@ +32,5 @@
> +  }
> +
> +  virtual size_t SizeOfIncludingThis(MallocSizeOf aMallocSizeOf) const MOZ_OVERRIDE
> +  {
> +    return aMallocSizeOf(this) + SizeOfExcludingThis(aMallocSizeOf);

Same deal.

::: content/media/webaudio/blink/HRTFPanner.cpp
@@ +69,5 @@
>  {
>      MOZ_COUNT_DTOR(HRTFPanner);
>  }
>  
> +size_t HRTFPanner::sizeOfIncludingThis(mozilla::MallocSizeOf aMallocSizeOf)const

Huh, I'm surprised that compiles, will fix.
:roc I updated the member var order and fixed the casing of IsShared. I don't think you really need to go through this again, but it's a big enough change you might want to.
:njn I did pretty much everything we discussed. Removed unused vars / dead code, renamed size tracking struct, cleared out TODOs, removed changes to nsIMemoryReporting.

In the end :njn and :mccr8 convinced me to go with self-referencing to support memory reporting as the least-bad option.
Attachment #8407954 - Flags: review?(roc)
Attachment #8407954 - Flags: review?(n.nethercote)
Attachment #8406415 - Attachment is obsolete: true
Attachment #8407951 - Attachment is obsolete: true
Comment on attachment 8407954 [details] [diff] [review]
Finish memory reporters for Web Audio.

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

Looks great! Thanks for slogging through this. Sorry about the ref-counting madness, but everybody working on Gecko has to have their ref-counting baptism of fire at some point, and you've now made it through yours :)

::: content/media/MediaStreamGraph.cpp
@@ +2664,5 @@
>  #endif
>  
>    mCurrentTimeStamp = mInitialTimeStamp = mLastMainThreadUpdate = TimeStamp::Now();
> +
> +  mSelfRef = this;

Is it possible to remove this and do |, mSelfRef(this)| in the initializer list above?

::: content/media/MediaStreamGraphImpl.h
@@ +120,5 @@
>     * output.  Those objects currently only support audio, and are used to
>     * implement OfflineAudioContext.  They do not support MediaStream inputs.
>     */
>    explicit MediaStreamGraphImpl(bool aRealtime);
>    virtual ~MediaStreamGraphImpl();

Can you make this private, to ensure that nobody calls it directly? I'm not sure if it'll work, but is worth a try.

@@ +597,5 @@
> +   * Used to signal that a memory report has been requested.
> +   */
> +  Monitor mMemoryReportMonitor;
> +  /**
> +   * Handles ref counting for memory reporting.

Can you expand this comment slightly? Something like this:

"This class uses manual memory management, and all pointers to it are raw pointers. However, in order for it to implement nsIMemoryReporter, it needs to implement nsISupports and so be ref-counted. So it maintains a single nsRefPtr to itself, giving it a ref-count of 1 during its entire lifetime, and Destroy() nulls this self-reference in order to trigger self-deletion."
Attachment #8407954 - Flags: review?(n.nethercote) → review+
Moved mSelfRef init to initializer list, made constructor private and updated doc as requested.

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/7c18257d0163
https://hg.mozilla.org/mozilla-central/rev/7c18257d0163
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in before you can comment on or make changes to this bug.