Skip to content

Remove Clone Staring Me In the Face #6969

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 1 commit into
base: trunk
Choose a base branch
from
Draft

Remove Clone Staring Me In the Face #6969

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 6 additions & 9 deletions wgpu-core/src/track/buffer.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,8 @@ use crate::{
resource::{Buffer, Trackable},
snatch::SnatchGuard,
track::{
invalid_resource_state, skip_barrier, ResourceMetadata, ResourceMetadataProvider,
ResourceUsageCompatibilityError, ResourceUses,
invalid_resource_state, metadata::Insertable, skip_barrier, ResourceMetadata,
ResourceMetadataProvider, ResourceUsageCompatibilityError, ResourceUses,
},
};

Expand Down Expand Up @@ -585,9 +585,7 @@ impl DeviceBufferTracker {
index,
BufferStateProvider::Direct { state },
None,
ResourceMetadataProvider::Direct {
resource: &Arc::downgrade(buffer),
},
ResourceMetadataProvider::Direct { resource: buffer },
)
}
}
Expand Down Expand Up @@ -685,7 +683,7 @@ impl BufferStateProvider<'_> {
}

#[inline(always)]
unsafe fn insert<T: Clone>(
unsafe fn insert<T: Insertable>(
start_states: Option<&mut [BufferUses]>,
current_states: &mut [BufferUses],
resource_metadata: &mut ResourceMetadata<T>,
Expand All @@ -709,8 +707,7 @@ unsafe fn insert<T: Clone>(
}
*current_states.get_unchecked_mut(index) = new_end_state;

let resource = metadata_provider.get(index);
resource_metadata.insert(index, resource.clone());
resource_metadata.insert(index, metadata_provider);
}
}

Expand All @@ -729,7 +726,7 @@ unsafe fn merge(

if invalid_resource_state(merged_state) {
return Err(ResourceUsageCompatibilityError::from_buffer(
unsafe { metadata_provider.get(index) },
unsafe { &metadata_provider.get(index) },
*current_state,
new_state,
));
Expand Down
169 changes: 149 additions & 20 deletions wgpu-core/src/track/metadata.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,124 @@
//! The `ResourceMetadata` type.

use alloc::vec::Vec;
use alloc::{sync::Arc, sync::Weak, vec::Vec};

use bit_vec::BitVec;
use wgt::strict_assert;

#[cold]
fn cold() {}

/// A trait for abstracting over Arc and Weak metadata when inserting
/// into a `Option`.
///
/// We want to avoid cloning or downgrading the value we're inserting
/// if it is already present in the `Option`, as refcounting is the
/// hostest bit of code in the tracker. This allows us to to insert
/// only if the value is different or not present.
///
/// Because [`ResourceMetadata`] is generic over [`Arc`] and [`Weak`],
/// we need a trait to give us the implementations for each.
pub trait Insertable: Sized {
/// The value owned within the `Option`.
///
/// This value is always `Arc<T>` where Self is `Arc<T>` or `Weak<T>`.
type OwnedValue: Clone;

/// Insert into the given option from an [`Arc`].
fn insert_from_owned<'a>(option: &'a mut Option<Self>, value: &Self::OwnedValue) -> &'a Self;
/// Insert into the given option from Self. This is only different for [`Weak`].
fn insert_from_self<'a>(option: &'a mut Option<Self>, value: &Self) -> &'a Self;
/// Get the inner value from whatever is in the option.
///
/// This always clones the value, but it is only called in the error case.
fn get_inner(option: &Self) -> Self::OwnedValue;
}

impl<T> Insertable for Arc<T> {
type OwnedValue = Arc<T>;

#[inline(always)]
fn insert_from_owned<'a>(option: &'a mut Option<Arc<T>>, new_value: &Arc<T>) -> &'a Arc<T> {
match option {
Some(old_value) => {
if !Arc::ptr_eq(old_value, new_value) {
// This should never happen. Because owning trackers keep their
// values alive, the value at a given tracker index should never
// change. We build in this check though to prevent bugs from
// causing silent corruption. However we mark it cold.
cold();

// Only clone and insert if the value is different.
*old_value = new_value.clone();
}
old_value
}
None => {
// If the option is empty, we must clone and insert the new value.
option.insert(new_value.clone())
}
}
}

