Closed Bug 1074387 Opened 5 years ago Closed 5 years ago

Double-counting of memory, probably in webaudio

Categories

(Core :: Audio/Video, defect)

Other Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: jesup, Assigned: erahm)

References

()

Details

(Whiteboard: [MemShrink:P2])

Attachments

(3 files, 2 obsolete files)

Attached file memory-report.json.gz
Inbound debug build on linux64:

14:34]	jesup	[Parent 8555] ###!!! ASSERTION: Invalid value (486982224 / 485566176) for explicit/webaudio: 'false', file aboutMemory.js, line 0
[14:34]	jesup	[Parent 8555] ###!!! ASSERTION: Invalid value (486891568 / 485566176) for explicit/webaudio/audio-node: 'false', file aboutMemory.js, line 0
[14:35]	erahm	oh webaudio reporting, you are the bane of my existence
[14:35]	jesup	[Parent 8555] ###!!! ASSERTION: Invalid value (-150984400 / 485566176) for explicit/(22 tiny): 'false', file aboutMemory.js, line 0
[14:35]	jesup	[Parent 8555] ###!!! ASSERTION: Invalid value (-168642328 / 485566176) for explicit/(22 tiny)/heap-unclassified: 'false', file aboutMemory.js, line 0

This is caused by going to http://lotto.pch.com/pick-winning-numbers and then mousing over the buttons for a while.  It appears to create 1 or more audio streams per mouseover. I eventually got to 10000 streams, and a very busy Xeon with lag and audio glitches.

NOTE: a landing last night of assertions broke about:memory on debug builds when MSG is active (and it should, the operation being done was unsafe).  I'll attach a patch, and another patch I was trying though it's likely irrelevant to this.
Attached patch about_memory.patch (obsolete) — Splinter Review
Attached patch cleanup_msg_overload (obsolete) — Splinter Review
Attachment #8497017 - Attachment is obsolete: true
Uploaded the wrong patch for this one
Attachment #8497018 - Attachment is obsolete: true
Okay so the reason about:memory is going bonkers is this:

465.43 MB (100.0%) -- explicit
├──464.42 MB (99.78%) -- webaudio

Our heap-unclassified measurement blows up b/c there's more explicit reported than we actually measured being allocated. A DMD run would likely help track down where the extra reporting is happening. This definitely seems like a bug in the webaudio reporting itself, I'm not sure if Randell's patch fixes that though.

On the other hand this does highlight the problem of negative values being lumped into tiny (which I thought we fixed previously):

└──-142.89 MB (-30.70%) -- (22 tiny) [?!]
   ├─────3.10 MB (00.67%) ++ atom-tables
<snip>
   ├─────0.00 MB (00.00%) ── spell-check
   └──-159.67 MB (-34.31%) ── heap-unclassified [?!]
> which I thought we fixed previously
That was bug 1005489.
Whiteboard: [MemShrink]
Huh, I guess that was only diffs.
> On the other hand this does highlight the problem of negative values being
> lumped into tiny (which I thought we fixed previously):
> 
> └──-142.89 MB (-30.70%) -- (22 tiny) [?!]
>    ├─────3.10 MB (00.67%) ++ atom-tables
> <snip>
>    ├─────0.00 MB (00.00%) ── spell-check
>    └──-159.67 MB (-34.31%) ── heap-unclassified [?!]

That node is auto-expanded, though. That happens for any node that triggers a warning. Combine the auto-expanding with the warning at the top and the highlighting in red, and I'm content with the current behaviour. (Negative values are a much bigger issue in diffs because they're legitimate and frequent.)
Assignee: nobody → erahm
Summary: Huge negative memory sizes reported for heap-unclassified → Double-counting of memory, probably in webaudio
Whiteboard: [MemShrink] → [MemShrink:P2]
I'll do some DMD runs on OSX to see if I can track down the double reporting.
These appear to be primarily from |mozilla::dom::PannerNodeEngine::SizeOfIncludingThis| all pretty similar stacks:

>    #01: mozilla::dmd::ReportHelper(void const*, bool) (DMD.cpp:440, in libdmd.dylib)
>    #02: mozilla::MediaStreamGraphImpl::MallocSizeOf(void const*) (MediaStreamGraphImpl.h:660, in XUL)
>    #03: WebCore::HRTFElevation::sizeOfIncludingThis(unsigned long (*)(void const*)) const (FFTBlock.h:135, in XUL)
>    #04: WebCore::HRTFDatabase::sizeOfIncludingThis(unsigned long (*)(void const*)) const (HRTFDatabase.cpp:87, in XUL)
>    #05: WebCore::HRTFPanner::sizeOfIncludingThis(unsigned long (*)(void const*)) const (HRTFPanner.cpp:81, in XUL)
>    #06: mozilla::dom::PannerNodeEngine::SizeOfIncludingThis(unsigned long (*)(void const*)) const (PannerNode.cpp:203, in XUL)
>    #07: mozilla::AudioNodeStream::SizeOfAudioNodesIncludingThis(unsigned long (*)(void const*), mozilla::AudioNodeSizes&) const (AudioNodeEngine.h:373, in XUL)
>    #08: mozilla::MediaStreamGraphImpl::OneIteration(long long, long long, long long, long long) (MediaStreamGraph.cpp:1403, in XUL)

So it looks like maybe it's just |WebCore::HRTFElevation| that's getting multiply reported.
This seems to be due to HRTFDatabases being cached in a singleton [1]. We should either not report these at all (quick fix) or add a static memory reporter to HRTFDatabaseLoader and then add a manual PannerNode entry in |MediaStreamGraphImpl::CollectReports| [2].

Something like:

>    REPORT("explicit/webaudio/audio-node/PannerNode/engine-objects",
>           HRTFDatabaseLoader::SizeOfIncludingThis(),
>           "Memory used by AudioNode engine objects (Web Audio).");


[1] http://hg.mozilla.org/mozilla-central/annotate/e4cfacb76830/content/media/webaudio/blink/HRTFDatabaseLoader.cpp#l40
[2] http://hg.mozilla.org/mozilla-central/annotate/e4cfacb76830/content/media/MediaStreamGraph.cpp#l2902
This patch splits out reporting of HRTFDatabase memory to static reporter as that memory is shared. Output as follows:

284.38 MB (100.0%) -- explicit
├───62.32 MB (21.91%) -- webaudio
│   ├──62.31 MB (21.91%) -- audio-node
│   │  ├──49.18 MB (17.29%) -- PannerNode
│   │  │  ├──35.73 MB (12.57%) ── hrtf-databases
│   │  │  ├──13.33 MB (04.69%) ── engine-objects [111]
│   │  │  └───0.11 MB (00.04%) ++ (2 tiny)
│   │  ├──12.96 MB (04.56%) -- AudioBufferSourceNode
│   │  │  ├──12.87 MB (04.53%) ── dom-nodes [111]
│   │  │  └───0.09 MB (00.03%) ++ (2 tiny)
│   │  └───0.17 MB (00.06%) ++ (2 tiny)
│   └───0.01 MB (00.00%) ── audiocontext
├───32.83 MB (11.54%) ── heap-unclassified
Attachment #8502685 - Flags: review?(rjesup)
Attachment #8502685 - Flags: review?(rjesup) → review+
try server was busted, new try: https://tbpl.mozilla.org/?tree=Try&rev=ead34cfc673f
https://hg.mozilla.org/mozilla-central/rev/73e48ae73c32
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
There still seems to be some double-counting in explicit/webaudio/audio-node/AudioBufferSourceNode/dom-nodes, which I can trigger by visiting The Infinite Jukebox (e.g. http://labs.echonest.com/Uploader/index.html?trid=TRDUQAQ14B46F9D0FF) and playing a song there. It seems that each time the playback position jumps to a different part of the song, the memory reported for dom-nodes jumps up a bit, until it finally exceeds the amount of explicitly allocated memory. Would that be handled here, too, or should I file a follow-up bug?
(In reply to JanH from comment #15)
> Would that be handled here, too, or should I file a follow-up bug?

The patch in this bug landed quite a while ago, so please file a new bug.  Thanks for the report!
Okay, filed as bug 1144358.
You need to log in before you can comment on or make changes to this bug.