From a11a86581615f67bc8fe46dfe6a6094f3020e9e5 Mon Sep 17 00:00:00 2001 From: teoxoy <28601907+teoxoy@users.noreply.github.com> Date: Wed, 4 Jan 2023 15:48:53 +0100 Subject: [PATCH] make `ObjectId` structure and invariants idiomatic --- wgpu-core/src/id.rs | 5 ---- wgpu/src/backend/direct.rs | 17 ++++++----- wgpu/src/backend/web.rs | 31 +++++++++++--------- wgpu/src/context.rs | 59 ++++++++++++++++++++++++-------------- wgpu/src/lib.rs | 34 +++++++++++----------- 5 files changed, 80 insertions(+), 66 deletions(-) diff --git a/wgpu-core/src/id.rs b/wgpu-core/src/id.rs index a326e08307d..9c654e3ca9b 100644 --- a/wgpu-core/src/id.rs +++ b/wgpu-core/src/id.rs @@ -172,7 +172,6 @@ pub(crate) struct Valid(pub I); /// need to construct `Id` values directly, or access their components, like the /// WGPU recording player, may use this trait to do so. pub trait TypedId: Copy { - fn as_raw(&self) -> NonZeroId; fn zip(index: Index, epoch: Epoch, backend: Backend) -> Self; fn unzip(self) -> (Index, Epoch, Backend); fn into_raw(self) -> NonZeroId; @@ -180,10 +179,6 @@ pub trait TypedId: Copy { #[allow(trivial_numeric_casts)] impl TypedId for Id { - fn as_raw(&self) -> NonZeroId { - self.0 - } - fn zip(index: Index, epoch: Epoch, backend: Backend) -> Self { assert_eq!(0, epoch >> EPOCH_BITS); assert_eq!(0, (index as IdType) >> INDEX_BITS); diff --git a/wgpu/src/backend/direct.rs b/wgpu/src/backend/direct.rs index d9632843d41..82613084b33 100644 --- a/wgpu/src/backend/direct.rs +++ b/wgpu/src/backend/direct.rs @@ -2885,22 +2885,21 @@ impl crate::Context for Context { } impl From for wgc::id::Id { - // If the id32 feature is enabled, this conversion is not useless. - #[allow(clippy::useless_conversion)] fn from(id: ObjectId) -> Self { - let raw = std::num::NonZeroU128::from(id); - // If the id32 feature is enabled, this will truncate the id to a NonZeroU32. - let id = raw.try_into().expect("Id exceeded 32-bits"); - // FIXME: This is not safe + // If the id32 feature is enabled in wgpu-core, this will make sure that the id fits in a NonZeroU32. + #[allow(clippy::useless_conversion)] + let id = id.id().try_into().expect("Id exceeded 32-bits"); + // SAFETY: The id was created via the impl below unsafe { Self::from_raw(id) } } } impl From> for ObjectId { - // If the id32 feature is enabled, this conversion is not useless. - #[allow(clippy::useless_conversion)] fn from(id: wgc::id::Id) -> Self { - ObjectId::from(std::num::NonZeroU128::from(id.as_raw())) + // If the id32 feature is enabled in wgpu-core, the conversion is not useless + #[allow(clippy::useless_conversion)] + let id = id.into_raw().into(); + Self::from_global_id(id) } } diff --git a/wgpu/src/backend/web.rs b/wgpu/src/backend/web.rs index 7a35437be94..9a3725d4c44 100644 --- a/wgpu/src/backend/web.rs +++ b/wgpu/src/backend/web.rs @@ -6,7 +6,6 @@ use std::{ cell::RefCell, fmt, future::Future, - num::NonZeroU128, ops::Range, pin::Pin, rc::Rc, @@ -26,10 +25,11 @@ use crate::{ fn create_identified(value: T) -> Identified { cfg_if::cfg_if! { if #[cfg(feature = "expose-ids")] { - static NEXT_ID: std::sync::atomic::AtomicU64 = std::sync::atomic::AtomicU64::new(0); - Identified(value, NEXT_ID.fetch_add(1, std::sync::atomic::Ordering::Relaxed)) + static NEXT_ID: std::sync::atomic::AtomicU64 = std::sync::atomic::AtomicU64::new(1); + let id = NEXT_ID.fetch_add(1, std::sync::atomic::Ordering::Relaxed); + Identified(value, crate::Id(core::num::NonZeroU64::new(id).unwrap())) } else { - Identified(value, 0) + Identified(value) } } } @@ -44,25 +44,30 @@ fn create_identified(value: T) -> Identified { impl + JsCast> From for Identified { fn from(id: ObjectId) -> Self { - let raw = std::num::NonZeroU128::from(id); - let global_id = (raw.get() >> 64) as u64; - + let id = id.id().get() as u32; // SAFETY: wasm_bindgen says an ABI representation may only be cast to a wrapper type if it was created // using into_abi. // // This assumption we sadly have to assume to prevent littering the code with unsafe blocks. - let wasm = unsafe { JsValue::from_abi(raw.get() as u32) }; + let wasm = unsafe { JsValue::from_abi(id) }; wgt::strict_assert!(wasm.is_instance_of::()); // SAFETY: The ABI of the type must be a u32, and strict asserts ensure the right type is used. - Self(wasm.unchecked_into(), global_id) + Self( + wasm.unchecked_into(), + #[cfg(feature = "expose-ids")] + id.global_id(), + ) } } impl> From> for ObjectId { fn from(id: Identified) -> Self { - let abi = id.0.into_abi(); - let id = abi as u128 | ((id.1 as u128) << 64); - Self::from(NonZeroU128::new(id).unwrap()) + let id = core::num::NonZeroU64::new(id.0.into_abi() as u64).unwrap(); + Self::new( + id, + #[cfg(feature = "expose-ids")] + id.1, + ) } } @@ -72,7 +77,7 @@ unsafe impl Send for Sendable {} unsafe impl Sync for Sendable {} #[derive(Clone, Debug)] -pub(crate) struct Identified(T, #[cfg(feature = "expose-ids")] u64); +pub(crate) struct Identified(T, #[cfg(feature = "expose-ids")] crate::Id); unsafe impl Send for Identified {} unsafe impl Sync for Identified {} diff --git a/wgpu/src/context.rs b/wgpu/src/context.rs index 410ed2c0b77..2bc4b4faeab 100644 --- a/wgpu/src/context.rs +++ b/wgpu/src/context.rs @@ -1,6 +1,4 @@ -use std::{ - any::Any, fmt::Debug, future::Future, num::NonZeroU128, ops::Range, pin::Pin, sync::Arc, -}; +use std::{any::Any, fmt::Debug, future::Future, num::NonZeroU64, ops::Range, pin::Pin, sync::Arc}; use wgt::{ strict_assert, strict_assert_eq, AdapterInfo, BufferAddress, BufferSize, Color, @@ -985,27 +983,46 @@ pub trait Context: Debug + Send + Sized + Sync { } /// Object id. -/// -/// An ObjectId is a 128-bit number internally, where the first 64-bits represent a backend internal id and -/// the last 64-bits are a global id. -#[derive(Debug, Clone, Copy)] -pub struct ObjectId(NonZeroU128); +#[derive(Debug, Copy, Clone, PartialEq, Eq)] +pub struct ObjectId { + /// ID that is unique at any given time + id: Option, + #[cfg(feature = "expose-ids")] + /// ID that is unique at all times + global_id: Option, +} impl ObjectId { - pub fn global_id(&self) -> u64 { - (self.0.get() >> 64) as u64 + const UNUSED: Self = ObjectId { + id: None, + #[cfg(feature = "expose-ids")] + global_id: None, + }; + + pub fn new(id: NonZeroU64, #[cfg(feature = "expose-ids")] global_id: crate::Id) -> Self { + Self { + id: Some(id), + #[cfg(feature = "expose-ids")] + global_id: Some(id), + } } -} -impl From for ObjectId { - fn from(raw: NonZeroU128) -> Self { - Self(raw) + pub fn from_global_id(global_id: NonZeroU64) -> Self { + Self { + id: Some(global_id), + #[cfg(feature = "expose-ids")] + global_id: Some(crate::Id(global_id)), + } } -} -impl From for NonZeroU128 { - fn from(id: ObjectId) -> Self { - id.0 + pub fn id(&self) -> NonZeroU64 { + self.id.unwrap() + } + + #[cfg(feature = "expose-ids")] + #[cfg_attr(docsrs, doc(cfg(feature = "expose-ids")))] + pub fn global_id(&self) -> crate::Id { + self.global_id.unwrap() } } @@ -1029,18 +1046,16 @@ fn downcast_mut(data: &mut crate::Data) -> &mu #[derive(Debug, Clone, Copy)] pub struct Unused; -const UNUSED_SENTINEL: Option = NonZeroU128::new(u128::MAX); - impl From for Unused { fn from(id: ObjectId) -> Self { - strict_assert_eq!(Some(NonZeroU128::from(id)), UNUSED_SENTINEL); + strict_assert_eq!(id, ObjectId::UNUSED); Self } } impl From for ObjectId { fn from(_: Unused) -> Self { - ObjectId::from(UNUSED_SENTINEL.expect("This should never panic")) + ObjectId::UNUSED } } diff --git a/wgpu/src/lib.rs b/wgpu/src/lib.rs index 809b27e9d9c..0334e1a9335 100644 --- a/wgpu/src/lib.rs +++ b/wgpu/src/lib.rs @@ -4027,7 +4027,7 @@ impl Surface { #[cfg_attr(docsrs, doc(cfg(feature = "expose-ids")))] #[repr(transparent)] #[derive(Debug, Copy, Clone, PartialEq, Eq, Hash)] -pub struct Id(u64); +pub struct Id(core::num::NonZeroU64); #[cfg(feature = "expose-ids")] impl Adapter { @@ -4037,7 +4037,7 @@ impl Adapter { /// The returned value is guaranteed to be different for all resources created from the same `Instance`. #[cfg_attr(docsrs, doc(cfg(feature = "expose-ids")))] pub fn global_id(&self) -> Id { - Id(self.id.global_id()) + self.id.global_id() } } @@ -4049,7 +4049,7 @@ impl Device { /// The returned value is guaranteed to be different for all resources created from the same `Instance`. #[cfg_attr(docsrs, doc(cfg(feature = "expose-ids")))] pub fn global_id(&self) -> Id { - Id(self.id.global_id()) + self.id.global_id() } } @@ -4061,7 +4061,7 @@ impl Queue { /// The returned value is guaranteed to be different for all resources created from the same `Instance`. #[cfg_attr(docsrs, doc(cfg(feature = "expose-ids")))] pub fn global_id(&self) -> Id { - Id(self.id.global_id()) + self.id.global_id() } } @@ -4073,7 +4073,7 @@ impl ShaderModule { /// The returned value is guaranteed to be different for all resources created from the same `Instance`. #[cfg_attr(docsrs, doc(cfg(feature = "expose-ids")))] pub fn global_id(&self) -> Id { - Id(self.id.global_id()) + self.id.global_id() } } @@ -4085,7 +4085,7 @@ impl BindGroupLayout { /// The returned value is guaranteed to be different for all resources created from the same `Instance`. #[cfg_attr(docsrs, doc(cfg(feature = "expose-ids")))] pub fn global_id(&self) -> Id { - Id(self.id.global_id()) + self.id.global_id() } } @@ -4097,7 +4097,7 @@ impl BindGroup { /// The returned value is guaranteed to be different for all resources created from the same `Instance`. #[cfg_attr(docsrs, doc(cfg(feature = "expose-ids")))] pub fn global_id(&self) -> Id { - Id(self.id.global_id()) + self.id.global_id() } } @@ -4109,7 +4109,7 @@ impl TextureView { /// The returned value is guaranteed to be different for all resources created from the same `Instance`. #[cfg_attr(docsrs, doc(cfg(feature = "expose-ids")))] pub fn global_id(&self) -> Id { - Id(self.id.global_id()) + self.id.global_id() } } @@ -4121,7 +4121,7 @@ impl Sampler { /// The returned value is guaranteed to be different for all resources created from the same `Instance`. #[cfg_attr(docsrs, doc(cfg(feature = "expose-ids")))] pub fn global_id(&self) -> Id { - Id(self.id.global_id()) + self.id.global_id() } } @@ -4133,7 +4133,7 @@ impl Buffer { /// The returned value is guaranteed to be different for all resources created from the same `Instance`. #[cfg_attr(docsrs, doc(cfg(feature = "expose-ids")))] pub fn global_id(&self) -> Id { - Id(self.id.global_id()) + self.id.global_id() } } @@ -4145,7 +4145,7 @@ impl Texture { /// The returned value is guaranteed to be different for all resources created from the same `Instance`. #[cfg_attr(docsrs, doc(cfg(feature = "expose-ids")))] pub fn global_id(&self) -> Id { - Id(self.id.global_id()) + self.id.global_id() } } @@ -4157,7 +4157,7 @@ impl QuerySet { /// The returned value is guaranteed to be different for all resources created from the same `Instance`. #[cfg_attr(docsrs, doc(cfg(feature = "expose-ids")))] pub fn global_id(&self) -> Id { - Id(self.id.global_id()) + self.id.global_id() } } @@ -4169,7 +4169,7 @@ impl PipelineLayout { /// The returned value is guaranteed to be different for all resources created from the same `Instance`. #[cfg_attr(docsrs, doc(cfg(feature = "expose-ids")))] pub fn global_id(&self) -> Id { - Id(self.id.global_id()) + self.id.global_id() } } @@ -4181,7 +4181,7 @@ impl RenderPipeline { /// The returned value is guaranteed to be different for all resources created from the same `Instance`. #[cfg_attr(docsrs, doc(cfg(feature = "expose-ids")))] pub fn global_id(&self) -> Id { - Id(self.id.global_id()) + self.id.global_id() } } @@ -4193,7 +4193,7 @@ impl ComputePipeline { /// The returned value is guaranteed to be different for all resources created from the same `Instance`. #[cfg_attr(docsrs, doc(cfg(feature = "expose-ids")))] pub fn global_id(&self) -> Id { - Id(self.id.global_id()) + self.id.global_id() } } @@ -4205,7 +4205,7 @@ impl RenderBundle { /// The returned value is guaranteed to be different for all resources created from the same `Instance`. #[cfg_attr(docsrs, doc(cfg(feature = "expose-ids")))] pub fn global_id(&self) -> Id { - Id(self.id.global_id()) + self.id.global_id() } } @@ -4217,7 +4217,7 @@ impl Surface { /// The returned value is guaranteed to be different for all resources created from the same `Instance`. #[cfg_attr(docsrs, doc(cfg(feature = "expose-ids")))] pub fn global_id(&self) -> Id { - Id(self.id.global_id()) + self.id.global_id() } }