Make ObjectId structure and invariants idiomatic (#3347)

CHANGELOG.md

@@ -122,6 +122,7 @@ Additionally `Surface::get_default_config` now returns an Option and returns Non
 - Dereferencing a buffer view is now marked inline. By @Wumpf in [#3307](https://github.com/gfx-rs/wgpu/pull/3307)
 - The `strict_assert` family of macros was moved to `wgpu-types`. By @i509VCB in [#3051](https://github.com/gfx-rs/wgpu/pull/3051)
 - Add missing `DEPTH_BIAS_CLAMP` and `FULL_DRAW_INDEX_UINT32` downlevel flags. By @teoxoy in [#3316](https://github.com/gfx-rs/wgpu/pull/3316)
+- Make `ObjectId` structure and invariants idiomatic. By @teoxoy in [#3347](https://github.com/gfx-rs/wgpu/pull/3347)
 
 #### WebGPU
 

wgpu-core/src/id.rs

@@ -172,7 +172,6 @@ pub(crate) struct Valid<I>(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<T> TypedId for Id<T> {
-    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);

wgpu/Cargo.toml

@@ -80,7 +80,7 @@ name = "stencil-triangles"
 test = true
 
 [features]
-default = ["wgsl", "expose-ids"]
+default = ["wgsl"]
 # Apply run-time checks, even in release builds. These are in addition
 # to the validation carried out at public APIs in all builds.
 strict_asserts = ["wgc?/strict_asserts", "wgt/strict_asserts"]

wgpu/src/backend/direct.rs

@@ -2885,22 +2885,21 @@ impl crate::Context for Context {
 }
 
 impl<T> From<ObjectId> for wgc::id::Id<T> {
-    // 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<T> From<wgc::id::Id<T>> for ObjectId {
-    // If the id32 feature is enabled, this conversion is not useless.
-    #[allow(clippy::useless_conversion)]
     fn from(id: wgc::id::Id<T>) -> 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)
     }
 }
 

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<T>(value: T) -> Identified<T> {
     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<T>(value: T) -> Identified<T> {
 
 impl<T: FromWasmAbi<Abi = u32> + JsCast> From<ObjectId> for Identified<T> {
     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::<T>());
         // 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<T: IntoWasmAbi<Abi = u32>> From<Identified<T>> for ObjectId {
     fn from(id: Identified<T>) -> 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<T> Send for Sendable<T> {}
 unsafe impl<T> Sync for Sendable<T> {}
 
 #[derive(Clone, Debug)]
-pub(crate) struct Identified<T>(T, #[cfg(feature = "expose-ids")] u64);
+pub(crate) struct Identified<T>(T, #[cfg(feature = "expose-ids")] crate::Id);
 unsafe impl<T> Send for Identified<T> {}
 unsafe impl<T> Sync for Identified<T> {}
 

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,47 @@ 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<NonZeroU64>,
+    #[cfg(feature = "expose-ids")]
+    /// ID that is unique at all times
+    global_id: Option<crate::Id>,
+}
 
 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,
+    };
 
-impl From<NonZeroU128> for ObjectId {
-    fn from(raw: NonZeroU128) -> Self {
-        Self(raw)
+    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(global_id),
+        }
     }
-}
 
-impl From<ObjectId> for NonZeroU128 {
-    fn from(id: ObjectId) -> Self {
-        id.0
+    #[allow(dead_code)]
+    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)),
+        }
+    }
+
+    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 +1047,16 @@ fn downcast_mut<T: Debug + Send + Sync + 'static>(data: &mut crate::Data) -> &mu
 #[derive(Debug, Clone, Copy)]
 pub struct Unused;
 
-const UNUSED_SENTINEL: Option<NonZeroU128> = NonZeroU128::new(u128::MAX);
-
 impl From<ObjectId> 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<Unused> for ObjectId {
     fn from(_: Unused) -> Self {
-        ObjectId::from(UNUSED_SENTINEL.expect("This should never panic"))
+        ObjectId::UNUSED
     }
 }
 

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()
     }
 }