Bug 217685

Created attachment 411282 [details] Patch

Created attachment 411286 [details] Patch

Comment on attachment 411286 [details] Patch I don’t understand why all these jsCast are safe. What guarantees the objects have the correct type?

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

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

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

Created attachment 411297 [details] Patch

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

(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

(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 on attachment 411297 [details] Patch Ping review? I think the performance results are pretty compelling and I have stopped using JSC::Strong.

Comment on attachment 411297 [details] Patch r=me

Committed r268560: <https://trac.webkit.org/changeset/268560> All reviewed patches have been landed. Closing bug and clearing flags on attachment 411297 [details].

<rdar://problem/70357674>