Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(de-)allocation in render thread #359

Open
1 of 11 tasks
b-ma opened this issue Sep 5, 2023 · 9 comments
Open
1 of 11 tasks

(de-)allocation in render thread #359

b-ma opened this issue Sep 5, 2023 · 9 comments

Comments

@b-ma
Copy link
Collaborator

b-ma commented Sep 5, 2023

Follow up on #353, plus few ideas:

  • Implement Drop to drop AudioBuffer and Vec cleanly in renderers (cf. Otto's comment)
    • AudioBufferSourceRenderer::buffer
    • ConvolverRenderer::buffer
    • WaveshaperRenderer::curve
    • BiquadFilter::{x1, x2, y1, y2} -> consider using ArrayVec instead?
    • DelayRenderer::ring_buffer
    • DynamicsCompressorRenderer::ring_buffer
    • IirFilterRenderer::{norm_coefs, states} -> consider using ArrayVec instead, we can probably clamp eveything using MAX_CHANNELS and the max number of coefs (i.e. 20) ?
  • Use ArrayVec instead of Vec in AudioParam (this is already a dependency so that's cool, and except for resize this is mostly a drop in) Perf - use ArrayVec instead of Vec for internal AudioParam buffer #363
  • Implement optimizations proposed there, especially the second one as most of the time params are not modulating (or are not modulated by) an incoming signal (ok, this one is a bit out of context... :)
  • Install https://github.com/Windfisch/rust-assert-no-alloc to make sure our render thread is clean
@b-ma
Copy link
Collaborator Author

b-ma commented Oct 1, 2023

Hey,

Following the discusion here: #353 (comment) , I had a look on how we could reuse the garbage collector thread to drop AudioBuffers and co. outside render thread and here are my first impressions:

  • If we want to be able to give some sender toward the garbage collector to the AudioNodes, it seem to mean that somehow the audio context should be able to deliver it, so that we grab a clone of the sender when we create the renderers.
  • But with the current implement of the garbage collector, it does not seems possible to implement this pattern because llq is single producer single consumer channel, while we would need a multiple consumer single consumer channel.

What do you think? Should I try to proceed this way and replace the llq channel with a crossbeam-channel?

(aside, maybe we could even use raw std::mpsc instead of crossbeam, cf. rust-lang/rust#93563)

@orottier
Copy link
Owner

orottier commented Oct 2, 2023

Thanks for sharing your thoughts.

The reason we are using llq is because it offers a channel of unbounded size, without (de)allocating in the render thread. This is not possible with crossbeam. The catch is that with llq we need to ship a pre-allocated node beforehand.

We could try to expose a method in the RenderScope to get a hold of the sender. I will need to time to sketch this out though. It's not trivial.

And maybe the llq message type should be enum, instead of Box<dyn Any>.

enum Garbage {
     Message(Box<dyn Any>),
     AudioBuffer(AudioBuffer),
     // etc
}

@b-ma
Copy link
Collaborator Author

b-ma commented Oct 3, 2023

Ok, I see, probably it's best if I let you define the general architecture then

@b-ma
Copy link
Collaborator Author

b-ma commented Nov 15, 2023

I have an idea about deallocation which I have the impression could solve several problems with rather low overhead.
I didn't write any code yet as maybe I missed something, so I prefer having your opinion before digging into it.

If we take for example the AudioBufferSourceNode and AudioBuffer (pseudo code):

pub fn set_buffer(&mut self, audio_buffer: AudioBuffer) {
    if self.buffer.is_some() {
        panic!("InvalidStateError - cannot assign buffer twice");
    }

    let audio_buffer = Arc::new(audio_buffer);

    self.buffer = Some(audio_buffer);
    // send a clone to the garbage collector and another one to audio thread
    self.registration.garbage_collector.send(Arc::clone(&audio_buffer));
    self.registration.post_message(Arc::clone(&audio_buffer));
}

Then in the garbage collector we would just maintain a list of Arc<AudioBuffer> and check at every loop if Arc::strong_count(&audio_buffer) == 1 to drop them safely, because we would know both the node and renderer have been dropped.

That would mostly imply to modify quite a bit how the garbage collector is created, i.e. in main thread rather than in audio thread. But then I have the impression it could be quite straightforward and could allow to handle several of our issues in a unified way.

What do you think?

@cybersoulK

This comment was marked as off-topic.

@orottier

This comment was marked as off-topic.

@orottier

This comment was marked as off-topic.

@cybersoulK

This comment was marked as off-topic.

@orottier
Copy link
Owner

orottier commented Nov 20, 2023

I have an idea about deallocation which I have the impression could solve several problems with rather low overhead.

Thanks for the new suggestion. I had not considered such a thing (similar to the java GC). Could be interesting!

A few things to note though:

  • This only solves our GC issue for immutable objects on the heap (such as an AudioBuffer). Mutable objects (such as the audio processor, or the ring buffer of the delay node) still need another solution.
  • There may be performance overhead because of the additional level of indirection (stack is typically faster)
  • How do we limit the amount of CPU the garbage collector thread uses? If we scan continuously it may take too much effort. We could scan every N seconds, or use a CondVar to actively notify when the render thread drops.

Mostly due to item 1 I'm not directly convinced this will make our life easier, but feel free to experiment! In the meantime I will try to put a bit more effort in #368

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants