Bug 217685 – Cache JS arguments in AudioWorkletProcessor::process() for performance

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Audio

[Chris Dumez]

Cache JS arguments in AudioWorkletProcessor::process() for performance.

[Chris Dumez]

Created attachment 411282 [details]
Patch

[Chris Dumez]

Created attachment 411286 [details]
Patch

[Darin Adler]

Comment on attachment 411286 [details]
Patch

I don’t understand why all these jsCast are safe. What guarantees the objects have the correct type?

[Chris Dumez]

(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.

[Sam Weinig]

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?

[Geoffrey Garen]

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.

[Chris Dumez]

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

[Chris Dumez]

(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.

[Yusuke Suzuki]

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.

[Chris Dumez]

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.

[Chris Dumez]

Created attachment 411297 [details]
Patch

[Chris Dumez]

(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.

[Chris Dumez]

(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

[Darin Adler]

(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.

[Chris Dumez]

Comment on attachment 411297 [details]
Patch

Ping review? I think the performance results are pretty compelling and I have stopped using JSC::Strong.

[Geoffrey Garen]

Comment on attachment 411297 [details]
Patch

r=me

[EWS]

Committed r268560: <https://trac.webkit.org/changeset/268560>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 411297 [details].

[Radar WebKit Bug Importer]

<rdar://problem/70357674>