Closed
Bug 1407549
Opened 7 years ago
Closed 7 years ago
Bad performance of TrackUnionStream::CopyTrackData
Categories
(Core :: Audio/Video: MediaStreamGraph, defect, P2)
Core
Audio/Video: MediaStreamGraph
Tracking
()
RESOLVED
FIXED
mozilla61
Tracking | Status | |
---|---|---|
firefox61 | --- | fixed |
People
(Reporter: pehrsons, Assigned: pehrsons)
References
(Blocks 1 open bug)
Details
Attachments
(11 files, 1 obsolete file)
376.66 KB,
image/png
|
Details | |
198.37 KB,
image/png
|
Details | |
197.32 KB,
image/png
|
Details | |
(appear.in SFU, 6 remote peers) Percentage of before and after, highlighting biggest allocation wins
448.50 KB,
image/png
|
Details | |
352.81 KB,
image/png
|
Details | |
59 bytes,
text/x-review-board-request
|
padenot
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
padenot
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
padenot
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
padenot
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
padenot
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
padenot
:
review+
|
Details |
STR:
1 Go to https://jsfiddle.net/pehrsons/xaLgt3md/
2 Click "Start" and allow the capture
3 Click the "Add" buttons until your audio crackles (my 2016 Macbook Pro gets really bad at 7000)
4 Profile the content process
Expected
Anything on the graph thread needs to be really fast
Actual
TrackUnionStream::CopyTrackData shows up very very high in the profile (see attachment)
Assignee | ||
Updated•7 years ago
|
Rank: 18
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → apehrson
Rank: 18 → 12
Assignee | ||
Updated•7 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 1•7 years ago
|
||
I did some profiling to compare my WIP patches on linux.
This is with Intel Vtune Amplifier's basic hotspot analyser at 1ms sampling frequency.
The test case is a non-e10s optimized debug (so with assertions) build running appear.in with 3 remote peers on a macbook air.
TrackUnionStream::CopyTrackData takes 7.6% of the audio driver's DataCallback.
Assignee | ||
Comment 2•7 years ago
|
||
After the patches, TrackUnionStream::CopyTrackData takes 6.6% of the audio driver's DataCallback.
By drilling down I can confirm that any alloc's and dalloc's are gone. Most of the time according to the profiler is spent in things like nsTArray::Length(), Image::AddRef and similar.
Assignee | ||
Updated•7 years ago
|
Attachment #8963090 -
Attachment description: bug_1407549-before.png → [before] 3 remote peers appear.in SFU
Assignee | ||
Updated•7 years ago
|
Attachment #8963094 -
Attachment description: bug_1407549-after.png → [after] 3 remote peers appear.in SFU
Assignee | ||
Comment 3•7 years ago
|
||
Similarly but with 6 remote peers running on a macbook pro
Comment 4•7 years ago
|
||
Can you attach the patch?
Assignee | ||
Comment 5•7 years ago
|
||
Sorry, they were still WIP, I think what I have now is ready though.
Assignee | ||
Updated•7 years ago
|
Attachment #8963109 -
Attachment is obsolete: true
Assignee | ||
Comment 6•7 years ago
|
||
Assignee | ||
Comment 7•7 years ago
|
||
Assignee | ||
Comment 8•7 years ago
|
||
Comment on attachment 8963638 [details]
(appear.in SFU, 6 remote peers) Percentage of before and after, highlighting biggest allocation wins
NB I highlighted MediaPipelineTransmit::PipelineListener::NewData because it's external to TrackUnionStream, so shouldn't really be counted towards the total.
The highlighted MediaSegment calls went from 6.1% to 1.9% of the DataCallback.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 14•7 years ago
|
||
mozreview-review |
Comment on attachment 8963651 [details]
Bug 1407549 - Give MediaSegment's chunk array a default capacity.
https://reviewboard.mozilla.org/r/232544/#review238178
::: dom/media/MediaSegment.h:231
(Diff revision 1)
> // The latest principal handle that the MediaStreamGraph has processed for
> // this segment.
> PrincipalHandle mLastPrincipalHandle;
> };
>
> +#define DEFAULT_SEGMENT_CAPACITY 16
const size_t.
Why 16? Have you gathered some number or something? If it's arbitrary, it's fine, just add comment stating this.
Attachment #8963651 -
Flags: review?(padenot) → review+
Comment 15•7 years ago
|
||
mozreview-review |
Comment on attachment 8963652 [details]
Bug 1407549 - Avoid array operations that can cause alloc/dalloc in MediaSegment.
https://reviewboard.mozilla.org/r/232546/#review238210
Attachment #8963652 -
Flags: review?(padenot) → review+
Comment 16•7 years ago
|
||
mozreview-review |
Comment on attachment 8963653 [details]
Bug 1407549 - Make segments allocate chunk storage locally.
https://reviewboard.mozilla.org/r/232548/#review238212
Attachment #8963653 -
Flags: review?(padenot) → review+
Comment 17•7 years ago
|
||
mozreview-review |
Comment on attachment 8963654 [details]
Bug 1407549 - Simplify MSGImpl::AudioTrackPresent.
https://reviewboard.mozilla.org/r/232550/#review238214
Attachment #8963654 -
Flags: review?(padenot) → review+
Comment 18•7 years ago
|
||
mozreview-review |
Comment on attachment 8963655 [details]
Bug 1407549 - Avoid copying principal handles as much as possible.
https://reviewboard.mozilla.org/r/232552/#review238216
Attachment #8963655 -
Flags: review?(padenot) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 25•7 years ago
|
||
mozreview-review |
Comment on attachment 8964873 [details]
Bug 1407549 - Copy AudioSegment by constructor.
https://reviewboard.mozilla.org/r/233540/#review239184
Attachment #8964873 -
Flags: review?(padenot) → review+
Comment hidden (mozreview-request) |
Comment 27•7 years ago
|
||
Pushed by pehrsons@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/a3ce13ac0563
Give MediaSegment's chunk array a default capacity. r=padenot
https://hg.mozilla.org/integration/autoland/rev/a95b0c5c8e9f
Avoid array operations that can cause alloc/dalloc in MediaSegment. r=padenot
https://hg.mozilla.org/integration/autoland/rev/c9e04ecaaa46
Make segments allocate chunk storage locally. r=padenot
https://hg.mozilla.org/integration/autoland/rev/7204e90dce1a
Simplify MSGImpl::AudioTrackPresent. r=padenot
https://hg.mozilla.org/integration/autoland/rev/cb6f4b677a37
Avoid copying principal handles as much as possible. r=padenot
https://hg.mozilla.org/integration/autoland/rev/f312d31f9c3c
Copy AudioSegment by constructor. r=padenot
Comment 28•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/a3ce13ac0563
https://hg.mozilla.org/mozilla-central/rev/a95b0c5c8e9f
https://hg.mozilla.org/mozilla-central/rev/c9e04ecaaa46
https://hg.mozilla.org/mozilla-central/rev/7204e90dce1a
https://hg.mozilla.org/mozilla-central/rev/cb6f4b677a37
https://hg.mozilla.org/mozilla-central/rev/f312d31f9c3c
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in
before you can comment on or make changes to this bug.
Description
•