Deallocate processors outside render thread

[b-ma]

Hey,

Just to share a (rather dirty) test I've made to drop processors outside the render thread. Very probably not something to merge as is but can be a basis for discussion

Several notes:

• it implies adding a method on the AudioProcessor trait
• I chose to send back the processors to the control thread rather than in the garbage collector thread, because it opens the room for some node pooling system I guess
• I just abused the event system to make it work without much modifications, but probably a dedicated channel would be better.

[orottier]

Hey, thanks for getting the conversation started!
What's the main motivation for processor reuse? Do the browser implementations use this as well?
I would be very surprised if it actually brings a performance improvement. Most nodes are extremely lightweight. But I would love to be proven wrong. Perhaps we can think of a benchmark to validate this?

Some other remarks:

• if the main motivation is to prevent deallocations on the render thread, I think we can come up with easier solutions than full AudioProcessor shipments.
• why recycle back to the control thread? We could also stash them in the render thread which may be safer? Although this might put some administrative overhead on the render thread
• I wonder how the actual recycling will work. Will you recycle the control side nodes as well?
• resetting the state of the processor will be quite a lot of code in total I think, and it is probably error prone

In any case, please proceed. Interesting stuff

[orottier]

You could say here,
let node = nodes.remove(index).unwrap();
and then move the added section above to here

[orottier]

Yeah, this is really tricky..
If a user implements this interface incorrectly and its processor will be reused, very subtle bugs will arise...
Or we need a mechanism to only recycle our own nodes

[b-ma]

Hey,

Actually, the main goal is rather to find a solution to not deallocate the processors in the render thread, sorry for the confusing title. I really think it creates issues that get more apparent with the javascript GC.

For example, if we take the many_oscillator_with_env example (which is far from being the worse case we can imagine), we span 5 processors (2 nodes and 3 params) every 50ms, which means 100 processors per second. It may not seems that much, but I checked the GC calls in Node with this example and they appear every 4-5 seconds on my Mac (not very stable and probably different on other platforms, but that gives an order of magnitude). Then, if I understand well, that means we just drop ~500 processors at once when the javascript release ownership of the audio nodes, and not 5 of them every 50ms as in raw Rust. That's very probably not the only problem, but I think this is part of the perf issues I observe on Raspberry Pi.

The idea of sending the renderers back to the control thread was more to not close the door to an eventual future renderer pooling system, but I have no clear idea of how this could work nor of any eventual perf improvement. I just remember having this conversation w/ Paul saying he implemented such thing in Firefox, but maybe it was a bit late so let's not take this for granted :).

if the main motivation is to prevent deallocations on the render thread, I think we can come up with easier solutions than full AudioProcessor shipments.

Cool, let me know your ideas, I can try to prototype something

Side note:
Just an idea but maybe we could also refine Node:can_drop to also drop renderer of sources (i.e. AudioBufferSource, Oscillator) once they are stopped, even if the related control node as not been dropped itself, as they cannot be restarted in any case?

[orottier]

Sorry for the delay in response!

Perhaps a good experiment would be to replace can_drop with false. Effectively creating a resource leak because AudioProcessors are not deallocated. Does this solve stuttering? If not, there may be other issues at play.

Unfortunately I'm totally not familiar with napi-rs so I don't know how rust deallocation works together with the NodeJs GC. But I presume we only need to take care of heap-allocated objects (such as AudioBuffer) and for example not a pure stack-object such as the GainRenderer.
Looking at the AudioBufferSourceRenderer, only the buffer: Option<AudioBuffer> field is heap allocated. To prevent deallocation of it on the render thread, we can use the pattern that @uklotzde introduced for deallocating control messages in a separate thread. In short, the control thread supplies an llq::Node in advance, which the renderer holds on to, but when it drops it uses the link to send the heap allocated object to the deallocation thread.

I could whip up a prototype if you are unsure what I mean

[b-ma]

Hey,

No problem I was in holidays anyway.

Perhaps a good experiment would be to replace can_drop with false. Effectively creating a resource leak because AudioProcessors are not deallocated. Does this solve stuttering? If not, there may be other issues at play.

Indeed, good idea I will have a try

Looking at the AudioBufferSourceRenderer, only the buffer: Option field is heap allocated. To prevent deallocation of it on the render thread, we can use the pattern that @uklotzde introduced for deallocating control messages in a separate thread. In short, the control thread supplies an llq::Node in advance, which the renderer holds on to, but when it drops it uses the link to send the heap allocated object to the deallocation thread.

I could whip up a prototype if you are unsure what I mean

Hum right, I see what you mean. I will try to have a shot, that seems like a good exercice for me :)

Let's close that then