Optimize MediaStreamGraphImpl::UpdateStreamOrder in order to optimize away how much time we spend on it in every iteration of the MSG

RESOLVED FIXED in mozilla30

Status

()

defect
P1
normal
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Ehsan, Assigned: padenot)

Tracking

({perf})

Trunk
mozilla30
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [blocking-webaudio-])

Attachments

(3 attachments, 3 obsolete attachments)

See bug 882543 comment 6.
Posted patch Patch (v1) (obsolete) — Splinter Review
This patch eliminates the cost of this function from the profile, but it bumps up the run time of the benchmark from about 14s on my machine to about 50s!  Here's what the profile with this patch applied looks like: <http://grab.by/nwE0>.  It seems like these locking calls are coming from the lock which protects PrepareUpdatesToMainThreadState(), and for some reason that's super expensive when done quickly in a succession.

roc, do you have any idea how we can eliminate that locking there?
Attachment #762505 - Flags: review?(roc)
Comment on attachment 762505 [details] [diff] [review]
Patch (v1)

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

::: content/media/MediaStreamGraph.cpp
@@ +441,5 @@
>    if (aStream->mIsConsumed) {
>      return;
>    }
>    aStream->mIsConsumed = true;
> +  aStream->mGraph->SetStreamOrderDirty();

Why do you think this is necessary here? I don't think it is.

@@ +2129,5 @@
>    , mDetectedNotRunning(false)
>    , mPostedRunInStableState(false)
>    , mRealtime(aRealtime)
>    , mNonRealtimeProcessing(false)
> +  , mStreamOrderDirty(true)

I think this can start false.
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #1)
> This patch eliminates the cost of this function from the profile, but it
> bumps up the run time of the benchmark from about 14s on my machine to about
> 50s!

That makes no sense. Taking and releasing an uncontended lock should be very cheap. Sorry, I don't have any ideas.
Are you sure the test processes the same number of samples every time?
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #2)
> Comment on attachment 762505 [details] [diff] [review]
> Patch (v1)
> 
> Review of attachment 762505 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/media/MediaStreamGraph.cpp
> @@ +441,5 @@
> >    if (aStream->mIsConsumed) {
> >      return;
> >    }
> >    aStream->mIsConsumed = true;
> > +  aStream->mGraph->SetStreamOrderDirty();
> 
> Why do you think this is necessary here? I don't think it is.

It's necessary because UpdateStreamOrder sets mIsConsumed to false, and if it's not run after MarkConsumed, the flag might never get flipped.  Removing this line makes test_convolverNode_mono_mono.html fail.

> @@ +2129,5 @@
> >    , mDetectedNotRunning(false)
> >    , mPostedRunInStableState(false)
> >    , mRealtime(aRealtime)
> >    , mNonRealtimeProcessing(false)
> > +  , mStreamOrderDirty(true)
> 
> I think this can start false.

With that, test_currentTime.html fails.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #4)
> Are you sure the test processes the same number of samples every time?

Yes, it processes ~480000 samples per test with and without this patch.
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #5)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #2)
> > Comment on attachment 762505 [details] [diff] [review]
> > Patch (v1)
> > 
> > Review of attachment 762505 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: content/media/MediaStreamGraph.cpp
> > @@ +441,5 @@
> > >    if (aStream->mIsConsumed) {
> > >      return;
> > >    }
> > >    aStream->mIsConsumed = true;
> > > +  aStream->mGraph->SetStreamOrderDirty();
> > 
> > Why do you think this is necessary here? I don't think it is.
> 
> It's necessary because UpdateStreamOrder sets mIsConsumed to false, and if
> it's not run after MarkConsumed, the flag might never get flipped.  Removing
> this line makes test_convolverNode_mono_mono.html fail.

I don't understand this. Doesn't this mean that as long as at least one stream is consumed, we'll run UpdateStreamOrder in every iteration of the MSG?

> > @@ +2129,5 @@
> > >    , mDetectedNotRunning(false)
> > >    , mPostedRunInStableState(false)
> > >    , mRealtime(aRealtime)
> > >    , mNonRealtimeProcessing(false)
> > > +  , mStreamOrderDirty(true)
> > 
> > I think this can start false.
> 
> With that, test_currentTime.html fails.