#[inline(always)]
fn insert_from_self<'a>(option: &'a mut Option<Arc<T>>, new_value: &Arc<T>) -> &'a Arc<T> {
// `insert_from_owned` is the same as `insert_from_self` for Arc
Self::insert_from_owned(option, new_value)
}

#[inline(always)]
fn get_inner<'a>(option: &'a Arc<T>) -> Arc<T> {
option.clone()
}
}

impl<T> Insertable for Weak<T> {
type OwnedValue = Arc<T>;

#[inline(always)]
fn insert_from_owned<'a>(option: &'a mut Option<Weak<T>>, new_value: &Arc<T>) -> &'a Weak<T> {
match option {
Some(old_value) => {
if old_value.as_ptr() != Arc::as_ptr(new_value) {
// Unlike strongly owned trackers, the weak trackers (device tracker)
// never actually gets its items removed. We need to be able to overwrite
// the value in the tracker.

// Only downgrade and insert if the value is different.
*old_value = Arc::downgrade(new_value);
}
old_value
}
None => {
// If the option is empty, we must downgrade and insert the new value.
option.insert(Arc::downgrade(new_value))
}
}
}

#[inline(always)]
fn insert_from_self<'a>(option: &'a mut Option<Weak<T>>, new_value: &Weak<T>) -> &'a Weak<T> {
match option {
Some(old_value) => {
if old_value.as_ptr() != new_value.as_ptr() {
// Only clone and insert if the value is different.
*old_value = new_value.clone();
}
old_value
}
None => {
// If the option is empty, we must clone and insert the new value.
option.insert(new_value.clone())
}
}
}

#[inline(always)]
fn get_inner<'a>(option: &'a Weak<T>) -> Arc<T> {
option.upgrade().unwrap()
}
}

