Closed Bug 1407549 Opened 6 years ago Closed 6 years ago

Bad performance of TrackUnionStream::CopyTrackData

Categories

(Core :: Audio/Video: MediaStreamGraph, defect, P2)

Product:
Core
Component:
Audio/Video: MediaStreamGraph
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla61
Tracking Flags:
Tracking Status
firefox61 --- fixed

People

(Reporter: pehrsons, Assigned: pehrsons)

References

(Blocks 1 open bug)

Details

Attachments

(11 files, 1 obsolete file)

Screenshot of profiling results in Instruments (Mac)
376.66 KB, image/png
Details
[before] 3 remote peers appear.in SFU
198.37 KB, image/png
Details
[after] 3 remote peers appear.in SFU
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
(appear.in SFU, 6 remote peers) Percentage of before and after, showing allocations gone
352.81 KB, image/png
Details
Bug 1407549 - Give MediaSegment's chunk array a default capacity.
59 bytes, text/x-review-board-request
padenot
: review+
Details
Bug 1407549 - Avoid array operations that can cause alloc/dalloc in MediaSegment.
59 bytes, text/x-review-board-request
padenot
: review+
Details
Bug 1407549 - Make segments allocate chunk storage locally.
59 bytes, text/x-review-board-request
padenot
: review+
Details
Bug 1407549 - Simplify MSGImpl::AudioTrackPresent.
59 bytes, text/x-review-board-request
padenot
: review+
Details
Bug 1407549 - Avoid copying principal handles as much as possible.
59 bytes, text/x-review-board-request
padenot
: review+
Details
Bug 1407549 - Copy AudioSegment by constructor.
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)
Rank: 18
Assignee: nobody → apehrson
Rank: 18 → 12
Status: NEW → ASSIGNED
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.
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.
Attachment #8963090 - Attachment description: bug_1407549-before.png → [before] 3 remote peers appear.in SFU
Attachment #8963094 - Attachment description: bug_1407549-after.png → [after] 3 remote peers appear.in SFU
Attached image [before] 6 remote peers appear.in SFU (obsolete) — 
Similarly but with 6 remote peers running on a macbook pro
Can you attach the patch?
Sorry, they were still WIP, I think what I have now is ready though.
Attachment #8963109 - Attachment is obsolete: true
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 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 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 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 on attachment 8963654 [details]
Bug 1407549 - Simplify MSGImpl::AudioTrackPresent.

https://reviewboard.mozilla.org/r/232550/#review238214
Attachment #8963654 - Flags: review?(padenot) → 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 on attachment 8964873 [details]
Bug 1407549 - Copy AudioSegment by constructor.

https://reviewboard.mozilla.org/r/233540/#review239184
Attachment #8964873 - Flags: review?(padenot) → review+
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
You need to log in before you can comment on or make changes to this bug.