I don't understand this either.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #7)
> (In reply to :Ehsan Akhgari (needinfo? me!) from comment #5)
> > (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #2)
> > > Comment on attachment 762505 [details] [diff] [review]
> > > Patch (v1)
> > > 
> > > Review of attachment 762505 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > ::: content/media/MediaStreamGraph.cpp
> > > @@ +441,5 @@
> > > >    if (aStream->mIsConsumed) {
> > > >      return;
> > > >    }
> > > >    aStream->mIsConsumed = true;
> > > > +  aStream->mGraph->SetStreamOrderDirty();
> > > 
> > > Why do you think this is necessary here? I don't think it is.
> > 
> > It's necessary because UpdateStreamOrder sets mIsConsumed to false, and if
> > it's not run after MarkConsumed, the flag might never get flipped.  Removing
> > this line makes test_convolverNode_mono_mono.html fail.
> 
> I don't understand this. Doesn't this mean that as long as at least one
> stream is consumed, we'll run UpdateStreamOrder in every iteration of the
> MSG?

Yes, and that's what we want, otherwise once a stream is consumed, if no new input is added, UpdateConsumptionState does the wrong thing, and MarkConsumed will become a no-op, since the mIsConsumed flag will remain true.  UpdateStreamOrder should really be called UpdateStreamOrderAndMarkNotConsumed.

> > > @@ +2129,5 @@
> > > >    , mDetectedNotRunning(false)
> > > >    , mPostedRunInStableState(false)
> > > >    , mRealtime(aRealtime)
> > > >    , mNonRealtimeProcessing(false)
> > > > +  , mStreamOrderDirty(true)
> > > 
> > > I think this can start false.
> > 
> > With that, test_currentTime.html fails.
> 
> I don't understand this either.

Hah.  Looking more closely, I realized that UpdateStreamOrder sets mInBlockingSet, but on the surface that should not need to happen on every iteration.  However, I realized that MediaStream's constructor doesn't initialize some of its members, including mInBlockingSet, so it's just read as uninitialized memory, which is most likely not zero.  Fixing that, we can initialize mStreamOrderDirty to false.
Posted patch Patch (v2) (obsolete) — Splinter Review
Attachment #762505 - Attachment is obsolete: true
Attachment #762505 - Flags: review?(roc)
Attachment #762678 - Flags: review?(roc)
Posted patch Part 2 (obsolete) — Splinter Review
OK, I figured out the problem.  Turns out that there was indeed a lock contention.  So what was happening was that we were bombarding the main thread with stable state runnables, which would hold the mMonitor lock just to see that they have no work to do, resulting in this pathological behavior.  This patch fixes it.
Attachment #762799 - Flags: review?(roc)
Posted patch Part 2Splinter Review
The previous patch was missing a hunk from the header.
Attachment #762799 - Attachment is obsolete: true
Attachment #762799 - Flags: review?(roc)
Attachment #762843 - Flags: review?(roc)
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #8)
> Yes, and that's what we want, otherwise once a stream is consumed, if no new
> input is added, UpdateConsumptionState does the wrong thing, and
> MarkConsumed will become a no-op, since the mIsConsumed flag will remain
> true.  UpdateStreamOrder should really be called
> UpdateStreamOrderAndMarkNotConsumed.

But MarkConsumed is only called from UpdateStreamOrder. So UpdateStreamOrder could be called UpdateStreamOrderAndSetConsumedState. If we skip calling it, all the mIsConsumed flags will be untouched and should still be correct, so things should work. Am I missing something?
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #12)
> (In reply to :Ehsan Akhgari (needinfo? me!) from comment #8)
> > Yes, and that's what we want, otherwise once a stream is consumed, if no new
> > input is added, UpdateConsumptionState does the wrong thing, and
> > MarkConsumed will become a no-op, since the mIsConsumed flag will remain
> > true.  UpdateStreamOrder should really be called
> > UpdateStreamOrderAndMarkNotConsumed.
> 
> But MarkConsumed is only called from UpdateStreamOrder. So UpdateStreamOrder
> could be called UpdateStreamOrderAndSetConsumedState. If we skip calling it,
> all the mIsConsumed flags will be untouched and should still be correct, so
> things should work. Am I missing something?

No, you're right.
Posted patch Patch (v3)Splinter Review
Attachment #762678 - Attachment is obsolete: true
Attachment #762678 - Flags: review?(roc)
Attachment #763533 - Flags: review?(roc)
Whiteboard: [leave open]
So even with these patches, we're still sending too many updates to the main thread.  We should figure a way to not do that.  roc, do you have any ideas?
I won't have time to look into this in the near future, Paul, please feel free to take it.
Assignee: ehsan → nobody
Assignee: nobody → paul
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #17)
> So even with these patches, we're still sending too many updates to the main
> thread.  We should figure a way to not do that.  roc, do you have any ideas?

Just https://bugzilla.mozilla.org/show_bug.cgi?id=882543#c7
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #18)
> Comment on attachment 763533 [details] [diff] [review]
> Patch (v3)
> 
> Backed out because of test failures:
> 
> https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=07708c9fc5ef
> https://hg.mozilla.org/integration/mozilla-inbound/rev/f0db2c84b4d1

This patch gives substantial performances improvements, I'll look into the failures.
Whiteboard: [leave open] → [blocking-webaudio-]
Whiteboard: [blocking-webaudio-] → [blocking-webaudio-][webaudio-perf]
Keywords: perf
Whiteboard: [blocking-webaudio-][webaudio-perf] → [blocking-webaudio-]
It seems like this could be a real perf win when we're done.  So I'm making this a P1 bug.
Flags: needinfo?(paul)
Priority: -- → P1
Currently all performance measurements of OfflineAudioContext will be dominated by bug 945634. We need to reprofile after that's fixed to figure out what actually matters.
Depends on: 947746
(In reply to comment #24)
> Currently all performance measurements of OfflineAudioContext will be dominated
> by bug 945634. We need to reprofile after that's fixed to figure out what
> actually matters.

Wrong bug numbers? (both this and 947746)
I think you added a dep for the wrong bug.
No longer depends on: 947746
roc meant bug 947796, I think.
Depends on: 947796
Flags: needinfo?(paul)
https://hg.mozilla.org/mozilla-central/rev/090d90e542df
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Out of curiosity, what changed to fix this?
I think I added one or two SetStreamOrderDirty at a couple other locations, compared to your previous patch.
You need to log in before you can comment on or make changes to this bug.