Reduce the allocation in MediaStreamGraph

RESOLVED FIXED in Firefox 41

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: baku, Assigned: baku)

Tracking

({perf})

Trunk
mozilla41
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox41 fixed)

Details

Attachments

(2 attachments, 3 obsolete attachments)

Assignee

Description

4 years ago
Posted patch e.patch (obsolete) — Splinter Review
Often we create temporary arrays to store part of the graph.
We should try to avoid this because that can cause allocation.
Attachment #8604587 - Flags: review?(padenot)
Assignee

Updated

4 years ago
Keywords: perf
Assignee

Comment 1

4 years ago
This first patch contains also some indentation things. If you prefer I can split it in another patch.
Assignee

Comment 2

4 years ago
Posted patch f.patch (obsolete) — Splinter Review
This patch manly introduces StreamBuffer::GetAndResetTracksDirty() and that is used to do or not to do all the MediaStreamGraphImpl::CreateOrDestroyAudio logic.
Attachment #8604607 - Flags: review?(padenot)
Assignee

Comment 3

4 years ago
Posted patch f.patch (obsolete) — Splinter Review
Attachment #8604607 - Attachment is obsolete: true
Attachment #8604607 - Flags: review?(padenot)
Attachment #8604622 - Flags: review?(padenot)
Assignee

Comment 4

4 years ago
Posted patch f.patchSplinter Review
This patch reverts some changes done in previous patches.
The new logic was not fully correct. (but the tests pass...)
Attachment #8604622 - Attachment is obsolete: true
Attachment #8604622 - Flags: review?(padenot)
Attachment #8604707 - Flags: review?(padenot)
Assignee

Comment 5

4 years ago
Posted patch e.patchSplinter Review
Ah.. my fault. The previous comment was about this patch. f.patch is ok.
Attachment #8604587 - Attachment is obsolete: true
Attachment #8604587 - Flags: review?(padenot)
Attachment #8604709 - Flags: review?(padenot)
Comment on attachment 8604707 [details] [diff] [review]
f.patch

Review of attachment 8604707 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/StreamBuffer.cpp
@@ +82,5 @@
> +
> +    if (mTracks[middle]->GetID() > aID) {
> +      right = middle - 1;
> +    } else {
> +      left = middle + 1;

Maybe we can simply use https://dxr.mozilla.org/mozilla-central/source/xpcom/glue/nsTArray.h?from=nsTArray.h#1146 since mTracks is an nsTArray ?
Attachment #8604707 - Flags: review?(padenot) → review+
Attachment #8604709 - Flags: review?(padenot) → review+
Assignee

Comment 7

4 years ago
> Maybe we can simply use
> https://dxr.mozilla.org/mozilla-central/source/xpcom/glue/nsTArray.
> h?from=nsTArray.h#1146 since mTracks is an nsTArray ?

That returns the offset of a known item. I didn't find a binary search implementation for nsTArray that matches this case.
(In reply to Andrea Marchesini (:baku) from comment #7)
> > Maybe we can simply use
> > https://dxr.mozilla.org/mozilla-central/source/xpcom/glue/nsTArray.
> > h?from=nsTArray.h#1146 since mTracks is an nsTArray ?
> 
> That returns the offset of a known item. I didn't find a binary search
> implementation for nsTArray that matches this case.

I'm pretty sure you can use the nsTArray version, but even if you couldn't, you could at least use the underlying implementation it's based on:

http://mxr.mozilla.org/mozilla-central/source/mfbt/BinarySearch.h#45
Depends on: 1216417
You need to log in before you can comment on or make changes to this bug.