From 6cc53421bfcd513bff4e3ceb7b434955443c3141 Mon Sep 17 00:00:00 2001 From: teoxoy <28601907+teoxoy@users.noreply.github.com> Date: Tue, 15 Oct 2024 12:41:29 +0200 Subject: [PATCH] move `LifetimeTracker` into the `Queue` The `Device` should not contain any `Arc`s to resources as that creates cycles (since all resources hold strong references to the `Device`). Note that `LifetimeTracker` internally has `Arc`s to resources. --- wgpu-core/src/device/global.rs | 8 +++-- wgpu-core/src/device/queue.rs | 15 +++++--- wgpu-core/src/device/resource.rs | 50 +++++++++++++-------------- wgpu-core/src/lock/rank.rs | 4 +-- wgpu-core/src/resource.rs | 59 ++++++++++++++++---------------- 5 files changed, 73 insertions(+), 63 deletions(-) diff --git a/wgpu-core/src/device/global.rs b/wgpu-core/src/device/global.rs index 6f0359e21..f9b6794b4 100644 --- a/wgpu-core/src/device/global.rs +++ b/wgpu-core/src/device/global.rs @@ -219,9 +219,11 @@ impl Global { device.check_is_valid()?; buffer.check_usage(wgt::BufferUsages::MAP_WRITE)?; - let last_submission = device - .lock_life() - .get_buffer_latest_submission_index(&buffer); + let last_submission = device.get_queue().and_then(|queue| { + queue + .lock_life() + .get_buffer_latest_submission_index(&buffer) + }); if let Some(last_submission) = last_submission { device.wait_for_submit(last_submission)?; diff --git a/wgpu-core/src/device/queue.rs b/wgpu-core/src/device/queue.rs index c8862a518..afe763d77 100644 --- a/wgpu-core/src/device/queue.rs +++ b/wgpu-core/src/device/queue.rs @@ -14,7 +14,7 @@ use crate::{ hal_label, id::{self, QueueId}, init_tracker::{has_copy_partial_init_tracker_coverage, TextureInitRange}, - lock::{rank, Mutex, RwLockWriteGuard}, + lock::{rank, Mutex, MutexGuard, RwLockWriteGuard}, resource::{ Buffer, BufferAccessError, BufferMapState, DestroyedBuffer, DestroyedResourceError, DestroyedTexture, Fallible, FlushedStagingBuffer, InvalidResourceError, Labeled, @@ -37,12 +37,13 @@ use std::{ }; use thiserror::Error; -use super::Device; +use super::{life::LifetimeTracker, Device}; pub struct Queue { raw: ManuallyDrop>, pub(crate) device: Arc, pub(crate) pending_writes: Mutex>, + life_tracker: Mutex, } impl Queue { @@ -94,12 +95,18 @@ impl Queue { raw: ManuallyDrop::new(raw), device, pending_writes, + life_tracker: Mutex::new(rank::QUEUE_LIFE_TRACKER, LifetimeTracker::new()), }) } pub(crate) fn raw(&self) -> &dyn hal::DynQueue { self.raw.as_ref() } + + #[track_caller] + pub(crate) fn lock_life<'a>(&'a self) -> MutexGuard<'a, LifetimeTracker> { + self.life_tracker.lock() + } } crate::impl_resource_type!(Queue); @@ -1292,7 +1299,7 @@ impl Queue { profiling::scope!("cleanup"); // this will register the new submission to the life time tracker - self.device.lock_life().track_submission( + self.lock_life().track_submission( submit_index, pending_writes.temp_resources.drain(..), active_executions, @@ -1341,7 +1348,7 @@ impl Queue { ) -> Option { api_log!("Queue::on_submitted_work_done"); //TODO: flush pending writes - self.device.lock_life().add_work_done_closure(closure) + self.lock_life().add_work_done_closure(closure) } } diff --git a/wgpu-core/src/device/resource.rs b/wgpu-core/src/device/resource.rs index 5f50feed8..ee1f25233 100644 --- a/wgpu-core/src/device/resource.rs +++ b/wgpu-core/src/device/resource.rs @@ -4,10 +4,9 @@ use crate::{ binding_model::{self, BindGroup, BindGroupLayout, BindGroupLayoutEntryError}, command, conv, device::{ - bgl, create_validator, - life::{LifetimeTracker, WaitIdleError}, - map_buffer, AttachmentData, DeviceLostInvocation, HostMap, MissingDownlevelFlags, - MissingFeatures, RenderPassContext, CLEANUP_WAIT_MS, + bgl, create_validator, life::WaitIdleError, map_buffer, AttachmentData, + DeviceLostInvocation, HostMap, MissingDownlevelFlags, MissingFeatures, RenderPassContext, + CLEANUP_WAIT_MS, }, hal_label, init_tracker::{ @@ -15,7 +14,7 @@ use crate::{ TextureInitTrackerAction, }, instance::Adapter, - lock::{rank, Mutex, MutexGuard, RwLock}, + lock::{rank, Mutex, RwLock}, pipeline, pool::ResourcePool, resource::{ @@ -114,7 +113,6 @@ pub struct Device { /// Stores the state of buffers and textures. pub(crate) trackers: Mutex, pub(crate) tracker_indices: TrackerIndexAllocators, - life_tracker: Mutex, /// Pool of bind group layouts, allowing deduplication. pub(crate) bgl_pool: ResourcePool, pub(crate) alignments: hal::Alignments, @@ -266,7 +264,6 @@ impl Device { device_lost_closure: Mutex::new(rank::DEVICE_LOST_CLOSURE, None), trackers: Mutex::new(rank::DEVICE_TRACKERS, DeviceTracker::new()), tracker_indices: TrackerIndexAllocators::new(), - life_tracker: Mutex::new(rank::DEVICE_LIFE_TRACKER, LifetimeTracker::new()), bgl_pool: ResourcePool::new(), #[cfg(feature = "trace")] trace: Mutex::new( @@ -332,11 +329,6 @@ impl Device { assert!(self.queue_to_drop.set(queue).is_ok()); } - #[track_caller] - pub(crate) fn lock_life<'a>(&'a self) -> MutexGuard<'a, LifetimeTracker> { - self.life_tracker.lock() - } - /// Run some destroy operations that were deferred. /// /// Destroying the resources requires taking a write lock on the device's snatch lock, @@ -449,13 +441,20 @@ impl Device { .map_err(|e| self.handle_hal_error(e))?; } - let mut life_tracker = self.lock_life(); - let submission_closures = - life_tracker.triage_submissions(submission_index, &self.command_allocator); + let (submission_closures, mapping_closures, queue_empty) = + if let Some(queue) = self.get_queue() { + let mut life_tracker = queue.lock_life(); + let submission_closures = + life_tracker.triage_submissions(submission_index, &self.command_allocator); - let mapping_closures = life_tracker.handle_mapping(self.raw(), &snatch_guard); + let mapping_closures = life_tracker.handle_mapping(self.raw(), &snatch_guard); - let queue_empty = life_tracker.queue_empty(); + let queue_empty = life_tracker.queue_empty(); + + (submission_closures, mapping_closures, queue_empty) + } else { + (SmallVec::new(), Vec::new(), true) + }; // Detect if we have been destroyed and now need to lose the device. // If we are invalid (set at start of destroy) and our queue is empty, @@ -481,7 +480,6 @@ impl Device { } // Don't hold the locks while calling release_gpu_resources. - drop(life_tracker); drop(fence); drop(snatch_guard); @@ -3562,13 +3560,15 @@ impl Device { unsafe { self.raw().wait(fence.as_ref(), submission_index, !0) } .map_err(|e| self.handle_hal_error(e))?; drop(fence); - let closures = self - .lock_life() - .triage_submissions(submission_index, &self.command_allocator); - assert!( - closures.is_empty(), - "wait_for_submit is not expected to work with closures" - ); + if let Some(queue) = self.get_queue() { + let closures = queue + .lock_life() + .triage_submissions(submission_index, &self.command_allocator); + assert!( + closures.is_empty(), + "wait_for_submit is not expected to work with closures" + ); + } } Ok(()) } diff --git a/wgpu-core/src/lock/rank.rs b/wgpu-core/src/lock/rank.rs index 1e8b4d2ae..51c6c5431 100644 --- a/wgpu-core/src/lock/rank.rs +++ b/wgpu-core/src/lock/rank.rs @@ -118,9 +118,9 @@ define_lock_ranks! { rank QUEUE_PENDING_WRITES "Queue::pending_writes" followed by { COMMAND_ALLOCATOR_FREE_ENCODERS, SHARED_TRACKER_INDEX_ALLOCATOR_INNER, - DEVICE_LIFE_TRACKER, + QUEUE_LIFE_TRACKER, } - rank DEVICE_LIFE_TRACKER "Device::life_tracker" followed by { + rank QUEUE_LIFE_TRACKER "Queue::life_tracker" followed by { COMMAND_ALLOCATOR_FREE_ENCODERS, DEVICE_TRACE, } diff --git a/wgpu-core/src/resource.rs b/wgpu-core/src/resource.rs index 88b76bb67..6d7544d9b 100644 --- a/wgpu-core/src/resource.rs +++ b/wgpu-core/src/resource.rs @@ -634,7 +634,12 @@ impl Buffer { .buffers .set_single(self, internal_use); - let submit_index = device.lock_life().map(self).unwrap_or(0); // '0' means no wait is necessary + let submit_index = if let Some(queue) = device.get_queue() { + queue.lock_life().map(self).unwrap_or(0) // '0' means no wait is necessary + } else { + // TODO: map immediately + 0 + }; Ok(submit_index) } @@ -783,16 +788,15 @@ impl Buffer { let mut pending_writes = queue.pending_writes.lock(); if pending_writes.contains_buffer(self) { pending_writes.consume_temp(temp); - return Ok(()); + } else { + let mut life_lock = queue.lock_life(); + let last_submit_index = life_lock.get_buffer_latest_submission_index(self); + if let Some(last_submit_index) = last_submit_index { + life_lock.schedule_resource_destruction(temp, last_submit_index); + } } } - let mut life_lock = device.lock_life(); - let last_submit_index = life_lock.get_buffer_latest_submission_index(self); - if let Some(last_submit_index) = last_submit_index { - life_lock.schedule_resource_destruction(temp, last_submit_index); - } - Ok(()) } } @@ -1252,16 +1256,15 @@ impl Texture { let mut pending_writes = queue.pending_writes.lock(); if pending_writes.contains_texture(self) { pending_writes.consume_temp(temp); - return Ok(()); + } else { + let mut life_lock = queue.lock_life(); + let last_submit_index = life_lock.get_texture_latest_submission_index(self); + if let Some(last_submit_index) = last_submit_index { + life_lock.schedule_resource_destruction(temp, last_submit_index); + } } } - let mut life_lock = device.lock_life(); - let last_submit_index = life_lock.get_texture_latest_submission_index(self); - if let Some(last_submit_index) = last_submit_index { - life_lock.schedule_resource_destruction(temp, last_submit_index); - } - Ok(()) } } @@ -1971,16 +1974,15 @@ impl Blas { let mut pending_writes = queue.pending_writes.lock(); if pending_writes.contains_blas(self) { pending_writes.consume_temp(temp); - return Ok(()); + } else { + let mut life_lock = queue.lock_life(); + let last_submit_index = life_lock.get_blas_latest_submission_index(self); + if let Some(last_submit_index) = last_submit_index { + life_lock.schedule_resource_destruction(temp, last_submit_index); + } } } - let mut life_lock = device.lock_life(); - let last_submit_index = life_lock.get_blas_latest_submission_index(self); - if let Some(last_submit_index) = last_submit_index { - life_lock.schedule_resource_destruction(temp, last_submit_index); - } - Ok(()) } } @@ -2061,16 +2063,15 @@ impl Tlas { let mut pending_writes = queue.pending_writes.lock(); if pending_writes.contains_tlas(self) { pending_writes.consume_temp(temp); - return Ok(()); + } else { + let mut life_lock = queue.lock_life(); + let last_submit_index = life_lock.get_tlas_latest_submission_index(self); + if let Some(last_submit_index) = last_submit_index { + life_lock.schedule_resource_destruction(temp, last_submit_index); + } } } - let mut life_lock = device.lock_life(); - let last_submit_index = life_lock.get_tlas_latest_submission_index(self); - if let Some(last_submit_index) = last_submit_index { - life_lock.schedule_resource_destruction(temp, last_submit_index); - } - Ok(()) } }