1207003 - remove unnecessary tracks from AudioNodeStreams

Status: RESOLVED FIXED

(Core :: Web Audio, defect, P1)

Description

[Karl Tomlinson (:karlt)]

Managing the track appears to be a significant part of overhead of processing.
See FindTrack() and AdvanceOutputSegment() in bug 1205540 comment 1.
Non-external audio nodes should not need a track at all.

Comment 1

[Karl Tomlinson (:karlt)]

The distribution of MSG thread processing time from perf top -t <tid> as inactive nodes sit waiting for GC on http://webaudiodemos.appspot.com/MIDIDrums/index.html looks like:

    15.95%  libxul.so       [.] mozilla::StreamBuffer::FindTrack                
    11.28%  libxul.so       [.] mozilla::AudioNodeStream::AdvanceOutputSegment  
     9.21%  libxul.so       [.] mozilla::MediaSegmentBase<mozilla::AudioSegment,
     8.46%  libxul.so       [.] mozilla::MediaStreamGraphImpl::ProduceDataForStr
     6.54%  libxul.so       [.] mozilla::AudioNodeStream::ProcessInput          
     4.40%  libxul.so       [.] mozilla::AudioNodeStream::ObtainInputBlock      
     3.27%  liblgpllibs.so  [.] rdft_calc_c                                     
     2.37%  libxul.so       [.] mozilla::MediaStreamGraphImpl::UpdateStreamOrder
     1.82%  libxul.so       [.] mozilla::MediaStreamGraphImpl::NotifyHasCurrentD
     1.80%  liblgpllibs.so  [.] ff_fft_permute_sse.loop                         
     1.78%  libxul.so       [.] mozilla::StreamBuffer::ForgetUpTo               
     1.63%  libxul.so       [.] WebCore::DirectConvolver::process               
     1.60%  libxul.so       [.] mozilla::AudioBufferAddWithScale                
     1.55%  libxul.so       [.] WebCore::FFTConvolver::process                  
     1.36%  libxul.so       [.] moz_speex_interpolate_product_single            
     1.36%  libxul.so       [.] mozilla::BufferComplexMultiply                  
     1.26%  liblgpllibs.so  [.] pass_sse.loop                                   
     1.26%  libxul.so       [.] resampler_basic_interpolate_single              
     1.24%  libxul.so       [.] mozilla::FFTBlock::GetInverseWithoutScaling     
     1.14%  libxul.so       [.] mozilla::MediaStreamGraphImpl::UpdateCurrentTime
     0.95%  libxul.so       [.] cubic_coef                                      
     0.91%  libxul.so       [.] mozilla::MediaStream::EnsureTrack               
     0.91%  libxul.so       [.] mozilla::ProcessedMediaStream::AsProcessedStream
     0.85%  liblgpllibs.so  [.] fft16_sse                                       
     0.81%  libxul.so       [.] mozilla::MediaStreamGraphImpl::StreamSet::iterat

