Closed Bug 1407549 Opened 7 years ago Closed 7 years ago

Bad performance of TrackUnionStream::CopyTrackData

Categories

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

defect

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
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)
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+
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+
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.

Attachment

General

Created:
Updated:
Size: