Bug 217685 - Cache JS arguments in AudioWorkletProcessor::process() for performance
Summary: Cache JS arguments in AudioWorkletProcessor::process() for performance
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Audio (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks: 217058
  Show dependency treegraph
 
Reported: 2020-10-13 17:07 PDT by Chris Dumez
Modified: 2020-10-15 16:42 PDT (History)
15 users (show)

See Also:


Attachments
Patch (11.42 KB, patch)
2020-10-13 17:19 PDT, Chris Dumez
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (12.29 KB, patch)
2020-10-13 17:44 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (18.80 KB, patch)
2020-10-13 21:18 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2020-10-13 17:07:10 PDT
Cache JS arguments in AudioWorkletProcessor::process() for performance.
Comment 1 Chris Dumez 2020-10-13 17:19:18 PDT
Created attachment 411282 [details]
Patch
Comment 2 Chris Dumez 2020-10-13 17:44:25 PDT
Created attachment 411286 [details]
Patch
Comment 3 Darin Adler 2020-10-13 18:01:07 PDT
Comment on attachment 411286 [details]
Patch

I don’t understand why all these jsCast are safe. What guarantees the objects have the correct type?
Comment 4 Chris Dumez 2020-10-13 18:04:30 PDT
(In reply to Darin Adler from comment #3)
> Comment on attachment 411286 [details]
> Patch
> 
> I don’t understand why all these jsCast are safe. What guarantees the
> objects have the correct type?

We construct those arrays ourselves and freeze before passing them to JavaScript. Since those are frozen arrays, the JS cannot modify them.
Comment 5 Sam Weinig 2020-10-13 18:11:29 PDT
Comment on attachment 411286 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=411286&action=review

> Source/WebCore/ChangeLog:8
> +        Cache JS arguments in AudioWorkletProcessor::process() for performance.

Got any numbers?
Comment 6 Geoffrey Garen 2020-10-13 20:18:18 PDT
Comment on attachment 411286 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=411286&action=review

> Source/WebCore/Modules/webaudio/AudioWorkletProcessor.h:73
> +    JSC::Strong<JSC::JSArray> m_jsInputs;
> +    JSC::Strong<JSC::JSArray> m_jsOutputs;
> +    JSC::Strong<JSC::JSObject> m_jsParamValues;

Is the AudioWorkletProcessor in scope in JavaScript somewhere in the worklet? If so, I believe that "Array.prototype.customProperty = audioWorkletProcessor" would be a retain cycle.

Since this is just a cache, one alternative is to use JSC::Weak.
Comment 7 Chris Dumez 2020-10-13 20:36:05 PDT
(In reply to Geoffrey Garen from comment #6)
> Comment on attachment 411286 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=411286&action=review
> 
> > Source/WebCore/Modules/webaudio/AudioWorkletProcessor.h:73
> > +    JSC::Strong<JSC::JSArray> m_jsInputs;
> > +    JSC::Strong<JSC::JSArray> m_jsOutputs;
> > +    JSC::Strong<JSC::JSObject> m_jsParamValues;
> 
> Is the AudioWorkletProcessor in scope in JavaScript somewhere in the
> worklet? If so, I believe that "Array.prototype.customProperty =
> audioWorkletProcessor" would be a retain cycle.

Yes, the processor is an object in JS. This is actually the process we call the process() function on in JS. I had thought about this potential leak issue but thought it could not happen because the arrays are frozen. I had not thought about setting a property on the prototype.

> Since this is just a cache, one alternative is to use JSC::Weak.

I did not use Weak because I don’t think it would work. What keeps the JS arrays alive if they are weak? There are constructed in c++ side and only passed in argument to the JS process() function. It is not expected or even common that the JS side will hold on to these arrays in between process() function calls. Do I need some kind of visitor maybe?
Comment 8 Chris Dumez 2020-10-13 20:39:01 PDT
(In reply to Sam Weinig from comment #5)
> Comment on attachment 411286 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=411286&action=review
> 
> > Source/WebCore/ChangeLog:8
> > +        Cache JS arguments in AudioWorkletProcessor::process() for performance.
> 
> Got any numbers?

I checked that the cache is used most of the time since the topology rarely changes. As such, this avoids a lot of JS object allocations (since the process() function is called super frequently to process audio). Besides avoiding bad peaks of memory usage, this also means less GC. 

I don’t have actual CPU time numbers yet. I can probably get some tomorrow. I was personally satisfied with avoiding some many JS object allocations in the common case.
Comment 9 Yusuke Suzuki 2020-10-13 20:49:09 PDT
Comment on attachment 411286 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=411286&action=review

>>> Source/WebCore/Modules/webaudio/AudioWorkletProcessor.h:73
>>> +    JSC::Strong<JSC::JSObject> m_jsParamValues;
>> 
>> Is the AudioWorkletProcessor in scope in JavaScript somewhere in the worklet? If so, I believe that "Array.prototype.customProperty = audioWorkletProcessor" would be a retain cycle.
>> 
>> Since this is just a cache, one alternative is to use JSC::Weak.
> 
> Yes, the processor is an object in JS. This is actually the process we call the process() function on in JS. I had thought about this potential leak issue but thought it could not happen because the arrays are frozen. I had not thought about setting a property on the prototype.

It is possible that this causes leaks even if this is frozen. For example,

let map = new WeakMap;
map.set(frozenArray, wrapperOfAudioWorkletProcessor);

then, cyclic reference is created.
I recommend using JSC::Weak, or JSValueInWrappedObject.
Comment 10 Chris Dumez 2020-10-13 20:51:10 PDT
Comment on attachment 411286 [details]
Patch

Ok. I will look into using JSValueInWrappedObject tomorrow. I see that it has a visit() function, I guess this is what I need to do to keep those arrays alive.
Comment 11 Chris Dumez 2020-10-13 21:18:37 PDT
Created attachment 411297 [details]
Patch
Comment 12 Chris Dumez 2020-10-13 21:19:30 PDT
(In reply to Chris Dumez from comment #10)
> Comment on attachment 411286 [details]
> Patch
> 
> Ok. I will look into using JSValueInWrappedObject tomorrow. I see that it
> has a visit() function, I guess this is what I need to do to keep those
> arrays alive.

JSValueInWrappedObject is great, was super easy to use. I used to know about this class and then forgot about it. The latest patch iteration uses it.
Comment 13 Chris Dumez 2020-10-14 08:33:56 PDT
(In reply to Chris Dumez from comment #8)
> (In reply to Sam Weinig from comment #5)
> > Comment on attachment 411286 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=411286&action=review
> > 
> > > Source/WebCore/ChangeLog:8
> > > +        Cache JS arguments in AudioWorkletProcessor::process() for performance.
> > 
> > Got any numbers?
> 
> I checked that the cache is used most of the time since the topology rarely
> changes. As such, this avoids a lot of JS object allocations (since the
> process() function is called super frequently to process audio). Besides
> avoiding bad peaks of memory usage, this also means less GC. 
> 
> I don’t have actual CPU time numbers yet. I can probably get some tomorrow.
> I was personally satisfied with avoiding some many JS object allocations in
> the common case.

I tweaked imported/w3c/web-platform-tests/webaudio/the-audio-api/the-audioworklet-interface/simple-input-output.https.html to turn it into a perf test. I basically made the buffer very large and checked how long it takes to process the buffer. Note that the test has a fairly simple but common topology.

Without optimization: ~23 seconds
With optimization: ~15 seconds
Comment 14 Darin Adler 2020-10-14 11:27:26 PDT
(In reply to Chris Dumez from comment #12)
> JSValueInWrappedObject is great, was super easy to use. I used to know about
> this class and then forgot about it. The latest patch iteration uses it.

One small editorial comment: When I originally added JSValueInWrappedObject, I also came up with a testing technique for verifying that reference cycles were not keeping objects alive. I suggest we add tests like that for these references.

Let me know if you need a pointer to those tests so you can see the techniques they use.
Comment 15 Chris Dumez 2020-10-15 12:33:04 PDT
Comment on attachment 411297 [details]
Patch

Ping review? I think the performance results are pretty compelling and I have stopped using JSC::Strong.
Comment 16 Geoffrey Garen 2020-10-15 16:38:51 PDT
Comment on attachment 411297 [details]
Patch

r=me
Comment 17 EWS 2020-10-15 16:41:57 PDT
Committed r268560: <https://trac.webkit.org/changeset/268560>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 411297 [details].
Comment 18 Radar WebKit Bug Importer 2020-10-15 16:42:26 PDT
<rdar://problem/70357674>