/// A set of resources, holding a `Arc<T>` and epoch for each member.
///
/// Testing for membership is fast, and iterating over members is
Expand All @@ -13,15 +127,15 @@ use wgt::strict_assert;
/// members, but a bit vector tracks occupancy, so iteration touches
/// only occupied elements.
#[derive(Debug)]
pub(super) struct ResourceMetadata<T: Clone> {
pub(super) struct ResourceMetadata<T> {
/// If the resource with index `i` is a member, `owned[i]` is `true`.
owned: BitVec<usize>,

/// A vector holding clones of members' `T`s.
resources: Vec<Option<T>>,
}

impl<T: Clone> ResourceMetadata<T> {
impl<T: Insertable> ResourceMetadata<T> {
pub(super) fn new() -> Self {
Self {
owned: BitVec::default(),
Expand All @@ -30,7 +144,7 @@ impl<T: Clone> ResourceMetadata<T> {
}

pub(super) fn set_size(&mut self, size: usize) {
self.resources.resize(size, None);
self.resources.resize_with(size, || None);
resize_bitvec(&mut self.owned, size);
}

Expand Down Expand Up @@ -90,10 +204,24 @@ impl<T: Clone> ResourceMetadata<T> {
/// The given `index` must be in bounds for this `ResourceMetadata`'s
/// existing tables. See `tracker_assert_in_bounds`.
#[inline(always)]
pub(super) unsafe fn insert(&mut self, index: usize, resource: T) -> &T {
pub(super) unsafe fn insert(
&mut self,
index: usize,
resource: ResourceMetadataProvider<'_, T>,
) -> &T {
self.owned.set(index, true);

let resource_dst = unsafe { self.resources.get_unchecked_mut(index) };
resource_dst.insert(resource)

match resource {
ResourceMetadataProvider::Direct { resource } => {
T::insert_from_owned(resource_dst, resource)
}
ResourceMetadataProvider::Indirect { metadata } => {
let resource = unsafe { metadata.get_resource_unchecked(index) };
T::insert_from_self(resource_dst, resource)
}
}
}

/// Get the resource with the given index.
Expand Down Expand Up @@ -144,29 +272,30 @@ impl<T: Clone> ResourceMetadata<T> {
///
/// This is used to abstract over the various places
/// trackers can get new resource metadata from.
pub(super) enum ResourceMetadataProvider<'a, T: Clone> {
pub(super) enum ResourceMetadataProvider<'a, T: Insertable> {
/// Comes directly from explicit values.
Direct { resource: &'a T },
Direct { resource: &'a T::OwnedValue },
/// Comes from another metadata tracker.
Indirect { metadata: &'a ResourceMetadata<T> },
}
impl<T: Clone> ResourceMetadataProvider<'_, T> {
/// Get a reference to the resource from this.
impl<T> ResourceMetadataProvider<'_, T>
where
T: Insertable,
{
/// Get a reference to the resource from this. This clones the resource in all pathways
/// and should not be called unless required. Currently this is only used in error paths
/// which do not need to be fast.
///
/// # Safety
///
/// - The index must be in bounds of the metadata tracker if this uses an indirect source.
#[inline(always)]
pub(super) unsafe fn get(&self, index: usize) -> &T {
match self {
ResourceMetadataProvider::Direct { resource } => resource,
ResourceMetadataProvider::Indirect { metadata } => {
metadata.tracker_assert_in_bounds(index);
{
let resource = unsafe { metadata.resources.get_unchecked(index) }.as_ref();
unsafe { resource.unwrap_unchecked() }
}
}
pub(super) unsafe fn get(&self, index: usize) -> T::OwnedValue {
match *self {
ResourceMetadataProvider::Direct { resource } => resource.clone(),
ResourceMetadataProvider::Indirect { metadata } => unsafe {
T::get_inner(metadata.get_resource_unchecked(index))
},
}
}
}
Expand Down
21 changes: 9 additions & 12 deletions wgpu-core/src/track/texture.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,8 @@ use crate::{
resource::{Texture, TextureInner, TextureView, Trackable},
snatch::SnatchGuard,
track::{
invalid_resource_state, skip_barrier, ResourceMetadata, ResourceMetadataProvider,
ResourceUsageCompatibilityError, ResourceUses,
invalid_resource_state, metadata::Insertable, skip_barrier, ResourceMetadata,
ResourceMetadataProvider, ResourceUsageCompatibilityError, ResourceUses,
},
};
use hal::TextureBarrier;
Expand Down Expand Up @@ -719,9 +719,7 @@ impl DeviceTextureTracker {
index,
TextureStateProvider::KnownSingle { state: usage },
None,
ResourceMetadataProvider::Direct {
resource: &Arc::downgrade(texture),
},
ResourceMetadataProvider::Direct { resource: texture },
)
};
}
Expand Down Expand Up @@ -1074,7 +1072,7 @@ unsafe fn insert_or_barrier_update(
}

#[inline(always)]
unsafe fn insert<T: Clone>(
unsafe fn insert<T: Insertable>(
texture_selector: Option<&TextureSelector>,
start_state: Option<&mut TextureStateSet>,
end_state: &mut TextureStateSet,
Expand Down Expand Up @@ -1143,8 +1141,7 @@ unsafe fn insert<T: Clone>(
}

unsafe {
let resource = metadata_provider.get(index);
resource_metadata.insert(index, resource.clone());
resource_metadata.insert(index, metadata_provider);
}
}

Expand All @@ -1166,7 +1163,7 @@ unsafe fn merge(

if invalid_resource_state(merged_state) {
return Err(ResourceUsageCompatibilityError::from_texture(
unsafe { metadata_provider.get(index) },
unsafe { &metadata_provider.get(index) },
texture_selector.clone(),
*current_simple,
new_simple,
Expand All @@ -1191,7 +1188,7 @@ unsafe fn merge(

if invalid_resource_state(merged_state) {
return Err(ResourceUsageCompatibilityError::from_texture(
unsafe { metadata_provider.get(index) },
unsafe { &metadata_provider.get(index) },
selector,
*current_simple,
new_state,
Expand Down Expand Up @@ -1226,7 +1223,7 @@ unsafe fn merge(

if invalid_resource_state(merged_state) {
return Err(ResourceUsageCompatibilityError::from_texture(
unsafe { metadata_provider.get(index) },
unsafe { &metadata_provider.get(index) },
TextureSelector {
mips: mip_id..mip_id + 1,
layers: layers.clone(),
Expand Down Expand Up @@ -1262,7 +1259,7 @@ unsafe fn merge(

if invalid_resource_state(merged_state) {
return Err(ResourceUsageCompatibilityError::from_texture(
unsafe { metadata_provider.get(index) },
unsafe { &metadata_provider.get(index) },
TextureSelector {
mips: mip_id..mip_id + 1,
layers: layers.clone(),
Expand Down