From 80b1bf12f26dff8ab610fcad7a652afc22ab9044 Mon Sep 17 00:00:00 2001 From: Robbert van der Helm Date: Thu, 3 Mar 2022 21:18:57 +0100 Subject: [PATCH] Use AtomicRefCell for all uncontested locks Since it would be a bug if those locks were somehow contested. --- src/wrapper/clap/context.rs | 4 ++-- src/wrapper/clap/wrapper.rs | 35 +++++++++++++++++++---------------- src/wrapper/vst3/context.rs | 14 +++++++------- src/wrapper/vst3/inner.rs | 23 ++++++++++++----------- src/wrapper/vst3/wrapper.rs | 8 ++++---- 5 files changed, 44 insertions(+), 40 deletions(-) diff --git a/src/wrapper/clap/context.rs b/src/wrapper/clap/context.rs index 610e871b..44044d5f 100644 --- a/src/wrapper/clap/context.rs +++ b/src/wrapper/clap/context.rs @@ -1,4 +1,4 @@ -use parking_lot::RwLockWriteGuard; +use atomic_refcell::AtomicRefMut; use std::collections::VecDeque; use std::sync::atomic::Ordering; use std::sync::Arc; @@ -22,7 +22,7 @@ pub(crate) struct WrapperGuiContext { /// unnecessary atomic operations to lock the uncontested RwLocks. pub(crate) struct WrapperProcessContext<'a, P: ClapPlugin> { pub(super) wrapper: &'a Wrapper

, - pub(super) input_events_guard: RwLockWriteGuard<'a, VecDeque>, + pub(super) input_events_guard: AtomicRefMut<'a, VecDeque>, } impl GuiContext for WrapperGuiContext

