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

()

Core
Web Audio
P1
normal
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: Away for a while, Assigned: padenot)

Tracking

({perf})

Trunk
mozilla30
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [blocking-webaudio-])

Attachments

(3 attachments, 3 obsolete attachments)

(Reporter)

Description

5 years ago
See bug 882543 comment 6.
(Reporter)

Comment 1

5 years ago
Created attachment 762505 [details] [diff] [review]
Patch (v1)

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?
(Reporter)

Comment 5

5 years ago
(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.
(Reporter)

Comment 6

5 years ago
(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.
(Reporter)

Comment 8

5 years ago
(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.
(Reporter)

Comment 9

5 years ago
Created attachment 762678 [details] [diff] [review]
Patch (v2)
Attachment #762505 - Attachment is obsolete: true
Attachment #762505 - Flags: review?(roc)
Attachment #762678 - Flags: review?(roc)
(Reporter)

Comment 10

5 years ago
Created attachment 762799 [details] [diff] [review]
Part 2

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)
(Reporter)

Comment 11

5 years ago
Created attachment 762843 [details] [diff] [review]
Part 2

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?
Attachment #762843 - Flags: review?(roc) → review+
(Reporter)

Comment 13

5 years ago
Comment on attachment 762843 [details] [diff] [review]
Part 2

https://hg.mozilla.org/integration/mozilla-inbound/rev/6e264b512357
Attachment #762843 - Flags: checkin+
(Reporter)

Comment 14

5 years ago
(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.
(Reporter)

Comment 15

5 years ago
Created attachment 763533 [details] [diff] [review]
Patch (v3)
Attachment #762678 - Attachment is obsolete: true
Attachment #762678 - Flags: review?(roc)
Attachment #763533 - Flags: review?(roc)
(Reporter)

Updated

5 years ago
Whiteboard: [leave open]
Attachment #763533 - Flags: review?(roc) → review+
(Reporter)

Comment 16

5 years ago
Comment on attachment 763533 [details] [diff] [review]
Patch (v3)

https://hg.mozilla.org/integration/mozilla-inbound/rev/07708c9fc5ef
Attachment #763533 - Flags: checkin+
(Reporter)

Comment 17

5 years ago
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?
(Reporter)

Comment 18

5 years ago
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
Attachment #763533 - Flags: checkin+ → checkin-
(Reporter)

Comment 19

5 years ago
I won't have time to look into this in the near future, Paul, please feel free to take it.
Assignee: ehsan → nobody
(Assignee)

Updated

5 years ago
Assignee: nobody → paul
https://hg.mozilla.org/mozilla-central/rev/6e264b512357
(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
(Assignee)

Comment 22

4 years ago
(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.
(Reporter)

Updated

4 years ago
Whiteboard: [leave open] → [blocking-webaudio-]
(Assignee)

Updated

4 years ago
Whiteboard: [blocking-webaudio-] → [blocking-webaudio-][webaudio-perf]
(Assignee)

Updated

4 years ago
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
(Reporter)

Comment 25

4 years ago
(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
(Assignee)

Comment 27

4 years ago
roc meant bug 947796, I think.
Depends on: 947796
Flags: needinfo?(paul)
(Assignee)

Comment 28

4 years ago
https://tbpl.mozilla.org/?tree=Try&rev=0d52b8c118b1 !
(Assignee)

Comment 29

4 years ago
Created attachment 8370107 [details] [diff] [review]
Optimize MediaStreamGraphImpl::UpdateStreamOrder in order to optimize away how much time we spend on it in every iteration of the MSG. r=

This is green on try and locally, and saves a bunch of CPU time.
Attachment #8370107 - Flags: review?(roc)
Attachment #8370107 - Flags: review?(roc) → review+
(Assignee)

Comment 30

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/090d90e542df
https://hg.mozilla.org/mozilla-central/rev/090d90e542df
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
(Reporter)

Comment 32

4 years ago
Out of curiosity, what changed to fix this?
(Assignee)

Comment 33

4 years ago
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.