[core] Use internal locking in CommandAllocator.

Move the `Mutex` in `Device::command_allocator` inside the
`CommandAllocator` type itself, allowing it to be passed by shared
reference instead of mutable reference.

Passing `CommandAllocator` to functions like
`PendingWrites::post_submit` by mutable reference requires the caller
to acquire and hold the mutex for the entire time the callee runs, but
`CommandAllocator` is just a recycling pool, with very simple
invariants; there's no reason to hold the lock for a long time.
This commit is contained in:
Jim Blandy 2024-04-15 12:52:19 -07:00 committed by Erich Gubler
parent b9781ee6e2
commit bab6f53e86
5 changed files with 27 additions and 34 deletions

View File

@ -2,6 +2,8 @@ use crate::hal_api::HalApi;
use crate::resource_log; use crate::resource_log;
use hal::Device as _; use hal::Device as _;
use parking_lot::Mutex;
/// A pool of free [`wgpu_hal::CommandEncoder`]s, owned by a `Device`. /// A pool of free [`wgpu_hal::CommandEncoder`]s, owned by a `Device`.
/// ///
/// Each encoder in this list is in the "closed" state. /// Each encoder in this list is in the "closed" state.
@ -12,13 +14,13 @@ use hal::Device as _;
/// [ce]: wgpu_hal::CommandEncoder /// [ce]: wgpu_hal::CommandEncoder
/// [cb]: wgpu_hal::Api::CommandBuffer /// [cb]: wgpu_hal::Api::CommandBuffer
pub(crate) struct CommandAllocator<A: HalApi> { pub(crate) struct CommandAllocator<A: HalApi> {
free_encoders: Vec<A::CommandEncoder>, free_encoders: Mutex<Vec<A::CommandEncoder>>,
} }
impl<A: HalApi> CommandAllocator<A> { impl<A: HalApi> CommandAllocator<A> {
pub(crate) fn new() -> Self { pub(crate) fn new() -> Self {
Self { Self {
free_encoders: Vec::new(), free_encoders: Mutex::new(Vec::new()),
} }
} }
@ -27,11 +29,12 @@ impl<A: HalApi> CommandAllocator<A> {
/// If we have free encoders in the pool, take one of those. Otherwise, /// If we have free encoders in the pool, take one of those. Otherwise,
/// create a new one on `device`. /// create a new one on `device`.
pub(crate) fn acquire_encoder( pub(crate) fn acquire_encoder(
&mut self, &self,
device: &A::Device, device: &A::Device,
queue: &A::Queue, queue: &A::Queue,
) -> Result<A::CommandEncoder, hal::DeviceError> { ) -> Result<A::CommandEncoder, hal::DeviceError> {
match self.free_encoders.pop() { let mut free_encoders = self.free_encoders.lock();
match free_encoders.pop() {
Some(encoder) => Ok(encoder), Some(encoder) => Ok(encoder),
None => unsafe { None => unsafe {
let hal_desc = hal::CommandEncoderDescriptor { label: None, queue }; let hal_desc = hal::CommandEncoderDescriptor { label: None, queue };
@ -41,19 +44,18 @@ impl<A: HalApi> CommandAllocator<A> {
} }
/// Add `encoder` back to the free pool. /// Add `encoder` back to the free pool.
pub(crate) fn release_encoder(&mut self, encoder: A::CommandEncoder) { pub(crate) fn release_encoder(&self, encoder: A::CommandEncoder) {
self.free_encoders.push(encoder); let mut free_encoders = self.free_encoders.lock();
free_encoders.push(encoder);
} }
/// Free the pool of command encoders. /// Free the pool of command encoders.
/// ///
/// This is only called when the `Device` is dropped. /// This is only called when the `Device` is dropped.
pub(crate) fn dispose(self, device: &A::Device) { pub(crate) fn dispose(&self, device: &A::Device) {
resource_log!( let mut free_encoders = self.free_encoders.lock();
"CommandAllocator::dispose encoders {}", resource_log!("CommandAllocator::dispose encoders {}", free_encoders.len());
self.free_encoders.len() for cmd_encoder in free_encoders.drain(..) {
);
for cmd_encoder in self.free_encoders {
unsafe { unsafe {
device.destroy_command_encoder(cmd_encoder); device.destroy_command_encoder(cmd_encoder);
} }

View File

@ -1351,9 +1351,6 @@ impl Global {
}; };
let encoder = match device let encoder = match device
.command_allocator .command_allocator
.lock()
.as_mut()
.unwrap()
.acquire_encoder(device.raw(), queue.raw.as_ref().unwrap()) .acquire_encoder(device.raw(), queue.raw.as_ref().unwrap())
{ {
Ok(raw) => raw, Ok(raw) => raw,

View File

@ -361,7 +361,7 @@ impl<A: HalApi> LifetimeTracker<A> {
pub fn triage_submissions( pub fn triage_submissions(
&mut self, &mut self,
last_done: SubmissionIndex, last_done: SubmissionIndex,
command_allocator: &mut crate::command::CommandAllocator<A>, command_allocator: &crate::command::CommandAllocator<A>,
) -> SmallVec<[SubmittedWorkDoneClosure; 1]> { ) -> SmallVec<[SubmittedWorkDoneClosure; 1]> {
profiling::scope!("triage_submissions"); profiling::scope!("triage_submissions");

View File

@ -258,7 +258,7 @@ impl<A: HalApi> PendingWrites<A> {
#[must_use] #[must_use]
fn post_submit( fn post_submit(
&mut self, &mut self,
command_allocator: &mut CommandAllocator<A>, command_allocator: &CommandAllocator<A>,
device: &A::Device, device: &A::Device,
queue: &A::Queue, queue: &A::Queue,
) -> Option<EncoderInFlight<A>> { ) -> Option<EncoderInFlight<A>> {
@ -1530,7 +1530,7 @@ impl Global {
profiling::scope!("cleanup"); profiling::scope!("cleanup");
if let Some(pending_execution) = pending_writes.post_submit( if let Some(pending_execution) = pending_writes.post_submit(
device.command_allocator.lock().as_mut().unwrap(), &device.command_allocator,
device.raw(), device.raw(),
queue.raw.as_ref().unwrap(), queue.raw.as_ref().unwrap(),
) { ) {

View File

@ -97,7 +97,7 @@ pub struct Device<A: HalApi> {
pub(crate) zero_buffer: Option<A::Buffer>, pub(crate) zero_buffer: Option<A::Buffer>,
pub(crate) info: ResourceInfo<Device<A>>, pub(crate) info: ResourceInfo<Device<A>>,
pub(crate) command_allocator: Mutex<Option<command::CommandAllocator<A>>>, pub(crate) command_allocator: command::CommandAllocator<A>,
//Note: The submission index here corresponds to the last submission that is done. //Note: The submission index here corresponds to the last submission that is done.
pub(crate) active_submission_index: AtomicU64, //SubmissionIndex, pub(crate) active_submission_index: AtomicU64, //SubmissionIndex,
// NOTE: if both are needed, the `snatchable_lock` must be consistently acquired before the // NOTE: if both are needed, the `snatchable_lock` must be consistently acquired before the
@ -165,7 +165,7 @@ impl<A: HalApi> Drop for Device<A> {
let raw = self.raw.take().unwrap(); let raw = self.raw.take().unwrap();
let pending_writes = self.pending_writes.lock().take().unwrap(); let pending_writes = self.pending_writes.lock().take().unwrap();
pending_writes.dispose(&raw); pending_writes.dispose(&raw);
self.command_allocator.lock().take().unwrap().dispose(&raw); self.command_allocator.dispose(&raw);
unsafe { unsafe {
raw.destroy_buffer(self.zero_buffer.take().unwrap()); raw.destroy_buffer(self.zero_buffer.take().unwrap());
raw.destroy_fence(self.fence.write().take().unwrap()); raw.destroy_fence(self.fence.write().take().unwrap());
@ -223,7 +223,7 @@ impl<A: HalApi> Device<A> {
let fence = let fence =
unsafe { raw_device.create_fence() }.map_err(|_| CreateDeviceError::OutOfMemory)?; unsafe { raw_device.create_fence() }.map_err(|_| CreateDeviceError::OutOfMemory)?;
let mut com_alloc = command::CommandAllocator::new(); let com_alloc = command::CommandAllocator::new();
let pending_encoder = com_alloc let pending_encoder = com_alloc
.acquire_encoder(&raw_device, raw_queue) .acquire_encoder(&raw_device, raw_queue)
.map_err(|_| CreateDeviceError::OutOfMemory)?; .map_err(|_| CreateDeviceError::OutOfMemory)?;
@ -269,7 +269,7 @@ impl<A: HalApi> Device<A> {
queue_to_drop: OnceCell::new(), queue_to_drop: OnceCell::new(),
zero_buffer: Some(zero_buffer), zero_buffer: Some(zero_buffer),
info: ResourceInfo::new("<device>", None), info: ResourceInfo::new("<device>", None),
command_allocator: Mutex::new(Some(com_alloc)), command_allocator: com_alloc,
active_submission_index: AtomicU64::new(0), active_submission_index: AtomicU64::new(0),
fence: RwLock::new(Some(fence)), fence: RwLock::new(Some(fence)),
snatchable_lock: unsafe { SnatchLock::new() }, snatchable_lock: unsafe { SnatchLock::new() },
@ -423,10 +423,8 @@ impl<A: HalApi> Device<A> {
}; };
let mut life_tracker = self.lock_life(); let mut life_tracker = self.lock_life();
let submission_closures = life_tracker.triage_submissions( let submission_closures =
last_done_index, life_tracker.triage_submissions(last_done_index, &self.command_allocator);
self.command_allocator.lock().as_mut().unwrap(),
);
{ {
// Normally, `temp_suspected` exists only to save heap // Normally, `temp_suspected` exists only to save heap
@ -3483,10 +3481,9 @@ impl<A: HalApi> Device<A> {
.map_err(DeviceError::from)? .map_err(DeviceError::from)?
}; };
drop(guard); drop(guard);
let closures = self.lock_life().triage_submissions( let closures = self
submission_index, .lock_life()
self.command_allocator.lock().as_mut().unwrap(), .triage_submissions(submission_index, &self.command_allocator);
);
assert!( assert!(
closures.is_empty(), closures.is_empty(),
"wait_for_submit is not expected to work with closures" "wait_for_submit is not expected to work with closures"
@ -3614,10 +3611,7 @@ impl<A: HalApi> Device<A> {
log::error!("failed to wait for the device: {error}"); log::error!("failed to wait for the device: {error}");
} }
let mut life_tracker = self.lock_life(); let mut life_tracker = self.lock_life();
let _ = life_tracker.triage_submissions( let _ = life_tracker.triage_submissions(current_index, &self.command_allocator);
current_index,
self.command_allocator.lock().as_mut().unwrap(),
);
if let Some(device_lost_closure) = life_tracker.device_lost_closure.take() { if let Some(device_lost_closure) = life_tracker.device_lost_closure.take() {
// It's important to not hold the lock while calling the closure. // It's important to not hold the lock while calling the closure.
drop(life_tracker); drop(life_tracker);