Web Audio can potentially use a considerable amount of memory. We need to be able to measure it in about:memory.

We already have the "explicit/media/decoded-audio" reporter: http://mxr.mozilla.org/mozilla-central/source/content/media/MediaDecoder.cpp#1750 Does that miss some stuff?

(In reply to comment #1) > We already have the "explicit/media/decoded-audio" reporter: > > http://mxr.mozilla.org/mozilla-central/source/content/media/MediaDecoder.cpp#1750 > > Does that miss some stuff? It misses everything related to Web Audio. :-) Web Audio is a new API which we have implemented, see AudioContext() and friends.

Web audio involves setting up these giant graphs of audio processing filters and then pushing sound through them, or something.

Yes. The implementation lives in <http://mxr.mozilla.org/mozilla-central/source/content/media/webaudio/>.

I'm making this a P2, but I think we should quickly raise this to a P1 once some of our current P1s have landed.

So, considering we don't want a fiasco like bug 936784 to happen again, and because we don't want to waste njn's and other people time again, we should implement this sooner than later.

I'll take care of this in the holidays!

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #7) > I'll take care of this in the holidays! Thank you, Ehsan! (I was just starting to look for an owner.)

Comment on attachment 8349180 [details] [diff] [review] Part 1: Add a memory reporter for AudioContexts; r=roc,njn Review of attachment 8349180 [details] [diff] [review]: ----------------------------------------------------------------- Let's get started! This adds a memory reporter for AudioContext. I'll add more for AudioNodes, etc. Please review this carefully to make sure I've accounted for everything correctly. Thanks!

Comment on attachment 8349180 [details] [diff] [review] Part 1: Add a memory reporter for AudioContexts; r=roc,njn Review of attachment 8349180 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/webaudio/AudioContext.cpp @@ +656,5 @@ > + amount += mDecodeJobs[i]->SizeOfExcludingThis(aMallocSizeOf); > + } > + // AudioNodes are tracked separately > + amount += mActiveNodes.SizeOfExcludingThis(nullptr, aMallocSizeOf); > + amount += mPannerNodes.SizeOfExcludingThis(nullptr, aMallocSizeOf); If we're going to track AudioNodes separately, isn't this going to double-count some nodes? ::: content/media/webaudio/MediaBufferDecoder.cpp @@ +641,5 @@ > + amount += mSuccessCallback->SizeOfIncludingThis(aMallocSizeOf); > + } > + if (mFailureCallback) { > + amount += mFailureCallback->SizeOfIncludingThis(aMallocSizeOf); > + } How do we avoid double-counting callbacks that are shared across WebAudioDecodeJobs? Sorry if this is obvious.

Comment on attachment 8349180 [details] [diff] [review] Part 1: Add a memory reporter for AudioContexts; r=roc,njn Review of attachment 8349180 [details] [diff] [review]: ----------------------------------------------------------------- This is a good start, though I suspect you'll need to change AudioContext from a uni-reporter to a multi-reporter when you measure other things like AudioNodes. (More about that below.) BTW, I checked this from a memory reporting POV, but didn't look closely at all the SizeOf{In,Ex}cludingThis() functions because I don't know much about the data structures involved. Can you copy+paste sample output from about:memory? And have you verified the reporter with DMD? ::: content/media/webaudio/AudioBuffer.cpp @@ +258,5 @@ > > +size_t > +AudioBuffer::SizeOfIncludingThis(MallocSizeOf aMallocSizeOf) const > +{ > + size_t amount = aMallocSizeOf(this); FWIW, I usually use |n| for this variable. ::: content/media/webaudio/AudioContext.cpp @@ +81,5 @@ > uint32_t aNumberOfChannels, > uint32_t aLength, > float aSampleRate) > + : MemoryUniReporter("explicit/webaudio/audiocontext", KIND_HEAP, UNITS_BYTES, > + "Memory used by AudioContext objects (Web Audio)") Full stop at the end of the sentence, please. @@ +654,5 @@ > + amount += mDecodeJobs.SizeOfExcludingThis(aMallocSizeOf); > + for (uint32_t i = 0; i < mDecodeJobs.Length(); ++i) { > + amount += mDecodeJobs[i]->SizeOfExcludingThis(aMallocSizeOf); > + } > + // AudioNodes are tracked separately Are you planning to measure AudioNodes here in a subsequent patch? https://wiki.mozilla.org/Memory_Reporting#Other_Considerations discusses how to do multiple measurements, in the sentence starting "Sometimes you might want to split the measurements...". If you do plan to make multiple reports, it would probably be best to change AudioContext so it's not a MemoryUniReporter. Instead you would inherit directly from nsIMemoryReporter, and implement CollectReports() directly. You'll be able to do multiple |aHandleReport->Callback()| calls within CollectReports(). See nsCycleCollector::CollectReports() for a good example. ::: content/media/webaudio/AudioContext.h @@ +63,5 @@ > class WaveShaperNode; > class PeriodicWave; > > class AudioContext MOZ_FINAL : public nsDOMEventTargetHelper, > + public mozilla::MemoryUniReporter, Do these lines in AudioContext.cpp need updating? NS_IMPL_ADDREF_INHERITED(AudioContext, nsDOMEventTargetHelper) NS_IMPL_RELEASE_INHERITED(AudioContext, nsDOMEventTargetHelper) I'm not sure what the effect is of having two non-abstract classes in AudioContext's inheritance chain. I'm actually writing a patch right now to get rid of MemoryUniReporter precisely because the INHERITED stuff is complicated and the amount of boilerplate it avoids is actually pretty small. (This point is moot if you change AudioContext to report multiple measurements.) @@ +249,5 @@ > void RemoveFromDecodeQueue(WebAudioDecodeJob* aDecodeJob); > void ShutdownDecoder(); > > + size_t SizeOfIncludingThis(mozilla::MallocSizeOf aMallocSizeOf) const; > + virtual int64_t Amount() MOZ_OVERRIDE; Nit: I don't think this needs to be virtual, due to the MOZ_FINAL. (None of the existing reporters have |virtual| in this position.) (This point is also moot if you change AudioContext to report multiple measurements.)

> > + // AudioNodes are tracked separately > > + amount += mActiveNodes.SizeOfExcludingThis(nullptr, aMallocSizeOf); > > + amount += mPannerNodes.SizeOfExcludingThis(nullptr, aMallocSizeOf); > > If we're going to track AudioNodes separately, isn't this going to > double-count some nodes? The first arg to nsTHashtable::SizeOfExcludingThis() is the function that measures things hanging off each entry. Because it's nullptr in this case, these lines are just measuring the entry storage. But it still remains to be seen how the AudioNodes will be measured...

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #11) > Comment on attachment 8349180 [details] [diff] [review] > Part 1: Add a memory reporter for AudioContexts; r=roc,njn > > Review of attachment 8349180 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: content/media/webaudio/AudioContext.cpp > @@ +656,5 @@ > > + amount += mDecodeJobs[i]->SizeOfExcludingThis(aMallocSizeOf); > > + } > > + // AudioNodes are tracked separately > > + amount += mActiveNodes.SizeOfExcludingThis(nullptr, aMallocSizeOf); > > + amount += mPannerNodes.SizeOfExcludingThis(nullptr, aMallocSizeOf); > > If we're going to track AudioNodes separately, isn't this going to > double-count some nodes? What Nick said. > ::: content/media/webaudio/MediaBufferDecoder.cpp > @@ +641,5 @@ > > + amount += mSuccessCallback->SizeOfIncludingThis(aMallocSizeOf); > > + } > > + if (mFailureCallback) { > > + amount += mFailureCallback->SizeOfIncludingThis(aMallocSizeOf); > > + } > > How do we avoid double-counting callbacks that are shared across > WebAudioDecodeJobs? Sorry if this is obvious. There is no sharing, we create one of these objects per call to decodeAudioData, see this code in objdir/dom/bindings/AudioContextBinding.cpp: arg1 = new DecodeSuccessCallback(tempRoot, mozilla::dom::GetIncumbentGlobal()); It is the actual JS object that is shared, but we're not counting stuff on the JS heap here at all.