{ diff --git a/src/wrapper/clap/wrapper.rs b/src/wrapper/clap/wrapper.rs index 427d0288..da650efb 100644 --- a/src/wrapper/clap/wrapper.rs +++ b/src/wrapper/clap/wrapper.rs @@ -2,7 +2,7 @@ // explicitly pattern match on that unit #![allow(clippy::unused_unit)] -use atomic_refcell::AtomicRefCell; +use atomic_refcell::{AtomicRefCell, AtomicRefMut}; use clap_sys::events::{ clap_event_header, clap_event_note, clap_event_param_mod, clap_event_param_value, clap_input_events, clap_output_events, CLAP_CORE_EVENT_SPACE_ID, CLAP_EVENT_MIDI, @@ -35,7 +35,7 @@ use clap_sys::stream::{clap_istream, clap_ostream}; use crossbeam::atomic::AtomicCell; use crossbeam::queue::ArrayQueue; use lazy_static::lazy_static; -use parking_lot::{RwLock, RwLockWriteGuard}; +use parking_lot::RwLock; use raw_window_handle::RawWindowHandle; use std::any::Any; use std::cmp; @@ -85,7 +85,7 @@ pub struct Wrapper { pub clap_plugin: clap_plugin, /// A reference to this object, upgraded to an `Arc` for the GUI context. - this: RwLock>, + this: AtomicRefCell>, /// The wrapped plugin instance. plugin: RwLock

, @@ -111,7 +111,7 @@ pub struct Wrapper { /// /// TODO: Maybe load these lazily at some point instead of needing to spool them all to this /// queue first - input_events: RwLock>, + input_events: AtomicRefCell>, /// The current latency in samples, as set by the plugin through the [ProcessContext]. uses the /// latency extnesion pub current_latency: AtomicU32, @@ -119,7 +119,7 @@ pub struct Wrapper { /// apointer to pointers, so this needs to be preallocated in the setup call and kept around /// between process calls. This buffer owns the vector, because otherwise it would need to store /// a mutable reference to the data contained in this mutex. - pub output_buffer: RwLock>, + pub output_buffer: AtomicRefCell>, /// Needs to be boxed because the plugin object is supposed to contain a static reference to /// this. @@ -305,7 +305,7 @@ impl Wrapper

{ on_main_thread: Self::on_main_thread, }, - this: RwLock::new(Weak::new()), + this: AtomicRefCell::new(Weak::new()), plugin, editor, @@ -318,9 +318,9 @@ impl Wrapper

{ }), current_buffer_config: AtomicCell::new(None), bypass_state: AtomicBool::new(false), - input_events: RwLock::new(VecDeque::with_capacity(512)), + input_events: AtomicRefCell::new(VecDeque::with_capacity(512)), current_latency: AtomicU32::new(0), - output_buffer: RwLock::new(Buffer::default()), + output_buffer: AtomicRefCell::new(Buffer::default()), plugin_descriptor, @@ -466,7 +466,7 @@ impl Wrapper

{ // Finally, the wrapper needs to contain a reference to itself so we can create GuiContexts // when opening plugin editors let wrapper = Arc::new(wrapper); - *wrapper.this.write() = Arc::downgrade(&wrapper); + *wrapper.this.borrow_mut() = Arc::downgrade(&wrapper); wrapper } @@ -478,7 +478,7 @@ impl Wrapper

{ fn make_process_context(&self) -> WrapperProcessContext<'_, P> { WrapperProcessContext { wrapper: self, - input_events_guard: self.input_events.write(), + input_events_guard: self.input_events.borrow_mut(), } } @@ -559,7 +559,7 @@ impl Wrapper

{ /// Handle all incoming events from an event queue. This will clearn `self.input_events` first. pub unsafe fn handle_in_events(&self, in_: &clap_input_events) { - let mut input_events = self.input_events.write(); + let mut input_events = self.input_events.borrow_mut(); input_events.clear(); let num_events = ((*in_).size)(&*in_); @@ -611,7 +611,7 @@ impl Wrapper

{ pub unsafe fn handle_event( &self, event: *const clap_event_header, - input_events: &mut RwLockWriteGuard>, + input_events: &mut AtomicRefMut>, ) { let raw_event = &*event; match (raw_event.space_id, raw_event.type_) { @@ -723,9 +723,12 @@ impl Wrapper

{ ) { // Preallocate enough room in the output slices vector so we can convert a `*mut *mut // f32` to a `&mut [&mut f32]` in the process call - wrapper.output_buffer.write().with_raw_vec(|output_slices| { - output_slices.resize_with(bus_config.num_output_channels as usize, || &mut []) - }); + wrapper + .output_buffer + .borrow_mut() + .with_raw_vec(|output_slices| { + output_slices.resize_with(bus_config.num_output_channels as usize, || &mut []) + }); // Also store this for later, so we can reinitialize the plugin after restoring state wrapper.current_buffer_config.store(Some(buffer_config)); @@ -808,7 +811,7 @@ impl Wrapper

{ // TODO: The audio buffers have a latency field, should we use those? // TODO: Like with VST3, should we expose some way to access or set the silence/constant // flags? - let mut output_buffer = wrapper.output_buffer.write(); + let mut output_buffer = wrapper.output_buffer.borrow_mut(); output_buffer.with_raw_vec(|output_slices| { nih_debug_assert_eq!(num_output_channels, output_slices.len()); for (output_channel_idx, output_channel_slice) in diff --git a/src/wrapper/vst3/context.rs b/src/wrapper/vst3/context.rs index 1d6e97db..30650c6a 100644 --- a/src/wrapper/vst3/context.rs +++ b/src/wrapper/vst3/context.rs @@ -1,4 +1,4 @@ -use parking_lot::RwLockWriteGuard; +use atomic_refcell::AtomicRefMut; use std::collections::VecDeque; use std::sync::atomic::Ordering; use std::sync::Arc; @@ -19,17 +19,17 @@ pub(crate) struct WrapperGuiContext { /// A [ProcessContext] implementation for the wrapper. This is a separate object so it can hold on /// to lock guards for event queues. Otherwise reading these events would require constant -/// unnecessary atomic operations to lock the uncontested RwLocks. +/// unnecessary atomic operations to lock the uncontested locks. pub(crate) struct WrapperProcessContext<'a, P: Vst3Plugin> { pub(super) inner: &'a WrapperInner

, - pub(super) input_events_guard: RwLockWriteGuard<'a, VecDeque>, + pub(super) input_events_guard: AtomicRefMut<'a, VecDeque>, } impl GuiContext for WrapperGuiContext

{ // All of these functions are supposed to be called from the main thread, so we'll put some // trust in the caller and assume that this is indeed the case unsafe fn raw_begin_set_parameter(&self, param: ParamPtr) { - match &*self.inner.component_handler.read() { + match &*self.inner.component_handler.borrow() { Some(handler) => match self.inner.param_ptr_to_hash.get(¶m) { Some(hash) => { handler.begin_edit(*hash); @@ -41,7 +41,7 @@ impl GuiContext for WrapperGuiContext

{ } unsafe fn raw_set_parameter_normalized(&self, param: ParamPtr, normalized: f32) { - match &*self.inner.component_handler.read() { + match &*self.inner.component_handler.borrow() { Some(handler) => match self.inner.param_ptr_to_hash.get(¶m) { Some(hash) => { // Only update the parameters manually if the host is not processing audio. If @@ -68,7 +68,7 @@ impl GuiContext for WrapperGuiContext

{ } unsafe fn raw_end_set_parameter(&self, param: ParamPtr) { - match &*self.inner.component_handler.read() { + match &*self.inner.component_handler.borrow() { Some(handler) => match self.inner.param_ptr_to_hash.get(¶m) { Some(hash) => { handler.end_edit(*hash); @@ -95,7 +95,7 @@ impl ProcessContext for WrapperProcessContext<'_, P> { // Only trigger a restart if it's actually needed let old_latency = self.inner.current_latency.swap(samples, Ordering::SeqCst); if old_latency != samples { - let task_posted = unsafe { self.inner.event_loop.read().assume_init_ref() } + let task_posted = unsafe { self.inner.event_loop.borrow().assume_init_ref() } .do_maybe_async(Task::TriggerRestart( vst3_sys::vst::RestartFlags::kLatencyChanged as i32, )); diff --git a/src/wrapper/vst3/inner.rs b/src/wrapper/vst3/inner.rs index 15911827..d05878e9 100644 --- a/src/wrapper/vst3/inner.rs +++ b/src/wrapper/vst3/inner.rs @@ -1,3 +1,4 @@ +use atomic_refcell::AtomicRefCell; use crossbeam::atomic::AtomicCell; use parking_lot::RwLock; use std::collections::{HashMap, VecDeque}; @@ -29,7 +30,7 @@ pub(crate) struct WrapperInner { /// The host's `IComponentHandler` instance, if passed through /// `IEditController::set_component_handler`. - pub component_handler: RwLock>>, + pub component_handler: AtomicRefCell>>, /// Our own [IPlugView] instance. This is set while the editor is actually visible (which is /// different form the lifetimei of [super::WrapperView] itself). @@ -42,7 +43,7 @@ pub(crate) struct WrapperInner { /// mutably borrow the event loop, so reads will never be contested. /// /// TODO: Is there a better type for Send+Sync late initializaiton? - pub event_loop: RwLock>>, + pub event_loop: AtomicRefCell>>, /// Whether the plugin is currently processing audio. In other words, the last state /// `IAudioProcessor::setActive()` has been called with. @@ -63,12 +64,12 @@ pub(crate) struct WrapperInner { /// apointer to pointers, so this needs to be preallocated in the setup call and kept around /// between process calls. This buffer owns the vector, because otherwise it would need to store /// a mutable reference to the data contained in this mutex. - pub output_buffer: RwLock>, + pub output_buffer: AtomicRefCell>, /// The incoming events for the plugin, if `P::ACCEPTS_MIDI` is set. /// /// TODO: Maybe load these lazily at some point instead of needing to spool them all to this /// queue first - pub input_events: RwLock>, + pub input_events: AtomicRefCell>, /// The keys from `param_map` in a stable order. pub param_hashes: Vec, @@ -109,11 +110,11 @@ impl WrapperInner

{ plugin, editor, - component_handler: RwLock::new(None), + component_handler: AtomicRefCell::new(None), plug_view: RwLock::new(None), - event_loop: RwLock::new(MaybeUninit::uninit()), + event_loop: AtomicRefCell::new(MaybeUninit::uninit()), is_processing: AtomicBool::new(false), // Some hosts, like the current version of Bitwig and Ardour at the time of writing, @@ -128,8 +129,8 @@ impl WrapperInner

{ bypass_state: AtomicBool::new(false), last_process_status: AtomicCell::new(ProcessStatus::Normal), current_latency: AtomicU32::new(0), - output_buffer: RwLock::new(Buffer::default()), - input_events: RwLock::new(VecDeque::with_capacity(512)), + output_buffer: AtomicRefCell::new(Buffer::default()), + input_events: AtomicRefCell::new(VecDeque::with_capacity(512)), param_hashes: Vec::new(), param_by_hash: HashMap::new(), @@ -181,7 +182,7 @@ impl WrapperInner

{ // serving multiple plugin instances, Arc can't be used because its reference count // is separate from the internal COM-style reference count. let wrapper: Arc> = wrapper.into(); - *wrapper.event_loop.write() = + *wrapper.event_loop.borrow_mut() = MaybeUninit::new(OsEventLoop::new_and_spawn(Arc::downgrade(&wrapper))); wrapper @@ -194,7 +195,7 @@ impl WrapperInner

{ pub fn make_process_context(&self) -> WrapperProcessContext<'_, P> { WrapperProcessContext { inner: self, - input_events_guard: self.input_events.write(), + input_events_guard: self.input_events.borrow_mut(), } } @@ -237,7 +238,7 @@ impl MainThreadExecutor for WrapperInner

{ // function for checking if a to be scheduled task can be handled right ther and // then). match task { - Task::TriggerRestart(flags) => match &*self.component_handler.read() { + Task::TriggerRestart(flags) => match &*self.component_handler.borrow() { Some(handler) => { handler.restart_component(flags); } diff --git a/src/wrapper/vst3/wrapper.rs b/src/wrapper/vst3/wrapper.rs index 98d8fc81..5162d2f4 100644 --- a/src/wrapper/vst3/wrapper.rs +++ b/src/wrapper/vst3/wrapper.rs @@ -465,7 +465,7 @@ impl IEditController for Wrapper

{ &self, handler: SharedVstPtr, ) -> tresult { - *self.inner.component_handler.write() = handler.upgrade().map(VstPtr::from); + *self.inner.component_handler.borrow_mut() = handler.upgrade().map(VstPtr::from); kResultOk } @@ -599,7 +599,7 @@ impl IAudioProcessor for Wrapper

{ // f32` to a `&mut [&mut f32]` in the process call self.inner .output_buffer - .write() + .borrow_mut() .with_raw_vec(|output_slices| { output_slices.resize_with(bus_config.num_output_channels as usize, || &mut []) }); @@ -667,7 +667,7 @@ impl IAudioProcessor for Wrapper

{ // And also incoming note events if the plugin accepts MDII if P::ACCEPTS_MIDI { - let mut input_events = self.inner.input_events.write(); + let mut input_events = self.inner.input_events.borrow_mut(); if let Some(events) = data.input_events.upgrade() { let num_events = events.get_event_count(); @@ -731,7 +731,7 @@ impl IAudioProcessor for Wrapper

{ // This vector has been preallocated to contain enough slices as there are output // channels - let mut output_buffer = self.inner.output_buffer.write(); + let mut output_buffer = self.inner.output_buffer.borrow_mut(); output_buffer.with_raw_vec(|output_slices| { nih_debug_assert_eq!(num_output_channels, output_slices.len()); for (output_channel_idx, output_channel_slice) in