The patches that I'll attach here essentially removes 40% of this, including the top three entries, giving:

     9.77%  libxul.so       [.] mozilla::AudioNodeStream::ProcessInput          
     8.39%  libxul.so       [.] mozilla::AudioNodeStream::ObtainInputBlock      
     6.18%  liblgpllibs.so  [.] rdft_calc_c                                     
     5.27%  libxul.so       [.] mozilla::MediaStreamGraphImpl::UpdateStreamOrder
     5.20%  libxul.so       [.] mozilla::MediaStreamGraphImpl::ProduceDataForStr
     3.07%  liblgpllibs.so  [.] ff_fft_permute_sse.loop                         
     2.87%  libxul.so       [.] mozilla::AudioBufferAddWithScale                
     2.79%  libxul.so       [.] WebCore::FFTConvolver::process                  
     2.79%  libxul.so       [.] WebCore::DirectConvolver::process               
     2.66%  libxul.so       [.] mozilla::BufferComplexMultiply                  
     2.53%  libxul.so       [.] mozilla::MediaStreamGraphImpl::NotifyHasCurrentD
     2.51%  libxul.so       [.] moz_speex_interpolate_product_single            
     2.45%  libxul.so       [.] resampler_basic_interpolate_single              
     2.37%  liblgpllibs.so  [.] pass_sse.loop                                   
     2.36%  libxul.so       [.] mozilla::FFTBlock::GetInverseWithoutScaling     
     2.20%  libxul.so       [.] mozilla::MediaStreamGraphImpl::UpdateCurrentTime
     1.88%  libxul.so       [.] mozilla::MediaStreamGraphImpl::CreateOrDestroyAu
     1.72%  libxul.so       [.] mozilla::ProcessedMediaStream::AsProcessedStream
     1.65%  liblgpllibs.so  [.] fft16_sse                                       
     1.63%  libxul.so       [.] cubic_coef                                      
     1.62%  libxul.so       [.] mozilla::MediaStreamGraphImpl::StreamSet::iterat
     1.41%  libxul.so       [.] mozilla::FFTBlock::PerformFFT                   
     1.36%  libxul.so       [.] mozilla::DelayBuffer::ReadChannels              
     1.21%  libm-2.20.so    [.] __powf_finite                                   
     1.18%  liblgpllibs.so  [.] pass_interleave_sse.loop

Comment 2

[Karl Tomlinson (:karlt)]

Attachment: MozReview Request: bug 1207003 remove unnecessary EnsureTrack() call r?padenot

bug 1207003 remove unnecessary EnsureTrack() call r?padenot

EnsureTrack() will be called in AdvanceOutputSegment(), if not blocked.

Comment 3

[Karl Tomlinson (:karlt)]

Attachment: MozReview Request: bug 1207003 coallesce external output chunks when silent r?padenot

bug 1207003 coallesce external output chunks when silent r?padenot

AppendAndConsumeChunk() will always append a new chunk, but AppendNullData()
will use an existing null chunk if possible.

Comment 4

[Karl Tomlinson (:karlt)]

Attachment: MozReview Request: bug 1207003 fetch stream position once instead of three times r?padenot

bug 1207003 fetch stream position once instead of three times r?padenot

Comment 5

[Karl Tomlinson (:karlt)]

Attachment: MozReview Request: bug 1207003 remove unused aStream parameter r?padenot

bug 1207003 remove unused aStream parameter r?padenot

Comment 6

[Karl Tomlinson (:karlt)]

Attachment: MozReview Request: bug 1207003 add GraphTime parameter to ProcessBlock() and remove GetCurrentPosition() r?padenot

bug 1207003 add GraphTime parameter to ProcessBlock() and remove GetCurrentPosition() r?padenot

This is immediately useful for making the track unnecessary, but will also be
required when switching to the destination node stream for tracking time (bug
1205558) because using GetCurrentPosition() on the destination node stream
would give different results depending on the stream processing order (when
called during processing of streams not strictly upstream from the destination
node).

Comment 7

[Karl Tomlinson (:karlt)]

Attachment: MozReview Request: bug 1207003 only create track for external AudioNodeStreams r?padenot

bug 1207003 only create track for external AudioNodeStreams r?padenot

IsFinishedOnGraphThread() was unnecessary due to the !mFinished test.

Comment 8

[Karl Tomlinson (:karlt)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8725f6c9e210

Comment 9

[Paul Adenot (:padenot)]

Comment on attachment 8664029 [details]
MozReview Request: bug 1207003 remove unnecessary EnsureTrack() call r?padenot

https://reviewboard.mozilla.org/r/19903/#review17943

Comment 10

[Paul Adenot (:padenot)]

Comment on attachment 8664030 [details]
MozReview Request: bug 1207003 coallesce external output chunks when silent r?padenot

https://reviewboard.mozilla.org/r/19905/#review17945

Comment 11

[Paul Adenot (:padenot)]

Comment on attachment 8664031 [details]
MozReview Request: bug 1207003 fetch stream position once instead of three times r?padenot

https://reviewboard.mozilla.org/r/19907/#review17947

Comment 12

[Paul Adenot (:padenot)]

Comment on attachment 8664032 [details]
MozReview Request: bug 1207003 remove unused aStream parameter r?padenot

https://reviewboard.mozilla.org/r/19909/#review17951

Comment 13

[Paul Adenot (:padenot)]

Comment on attachment 8664033 [details]
MozReview Request: bug 1207003 add GraphTime parameter to ProcessBlock() and remove GetCurrentPosition() r?padenot

https://reviewboard.mozilla.org/r/19911/#review17953

Comment 14

[Paul Adenot (:padenot)]

Comment on attachment 8664034 [details]
MozReview Request: bug 1207003 only create track for external AudioNodeStreams r?padenot

https://reviewboard.mozilla.org/r/19913/#review17955

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ccec05c4554b
https://hg.mozilla.org/integration/mozilla-inbound/rev/b6ef4e4af23a
https://hg.mozilla.org/integration/mozilla-inbound/rev/ade3b46614b0
https://hg.mozilla.org/integration/mozilla-inbound/rev/2fe45d4590e5
https://hg.mozilla.org/integration/mozilla-inbound/rev/63b1064ba500
https://hg.mozilla.org/integration/mozilla-inbound/rev/15e7ac19b26e

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ccec05c4554b
https://hg.mozilla.org/mozilla-central/rev/b6ef4e4af23a
https://hg.mozilla.org/mozilla-central/rev/ade3b46614b0
https://hg.mozilla.org/mozilla-central/rev/2fe45d4590e5
https://hg.mozilla.org/mozilla-central/rev/63b1064ba500
https://hg.mozilla.org/mozilla-central/rev/15e7ac19b26e

Comment 17

[Karl Tomlinson (:karlt)]

(In reply to Karl Tomlinson (ni?:karlt) from comment #6)
> The results of the two benchmarks that I suspect were affected significantly
> by these patches have changed as follows:
> 
>       Simple Mixing with Gains  Granular Synthesis
> win       700 ->  580               8800 -> 12950
> linux    1050 ->  875              10000 -> 11250
> mac      1280 -> 1100              12000 -> 14000
> 
> I'm guessing that Granular Synthesis was adversely affected because of the
> number of nodes involved.  The extra pass over the mostly inactive nodes
> seems
> to be expensive.  Bug 1189562 should help with that, but I don't think it is
> going to undo all the adverse affects here.
> 
> I think it is best to back this out and come up with something different.
> I have some ideas for keeping track of the number of downstream references.
> That would mean that accounting is only required when adding a downstream
> reference and wanting to reuse a buffer.

(In reply to Pulsebot from comment #7)
> Backout:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/4d7fa39e2d3f

That backout reverted the improvements on Simple Mixing with Gains but didn't change the Granular Synthesis results, so that regression must be unrelated to the changes for the first approach here.

The second approach performs a little better on the Simple Mixing with Gains
benchmark than the first, and avoiding the extra pass is theoretically better
for graphs without so many gain nodes.

I couldn't reproduce the Granular Synthesis regression on i7 linux or pentium
win7 systems.  The variability in results was high (~50%).

Bug 1199559 and bug 1199561 were also in that regression range.  I can only
speculate on how they may be adverse affect specifically in Granular Synthesis
results, but they provided such large improvements elsewhere that I don't
think we want to revert them.

One difference is that ThreadSharedFloatArrayBufferList::Create() uses
js_pod_malloc() whereas JS_NewFloat32Array in AudioBuffer::Create() used
pod_calloc.  Those are used for the source node buffer, but the difference
didn't cause adverse results in other tests, and I wouldn't have expected any
difference in allocation location to make a difference on the large buffer for
the source nodes.

Perhaps other memory is laid out differently due to previous tests not touching so
many pages.

Improvements from fixing bug 1189506, bug 1205540, and bug 1207003 have more
than won back the regression anyway.