From b30c0c2e0097f64b7c702b00eb288e2dfb8045e9 Mon Sep 17 00:00:00 2001 From: Nicolas Silva Date: Thu, 11 Jan 2024 10:41:09 +0100 Subject: [PATCH] Remove the free_resources tracker (#5037) --- wgpu-core/src/device/life.rs | 127 +++++++++---------------------- wgpu-core/src/device/resource.rs | 4 - 2 files changed, 35 insertions(+), 96 deletions(-) diff --git a/wgpu-core/src/device/life.rs b/wgpu-core/src/device/life.rs index 3da7e0b0db..22699d1068 100644 --- a/wgpu-core/src/device/life.rs +++ b/wgpu-core/src/device/life.rs @@ -142,9 +142,8 @@ struct ActiveSubmission { /// Resources to be freed once this queue submission has completed. /// /// When the device is polled, for completed submissions, - /// `triage_submissions` merges these into - /// `LifetimeTracker::free_resources`. From there, - /// `LifetimeTracker::cleanup` passes them to the hal to be freed. + /// `triage_submissions` removes resources that don't need to be held alive any longer + /// from there. /// /// This includes things like temporary resources and resources that are /// used by submitted commands but have been dropped by the user (meaning that @@ -205,8 +204,6 @@ pub enum WaitIdleError { /// buffers that were dropped by the user get moved to /// `self.free_resources`. /// -/// 4) `cleanup` frees everything in `free_resources`. -/// /// Only calling `Global::buffer_map_async` clones a new `Arc` for the /// buffer. This new `Arc` is only dropped by `handle_mapping`. pub(crate) struct LifetimeTracker { @@ -233,13 +230,6 @@ pub(crate) struct LifetimeTracker { /// to particular entries. active: Vec>, - /// Raw backend resources that are neither referenced nor used. - /// - /// These are freed by `LifeTracker::cleanup`, which is called from periodic - /// maintenance functions like `Global::device_poll`, and when a device is - /// destroyed. - free_resources: ResourceMaps, - /// Buffers the user has asked us to map, and which are not used by any /// queue submission still in flight. ready_to_map: Vec>>, @@ -264,7 +254,6 @@ impl LifetimeTracker { future_suspected_textures: Vec::new(), suspected_resources: ResourceMaps::new(), active: Vec::new(), - free_resources: ResourceMaps::new(), ready_to_map: Vec::new(), work_done_closures: SmallVec::new(), device_lost_closure: None, @@ -374,7 +363,6 @@ impl LifetimeTracker { let mut work_done_closures: SmallVec<_> = self.work_done_closures.drain(..).collect(); for a in self.active.drain(..done_count) { log::debug!("Active submission {} is done", a.index); - self.free_resources.extend(a.last_resources); self.ready_to_map.extend(a.mapped); for encoder in a.encoders { let raw = unsafe { encoder.land() }; @@ -385,11 +373,6 @@ impl LifetimeTracker { work_done_closures } - pub fn cleanup(&mut self) { - profiling::scope!("LifetimeTracker::cleanup"); - self.free_resources.clear(); - } - pub fn schedule_resource_destruction( &mut self, temp_resource: TempResource, @@ -399,22 +382,24 @@ impl LifetimeTracker { .active .iter_mut() .find(|a| a.index == last_submit_index) - .map_or(&mut self.free_resources, |a| &mut a.last_resources); - match temp_resource { - TempResource::Buffer(raw) => { - resources.buffers.insert(raw.as_info().id(), raw); - } - TempResource::StagingBuffer(raw) => { - resources.staging_buffers.insert(raw.as_info().id(), raw); - } - TempResource::DestroyedBuffer(destroyed) => { - resources.destroyed_buffers.insert(destroyed.id, destroyed); - } - TempResource::Texture(raw) => { - resources.textures.insert(raw.as_info().id(), raw); - } - TempResource::DestroyedTexture(destroyed) => { - resources.destroyed_textures.insert(destroyed.id, destroyed); + .map(|a| &mut a.last_resources); + if let Some(resources) = resources { + match temp_resource { + TempResource::Buffer(raw) => { + resources.buffers.insert(raw.as_info().id(), raw); + } + TempResource::StagingBuffer(raw) => { + resources.staging_buffers.insert(raw.as_info().id(), raw); + } + TempResource::DestroyedBuffer(destroyed) => { + resources.destroyed_buffers.insert(destroyed.id, destroyed); + } + TempResource::Texture(raw) => { + resources.textures.insert(raw.as_info().id(), raw); + } + TempResource::DestroyedTexture(destroyed) => { + resources.destroyed_textures.insert(destroyed.id, destroyed); + } } } } @@ -437,7 +422,6 @@ impl LifetimeTracker { fn triage_resources( resources_map: &mut FastHashMap>, active: &mut [ActiveSubmission], - free_resources: &mut ResourceMaps, trackers: &mut impl ResourceTracker, get_resource_map: impl Fn(&mut ResourceMaps) -> &mut FastHashMap>, ) -> Vec> @@ -451,12 +435,14 @@ impl LifetimeTracker { let non_referenced_resources = active .iter_mut() .find(|a| a.index == submit_index) - .map_or(&mut *free_resources, |a| &mut a.last_resources); + .map(|a| &mut a.last_resources); let is_removed = trackers.remove_abandoned(id); if is_removed { removed_resources.push(resource.clone()); - get_resource_map(non_referenced_resources).insert(id, resource.clone()); + if let Some(ressources) = non_referenced_resources { + get_resource_map(ressources).insert(id, resource.clone()); + } } !is_removed }); @@ -469,7 +455,6 @@ impl LifetimeTracker { let mut removed_resources = Self::triage_resources( resource_map, self.active.as_mut_slice(), - &mut self.free_resources, &mut trackers.bundles, |maps| &mut maps.render_bundles, ); @@ -507,7 +492,6 @@ impl LifetimeTracker { let mut removed_resource = Self::triage_resources( resource_map, self.active.as_mut_slice(), - &mut self.free_resources, &mut trackers.bind_groups, |maps| &mut maps.bind_groups, ); @@ -544,7 +528,6 @@ impl LifetimeTracker { let mut removed_resources = Self::triage_resources( resource_map, self.active.as_mut_slice(), - &mut self.free_resources, &mut trackers.views, |maps| &mut maps.texture_views, ); @@ -565,7 +548,6 @@ impl LifetimeTracker { Self::triage_resources( resource_map, self.active.as_mut_slice(), - &mut self.free_resources, &mut trackers.textures, |maps| &mut maps.textures, ); @@ -578,7 +560,6 @@ impl LifetimeTracker { Self::triage_resources( resource_map, self.active.as_mut_slice(), - &mut self.free_resources, &mut trackers.samplers, |maps| &mut maps.samplers, ); @@ -588,23 +569,13 @@ impl LifetimeTracker { fn triage_suspected_buffers(&mut self, trackers: &Mutex>) -> &mut Self { let mut trackers = trackers.lock(); let resource_map = &mut self.suspected_resources.buffers; - let mut removed_resources = Self::triage_resources( + Self::triage_resources( resource_map, self.active.as_mut_slice(), - &mut self.free_resources, &mut trackers.buffers, |maps| &mut maps.buffers, ); - removed_resources.drain(..).for_each(|buffer| { - if let resource::BufferMapState::Init { - ref stage_buffer, .. - } = *buffer.map_state.lock() - { - self.free_resources - .buffers - .insert(stage_buffer.as_info().id(), stage_buffer.clone()); - } - }); + self } @@ -616,8 +587,6 @@ impl LifetimeTracker { .last_resources .destroyed_buffers .insert(id, buffer); - } else { - self.free_resources.destroyed_buffers.insert(id, buffer); } } } @@ -630,8 +599,6 @@ impl LifetimeTracker { .last_resources .destroyed_textures .insert(id, texture); - } else { - self.free_resources.destroyed_textures.insert(id, texture); } } } @@ -642,7 +609,6 @@ impl LifetimeTracker { let mut removed_resources = Self::triage_resources( resource_map, self.active.as_mut_slice(), - &mut self.free_resources, &mut trackers.compute_pipelines, |maps| &mut maps.compute_pipelines, ); @@ -661,7 +627,6 @@ impl LifetimeTracker { let mut removed_resources = Self::triage_resources( resource_map, self.active.as_mut_slice(), - &mut self.free_resources, &mut trackers.render_pipelines, |maps| &mut maps.render_pipelines, ); @@ -693,18 +658,11 @@ impl LifetimeTracker { } fn triage_suspected_bind_group_layouts(&mut self) -> &mut Self { - self.suspected_resources.bind_group_layouts.retain( - |bind_group_layout_id, bind_group_layout| { - //Note: this has to happen after all the suspected pipelines are destroyed - //Note: nothing else can bump the refcount since the guard is locked exclusively - //Note: same BGL can appear multiple times in the list, but only the last - // encounter could drop the refcount to 0. - self.free_resources - .bind_group_layouts - .insert(*bind_group_layout_id, bind_group_layout.clone()); - false - }, - ); + //Note: this has to happen after all the suspected pipelines are destroyed + //Note: nothing else can bump the refcount since the guard is locked exclusively + //Note: same BGL can appear multiple times in the list, but only the last + self.suspected_resources.bind_group_layouts.clear(); + self } @@ -714,7 +672,6 @@ impl LifetimeTracker { Self::triage_resources( resource_map, self.active.as_mut_slice(), - &mut self.free_resources, &mut trackers.query_sets, |maps| &mut maps.query_sets, ); @@ -722,14 +679,8 @@ impl LifetimeTracker { } fn triage_suspected_staging_buffers(&mut self) -> &mut Self { - self.suspected_resources - .staging_buffers - .retain(|staging_buffer_id, staging_buffer| { - self.free_resources - .staging_buffers - .insert(*staging_buffer_id, staging_buffer.clone()); - false - }); + self.suspected_resources.staging_buffers.clear(); + self } @@ -746,12 +697,8 @@ impl LifetimeTracker { /// - Add resources used by queue submissions still in flight to the /// [`last_resources`] table of the last such submission's entry in /// [`self.active`]. When that submission has finished execution. the - /// [`triage_submissions`] method will move them to - /// [`self.free_resources`]. - /// - /// - Add resources that can be freed right now to [`self.free_resources`] - /// directly. [`LifetimeTracker::cleanup`] will take care of them as - /// part of this poll. + /// [`triage_submissions`] method will remove from the tracker and the + /// resource reference count will be responsible carrying out deallocation. /// /// ## Entrained resources /// @@ -771,7 +718,6 @@ impl LifetimeTracker { /// [`last_resources`]: ActiveSubmission::last_resources /// [`self.active`]: LifetimeTracker::active /// [`triage_submissions`]: LifetimeTracker::triage_submissions - /// [`self.free_resources`]: LifetimeTracker::free_resources pub(crate) fn triage_suspected(&mut self, trackers: &Mutex>) { profiling::scope!("triage_suspected"); @@ -844,9 +790,6 @@ impl LifetimeTracker { if is_removed { *buffer.map_state.lock() = resource::BufferMapState::Idle; log::trace!("Buffer ready to map {:?} is not tracked anymore", buffer_id); - self.free_resources - .buffers - .insert(buffer_id, buffer.clone()); } else { let mapping = match std::mem::replace( &mut *buffer.map_state.lock(), diff --git a/wgpu-core/src/device/resource.rs b/wgpu-core/src/device/resource.rs index eab6f2a093..cf391a6011 100644 --- a/wgpu-core/src/device/resource.rs +++ b/wgpu-core/src/device/resource.rs @@ -372,9 +372,6 @@ impl Device { ); let mapping_closures = life_tracker.handle_mapping(self.raw(), &self.trackers); - //Cleaning up resources and released all unused suspected ones - life_tracker.cleanup(); - // 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, // and we have a DeviceLostClosure, return the closure to be called by @@ -3383,7 +3380,6 @@ impl Device { current_index, self.command_allocator.lock().as_mut().unwrap(), ); - life_tracker.cleanup(); #[cfg(feature = "trace")] { *self.trace.lock() = None;