From 5f464688e693ee0f16003ade3c02041810a2f75a Mon Sep 17 00:00:00 2001 From: Jim Blandy Date: Thu, 2 May 2024 21:06:41 -0700 Subject: [PATCH] [core] Don't check for uniquely referenced resources at submission. Remove unreachable code from `Global::queue_submit` that checks whether the resources used by the command buffer have a reference count of one, and adds them to `Device::temp_suspected` if so. When `queue_submit` is called, all the `Arc`s processed by this code have a reference count of at least three, even when the user has dropped the resource: - `Device::trackers` holds strong references to all the device's resources. - `CommandBufferMutable::trackers` holds strong references to all resources used by the command buffer. - The `used_resources` methods of the various members of `CommandBufferMutable::trackers` all return iterators of owned `Arc`s. Fortunately, since the `Global::device_drop_foo` methods all add the `foo` being dropped to `Device::suspected_resources`, and `LifetimeTracker::triage_suspected` does an adequate job of accounting for the uninteresting `Arc`s and leaves the resources there until they're actually dead, things do get cleaned up without the checks in `Global::queue_submit`. This allows `Device::temp_suspected` to be private to `device::resource`, with a sole remaining use in `Device::untrack`. Fixes #5647. --- wgpu-core/src/device/queue.rs | 94 +++++--------------------------- wgpu-core/src/device/resource.rs | 25 ++------- 2 files changed, 19 insertions(+), 100 deletions(-) diff --git a/wgpu-core/src/device/queue.rs b/wgpu-core/src/device/queue.rs index 76d2b1132..f0db961ff 100644 --- a/wgpu-core/src/device/queue.rs +++ b/wgpu-core/src/device/queue.rs @@ -7,7 +7,7 @@ use crate::{ ClearError, CommandAllocator, CommandBuffer, CopySide, ImageCopyTexture, TransferError, }, conv, - device::{life::ResourceMaps, DeviceError, WaitIdleError}, + device::{DeviceError, WaitIdleError}, get_lowest_common_denom, global::Global, hal_api::HalApi, @@ -1183,11 +1183,6 @@ impl Global { //TODO: if multiple command buffers are submitted, we can re-use the last // native command buffer of the previous chain instead of always creating // a temporary one, since the chains are not finished. - let mut temp_suspected = device.temp_suspected.lock(); - { - let mut suspected = temp_suspected.replace(ResourceMaps::new()).unwrap(); - suspected.clear(); - } // finish all the command buffers first for &cmb_id in command_buffer_ids { @@ -1235,41 +1230,23 @@ impl Global { // update submission IDs for buffer in cmd_buf_trackers.buffers.used_resources() { - let tracker_index = buffer.info.tracker_index(); - let raw_buf = match buffer.raw.get(&snatch_guard) { - Some(raw) => raw, - None => { - return Err(QueueSubmitError::DestroyedBuffer( - buffer.info.id(), - )); - } - }; + if buffer.raw.get(&snatch_guard).is_none() { + return Err(QueueSubmitError::DestroyedBuffer( + buffer.info.id(), + )); + } buffer.info.use_at(submit_index); - if buffer.is_unique() { - if let BufferMapState::Active { .. } = *buffer.map_state.lock() - { - log::warn!("Dropped buffer has a pending mapping."); - unsafe { device.raw().unmap_buffer(raw_buf) } - .map_err(DeviceError::from)?; - } - temp_suspected - .as_mut() - .unwrap() - .buffers - .insert(tracker_index, buffer.clone()); - } else { - match *buffer.map_state.lock() { - BufferMapState::Idle => (), - _ => { - return Err(QueueSubmitError::BufferStillMapped( - buffer.info.id(), - )) - } + + match *buffer.map_state.lock() { + BufferMapState::Idle => (), + _ => { + return Err(QueueSubmitError::BufferStillMapped( + buffer.info.id(), + )) } } } for texture in cmd_buf_trackers.textures.used_resources() { - let tracker_index = texture.info.tracker_index(); let should_extend = match texture.inner.get(&snatch_guard) { None => { return Err(QueueSubmitError::DestroyedTexture( @@ -1286,13 +1263,6 @@ impl Global { } }; texture.info.use_at(submit_index); - if texture.is_unique() { - temp_suspected - .as_mut() - .unwrap() - .textures - .insert(tracker_index, texture.clone()); - } if should_extend { unsafe { used_surface_textures @@ -1303,12 +1273,6 @@ impl Global { } for texture_view in cmd_buf_trackers.views.used_resources() { texture_view.info.use_at(submit_index); - if texture_view.is_unique() { - temp_suspected.as_mut().unwrap().texture_views.insert( - texture_view.as_info().tracker_index(), - texture_view.clone(), - ); - } } { for bg in cmd_buf_trackers.bind_groups.used_resources() { @@ -1322,13 +1286,6 @@ impl Global { for sampler in bg.used.samplers.used_resources() { sampler.info.use_at(submit_index); } - if bg.is_unique() { - temp_suspected - .as_mut() - .unwrap() - .bind_groups - .insert(bg.as_info().tracker_index(), bg.clone()); - } } } // assert!(cmd_buf_trackers.samplers.is_empty()); @@ -1336,32 +1293,14 @@ impl Global { cmd_buf_trackers.compute_pipelines.used_resources() { compute_pipeline.info.use_at(submit_index); - if compute_pipeline.is_unique() { - temp_suspected.as_mut().unwrap().compute_pipelines.insert( - compute_pipeline.as_info().tracker_index(), - compute_pipeline.clone(), - ); - } } for render_pipeline in cmd_buf_trackers.render_pipelines.used_resources() { render_pipeline.info.use_at(submit_index); - if render_pipeline.is_unique() { - temp_suspected.as_mut().unwrap().render_pipelines.insert( - render_pipeline.as_info().tracker_index(), - render_pipeline.clone(), - ); - } } for query_set in cmd_buf_trackers.query_sets.used_resources() { query_set.info.use_at(submit_index); - if query_set.is_unique() { - temp_suspected.as_mut().unwrap().query_sets.insert( - query_set.as_info().tracker_index(), - query_set.clone(), - ); - } } for bundle in cmd_buf_trackers.bundles.used_resources() { bundle.info.use_at(submit_index); @@ -1376,13 +1315,6 @@ impl Global { for query_set in bundle.used.query_sets.read().used_resources() { query_set.info.use_at(submit_index); } - if bundle.is_unique() { - temp_suspected - .as_mut() - .unwrap() - .render_bundles - .insert(bundle.as_info().tracker_index(), bundle.clone()); - } } } let mut baked = cmdbuf.from_arc_into_baked(); diff --git a/wgpu-core/src/device/resource.rs b/wgpu-core/src/device/resource.rs index c571090a4..aa5576b82 100644 --- a/wgpu-core/src/device/resource.rs +++ b/wgpu-core/src/device/resource.rs @@ -127,9 +127,6 @@ pub struct Device { pub(crate) tracker_indices: TrackerIndexAllocators, // Life tracker should be locked right after the device and before anything else. life_tracker: Mutex>, - /// Temporary storage for resource management functions. Cleared at the end - /// of every call (unless an error occurs). - pub(crate) temp_suspected: Mutex>>, /// Pool of bind group layouts, allowing deduplication. pub(crate) bgl_pool: ResourcePool>, pub(crate) alignments: hal::Alignments, @@ -142,6 +139,10 @@ pub struct Device { #[cfg(feature = "trace")] pub(crate) trace: Mutex>, pub(crate) usage_scopes: UsageScopePool, + + /// Temporary storage, cleared at the start of every call, + /// retained only to save allocations. + temp_suspected: Mutex>>, } pub(crate) enum DeferredDestroy { @@ -434,23 +435,9 @@ impl Device { let submission_closures = life_tracker.triage_submissions(last_done_index, &self.command_allocator); - { - // Normally, `temp_suspected` exists only to save heap - // allocations: it's cleared at the start of the function - // call, and cleared by the end. But `Global::queue_submit` is - // fallible; if it exits early, it may leave some resources in - // `temp_suspected`. - let temp_suspected = self - .temp_suspected - .lock() - .replace(ResourceMaps::new()) - .unwrap(); + life_tracker.triage_suspected(&self.trackers); - life_tracker.suspected_resources.extend(temp_suspected); - - life_tracker.triage_suspected(&self.trackers); - life_tracker.triage_mapped(); - } + life_tracker.triage_mapped(); let mapping_closures = life_tracker.handle_mapping(self.raw(), &self.trackers, &snatch_guard);