(In reply to Nicholas Nethercote [:njn] from comment #12) > This is a good start, though I suspect you'll need to change AudioContext > from a uni-reporter to a multi-reporter when you measure other things like > AudioNodes. (More about that below.) > > BTW, I checked this from a memory reporting POV, but didn't look closely at > all the SizeOf{In,Ex}cludingThis() functions because I don't know much about > the data structures involved. Makes sense! > Can you copy+paste sample output from about:memory? It's quite boring at this point! ├────────5,440 B (00.00%) ── webaudio/audiocontext [2] ├────────1,520 B (00.00%) ── history-links-hashtable ├────────────0 B (00.00%) ── icu └────────────0 B (00.00%) ── spell-check > And have you verified the reporter with DMD? Hmm no. Why do I need to do that? (FWIW I stepped through all of this code in the debugger to "verify" my patch.) > ::: content/media/webaudio/AudioBuffer.cpp > @@ +258,5 @@ > > > > +size_t > > +AudioBuffer::SizeOfIncludingThis(MallocSizeOf aMallocSizeOf) const > > +{ > > + size_t amount = aMallocSizeOf(this); > > FWIW, I usually use |n| for this variable. Yes I saw that, and I don't like it. ;-) > ::: content/media/webaudio/AudioContext.cpp > @@ +81,5 @@ > > uint32_t aNumberOfChannels, > > uint32_t aLength, > > float aSampleRate) > > + : MemoryUniReporter("explicit/webaudio/audiocontext", KIND_HEAP, UNITS_BYTES, > > + "Memory used by AudioContext objects (Web Audio)") > > Full stop at the end of the sentence, please. Sure. > @@ +654,5 @@ > > + amount += mDecodeJobs.SizeOfExcludingThis(aMallocSizeOf); > > + for (uint32_t i = 0; i < mDecodeJobs.Length(); ++i) { > > + amount += mDecodeJobs[i]->SizeOfExcludingThis(aMallocSizeOf); > > + } > > + // AudioNodes are tracked separately > > Are you planning to measure AudioNodes here in a subsequent patch? Yes! > https://wiki.mozilla.org/Memory_Reporting#Other_Considerations discusses how o_O there's a wiki page? I just read nsIMemoryReporter.idl and grepped around! > to do multiple measurements, in the sentence starting "Sometimes you might > want to split the measurements...". I'm actually not sure how that applies here. > If you do plan to make multiple reports, it would probably be best to change > AudioContext so it's not a MemoryUniReporter. Instead you would inherit > directly from nsIMemoryReporter, and implement CollectReports() directly. > You'll be able to do multiple |aHandleReport->Callback()| calls within > CollectReports(). See nsCycleCollector::CollectReports() for a good example. The reason why I want to measure AudioNode's separately is that it's not possible to access all of them from the context object. So the way that this would work is that each AudioNode would be its own MemoryUniReporter and would be responsible to report its own memory. I'm not sure how to measure the off-main-thread stuff yet, but that would probably go in its own unireporter. > ::: content/media/webaudio/AudioContext.h > @@ +63,5 @@ > > class WaveShaperNode; > > class PeriodicWave; > > > > class AudioContext MOZ_FINAL : public nsDOMEventTargetHelper, > > + public mozilla::MemoryUniReporter, > > Do these lines in AudioContext.cpp need updating? > > NS_IMPL_ADDREF_INHERITED(AudioContext, nsDOMEventTargetHelper) > NS_IMPL_RELEASE_INHERITED(AudioContext, nsDOMEventTargetHelper) Hmm, no, why do you ask? Is it because that MemoryUniReporter has its own refcount? That's just broken as I mentioned in bug 946870. > I'm not sure what the effect is of having two non-abstract classes in > AudioContext's inheritance chain. I'm actually writing a patch right now to > get rid of MemoryUniReporter precisely because the INHERITED stuff is > complicated and the amount of boilerplate it avoids is actually pretty small. Hmm, wait, what's going to replace MemoryUniReporter? What will happen to my patch, given that I'm going to keep this a uni reporter? > (This point is moot if you change AudioContext to report multiple > measurements.) > > @@ +249,5 @@ > > void RemoveFromDecodeQueue(WebAudioDecodeJob* aDecodeJob); > > void ShutdownDecoder(); > > > > + size_t SizeOfIncludingThis(mozilla::MallocSizeOf aMallocSizeOf) const; > > + virtual int64_t Amount() MOZ_OVERRIDE; > > Nit: I don't think this needs to be virtual, due to the MOZ_FINAL. (None > of the existing reporters have |virtual| in this position.) Hmm, Amount() is a virtual method in MemoryUniReporter, so it will be virtual here no matter whether I include the keyword or not. Including the keyword makes it easier to spot (and I guess the override keyword doesn't hurt either!)

> And have you verified the reporter with DMD? > > Hmm no. Why do I need to do that? (FWIW I stepped through all of this code > in the debugger to "verify" my patch.) DMD can tell you if any block is double-counted. Unlikely with this patch, but more likely once you start counting things from multiple threads. > The reason why I want to measure AudioNode's separately is that it's not > possible to access all of them from the context object. Ok. The comments about AudioNodes should explain _why_ you're not measuring them here. > So the way that this would work is that each AudioNode would be its own MemoryUniReporter and > would be responsible to report its own memory. How many AudioNodes do we typically have? If it's more than a few (or maybe a few dozen) then having that many reporters isn't ideal. > Hmm, wait, what's going to replace MemoryUniReporter? What will happen to > my patch, given that I'm going to keep this a uni reporter? Nothing will replace it, I'm just converting them to vanilla nsIMemoryReporters, which means they have to implement CollectReports(). Don't worry about changing, you'll probably land this before I finish and so I'll be happy to change it then.

(In reply to Nicholas Nethercote [:njn] from comment #16) > > And have you verified the reporter with DMD? > > > > Hmm no. Why do I need to do that? (FWIW I stepped through all of this code > > in the debugger to "verify" my patch.) > > DMD can tell you if any block is double-counted. Unlikely with this patch, > but more likely once you start counting things from multiple threads. Oh, that's very neat! I'll try that. > > The reason why I want to measure AudioNode's separately is that it's not > > possible to access all of them from the context object. > > Ok. The comments about AudioNodes should explain _why_ you're not measuring > them here. More than "AudioNodes are tracked separately"? > > So the way that this would work is that each AudioNode would be its own MemoryUniReporter and > > would be responsible to report its own memory. > > How many AudioNodes do we typically have? If it's more than a few (or maybe > a few dozen) then having that many reporters isn't ideal. It should be a few dozen. I'd really like to avoid having to track all of the nodes in the graph from the context if possible. > > Hmm, wait, what's going to replace MemoryUniReporter? What will happen to > > my patch, given that I'm going to keep this a uni reporter? > > Nothing will replace it, I'm just converting them to vanilla > nsIMemoryReporters, which means they have to implement CollectReports(). > Don't worry about changing, you'll probably land this before I finish and so > I'll be happy to change it then. Ok, great. Thanks!

> More than "AudioNodes are tracked separately"? It's non-obvious why they're tracked separately -- this seems like an obvious spot to track them. Something like "AudioNodes are not owned by the main thread, so must be tracked separately." Or whatever the reason is.

(In reply to comment #18) > > More than "AudioNodes are tracked separately"? > > It's non-obvious why they're tracked separately -- this seems like an obvious > spot to track them. Something like "AudioNodes are not owned by the main > thread, so must be tracked separately." Or whatever the reason is. Oh I see what you mean now. OK, will do.

In AudioBuffer.cpp, should there be a += here? if (mSharedChannels) { mSharedChannels->SizeOfExcludingThis(aMallocSizeOf); }

> In AudioBuffer.cpp, should there be a += here? Yes! This kind of bug gives me nightmares, because it's hard to detect -- DMD will think that memory is measured, but the simple logic error causes it to not actually be.

(In reply to comment #21) > > In AudioBuffer.cpp, should there be a += here? > > Yes! This kind of bug gives me nightmares, because it's hard to detect -- DMD > will think that memory is measured, but the simple logic error causes it to not > actually be. Perhaps we can annotate these functions with MOZ_WARN_UNUSED_RESULT?

> Perhaps we can annotate these functions with MOZ_WARN_UNUSED_RESULT? Good idea!

(In reply to comment #23) > > Perhaps we can annotate these functions with MOZ_WARN_UNUSED_RESULT? > > Good idea! Bug 956400.

Does this new reporter cover the memory that was climbing in bug 936784? I don't know if that bug involved nodes or something else...

(In reply to Nicholas Nethercote [:njn] from comment #27) > Does this new reporter cover the memory that was climbing in bug 936784? I > don't know if that bug involved nodes or something else... Looking at the patch, not yet.

Yep, that's coming up.

I'm sorry guys, I'm getting less and less free time on my weekends and I ended up dropping the ball here again. Can somebody else take the rest of the work please?

I think it's bad form to land a patch in a bug and then leave the bug open indefinitely for additional patches. (If nothing else it makes a mockery of the "target milestone" field.) So I created bug 967817 for the remaining work. Thanks, Ehsan, for getting this started.