diff --git a/wgpu-core/src/any_surface.rs b/wgpu-core/src/any_surface.rs index 48d4b1d45a..cb6f9eb570 100644 --- a/wgpu-core/src/any_surface.rs +++ b/wgpu-core/src/any_surface.rs @@ -1,92 +1,93 @@ use wgt::Backend; -/// The `AnySurface` type: a `Arc` of a `HalSurface` for any backend `A`. +/// The `AnySurface` type: a `Arc` of a `A::Surface` for any backend `A`. use crate::hal_api::HalApi; -use crate::instance::HalSurface; -use std::any::Any; use std::fmt; -use std::sync::Arc; +use std::mem::ManuallyDrop; +use std::ptr::NonNull; -/// A `Arc` of a `HalSurface`, for any backend `A`. +struct AnySurfaceVtable { + // We oppurtunistically store the backend here, since we now it will be used + // with backend selection and it can be stored in static memory. + backend: Backend, + // Drop glue which knows how to drop the stored data. + drop: unsafe fn(*mut ()), +} + +/// An `A::Surface`, for any backend `A`. /// -/// Any `AnySurface` is just like an `Arc>`, except that the -/// `A` type parameter is erased. To access the `Surface`, you must -/// downcast to a particular backend with the \[`downcast_ref`\] or -/// \[`take`\] methods. -pub struct AnySurface(Arc); +/// Any `AnySurface` is just like an `A::Surface`, except that the `A` type +/// parameter is erased. To access the `Surface`, you must downcast to a +/// particular backend with the \[`downcast_ref`\] or \[`take`\] methods. +pub struct AnySurface { + data: NonNull<()>, + vtable: &'static AnySurfaceVtable, +} impl AnySurface { - /// Return an `AnySurface` that holds an owning `Arc` to `HalSurface`. - pub fn new(surface: HalSurface) -> AnySurface { - AnySurface(Arc::new(surface)) + /// Construct an `AnySurface` that owns an `A::Surface`. + pub fn new(surface: A::Surface) -> AnySurface { + unsafe fn drop_glue(ptr: *mut ()) { + unsafe { + _ = Box::from_raw(ptr.cast::()); + } + } + + let data = NonNull::from(Box::leak(Box::new(surface))); + + AnySurface { + data: data.cast(), + vtable: &AnySurfaceVtable { + backend: A::VARIANT, + drop: drop_glue::, + }, + } } + /// Get the backend this surface was created through. pub fn backend(&self) -> Backend { - #[cfg(vulkan)] - if self.downcast_ref::().is_some() { - return Backend::Vulkan; - } - #[cfg(metal)] - if self.downcast_ref::().is_some() { - return Backend::Metal; - } - #[cfg(dx12)] - if self.downcast_ref::().is_some() { - return Backend::Dx12; - } - #[cfg(gles)] - if self.downcast_ref::().is_some() { - return Backend::Gl; - } - Backend::Empty + self.vtable.backend } - /// If `self` is an `Arc>`, return a reference to the - /// HalSurface. - pub fn downcast_ref(&self) -> Option<&HalSurface> { - self.0.downcast_ref::>() + /// If `self` refers to an `A::Surface`, returns a reference to it. + pub fn downcast_ref(&self) -> Option<&A::Surface> { + if A::VARIANT != self.vtable.backend { + return None; + } + + // SAFETY: We just checked the instance above implicitly by the backend + // that it was statically constructed through. + Some(unsafe { &*self.data.as_ptr().cast::() }) } - /// If `self` is an `Arc>`, returns that. - pub fn take(self) -> Option>> { - // `Arc::downcast` returns `Arc`, but requires that `T` be `Sync` and - // `Send`, and this is not the case for `HalSurface` in wasm builds. - // - // But as far as I can see, `Arc::downcast` has no particular reason to - // require that `T` be `Sync` and `Send`; the steps used here are sound. - if (self.0).is::>() { - // Turn the `Arc`, which is a pointer to an `ArcInner` struct, into - // a pointer to the `ArcInner`'s `data` field. Carry along the - // vtable from the original `Arc`. - let raw_erased: *const (dyn Any + 'static) = Arc::into_raw(self.0); - // Remove the vtable, and supply the concrete type of the `data`. - let raw_typed: *const HalSurface = raw_erased.cast::>(); - // Convert the pointer to the `data` field back into a pointer to - // the `ArcInner`, and restore reference-counting behavior. - let arc_typed: Arc> = unsafe { - // Safety: - // - We checked that the `dyn Any` was indeed a `HalSurface` above. - // - We're calling `Arc::from_raw` on the same pointer returned - // by `Arc::into_raw`, except that we stripped off the vtable - // pointer. - // - The pointer must still be live, because we've borrowed `self`, - // which holds another reference to it. - // - The format of a `ArcInner` must be the same as - // that of an `ArcInner>`, or else `AnyHalSurface::new` - // wouldn't be possible. - Arc::from_raw(raw_typed) - }; - Some(arc_typed) - } else { - None + /// If `self` is an `Arc`, returns that. + pub fn take(self) -> Option { + if A::VARIANT != self.vtable.backend { + return None; } + + // Disable drop glue, since we're returning the owned surface. The + // caller will be responsible for dropping it. + let this = ManuallyDrop::new(self); + + // SAFETY: We just checked the instance above implicitly by the backend + // that it was statically constructed through. + Some(unsafe { *Box::from_raw(this.data.as_ptr().cast::()) }) + } +} + +impl Drop for AnySurface { + fn drop(&mut self) { + unsafe { (self.vtable.drop)(self.data.as_ptr()) } } } impl fmt::Debug for AnySurface { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - f.write_str("AnySurface") + f.debug_tuple("AnySurface") + .field(&self.vtable.backend) + .finish() } } diff --git a/wgpu-core/src/device/any_device.rs b/wgpu-core/src/device/any_device.rs index b3c4a31839..d08c23cf99 100644 --- a/wgpu-core/src/device/any_device.rs +++ b/wgpu-core/src/device/any_device.rs @@ -1,72 +1,98 @@ +use wgt::Backend; + use super::Device; /// The `AnyDevice` type: a pointer to a `Device` for any backend `A`. use crate::hal_api::HalApi; -use std::any::Any; use std::fmt; +use std::mem::ManuallyDrop; +use std::ptr::NonNull; use std::sync::Arc; +struct AnyDeviceVtable { + // We oppurtunistically store the backend here, since we now it will be used + // with backend selection and it can be stored in static memory. + backend: Backend, + // Drop glue which knows how to drop the stored data. + drop: unsafe fn(*mut ()), +} + /// A pointer to a `Device`, for any backend `A`. /// -/// Any `AnyDevice` is just like an `Arc>`, except that the -/// `A` type parameter is erased. To access the `Device`, you must -/// downcast to a particular backend with the \[`downcast_ref`\] or -/// \[`downcast_clone`\] methods. -pub struct AnyDevice(Arc); +/// Any `AnyDevice` is just like an `Arc>`, except that the `A` type +/// parameter is erased. To access the `Device`, you must downcast to a +/// particular backend with the \[`downcast_ref`\] or \[`downcast_clone`\] +/// methods. +pub struct AnyDevice { + data: NonNull<()>, + vtable: &'static AnyDeviceVtable, +} impl AnyDevice { /// Return an `AnyDevice` that holds an owning `Arc` pointer to `device`. pub fn new(device: Arc>) -> AnyDevice { - AnyDevice(device) + unsafe fn drop_glue(ptr: *mut ()) { + // Drop the arc this instance is holding. + unsafe { + _ = Arc::from_raw(ptr.cast::()); + } + } + + // SAFETY: The pointer returned by Arc::into_raw is gauranteed to be + // non-null. + let data = unsafe { NonNull::new_unchecked(Arc::into_raw(device).cast_mut()) }; + + AnyDevice { + data: data.cast(), + vtable: &AnyDeviceVtable { + backend: A::VARIANT, + drop: drop_glue::, + }, + } } /// If `self` is an `Arc>`, return a reference to the /// device. pub fn downcast_ref(&self) -> Option<&Device> { - self.0.downcast_ref::>() + if self.vtable.backend != A::VARIANT { + return None; + } + + // SAFETY: We just checked the instance above implicitly by the backend + // that it was statically constructed through. + Some(unsafe { &*(self.data.as_ptr().cast::>()) }) } /// If `self` is an `Arc>`, return a clone of that. pub fn downcast_clone(&self) -> Option>> { - // `Arc::downcast` returns `Arc`, but requires that `T` be `Sync` and - // `Send`, and this is not the case for `Device` in wasm builds. - // - // But as far as I can see, `Arc::downcast` has no particular reason to - // require that `T` be `Sync` and `Send`; the steps used here are sound. - if (self.0).is::>() { - // Get an owned Arc. - let clone = self.0.clone(); - // Turn the `Arc`, which is a pointer to an `ArcInner` struct, into - // a pointer to the `ArcInner`'s `data` field. Carry along the - // vtable from the original `Arc`. - let raw_erased: *const (dyn Any + 'static) = Arc::into_raw(clone); - // Remove the vtable, and supply the concrete type of the `data`. - let raw_typed: *const Device = raw_erased.cast::>(); - // Convert the pointer to the `data` field back into a pointer to - // the `ArcInner`, and restore reference-counting behavior. - let arc_typed: Arc> = unsafe { - // Safety: - // - We checked that the `dyn Any` was indeed a `Device` above. - // - We're calling `Arc::from_raw` on the same pointer returned - // by `Arc::into_raw`, except that we stripped off the vtable - // pointer. - // - The pointer must still be live, because we've borrowed `self`, - // which holds another reference to it. - // - The format of a `ArcInner` must be the same as - // that of an `ArcInner>`, or else `AnyDevice::new` - // wouldn't be possible. - Arc::from_raw(raw_typed) - }; - Some(arc_typed) - } else { - None + if self.vtable.backend != A::VARIANT { + return None; } + + // We need to prevent the destructor of the arc from running, since it + // refers to the instance held by this object. Dropping it would + // invalidate this object. + // + // SAFETY: We just checked the instance above implicitly by the backend + // that it was statically constructed through. + let this = + ManuallyDrop::new(unsafe { Arc::from_raw(self.data.as_ptr().cast::>()) }); + + // Cloning it increases the reference count, and we return a new arc + // instance. + Some((*this).clone()) + } +} + +impl Drop for AnyDevice { + fn drop(&mut self) { + unsafe { (self.vtable.drop)(self.data.as_ptr()) } } } impl fmt::Debug for AnyDevice { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - f.write_str("AnyDevice") + write!(f, "AnyDevice") } } diff --git a/wgpu-core/src/device/global.rs b/wgpu-core/src/device/global.rs index cb5438ebba..6b9a6f6469 100644 --- a/wgpu-core/src/device/global.rs +++ b/wgpu-core/src/device/global.rs @@ -1966,11 +1966,7 @@ impl Global { let caps = unsafe { let suf = A::get_surface(surface); let adapter = &device.adapter; - match adapter - .raw - .adapter - .surface_capabilities(suf.unwrap().raw.as_ref()) - { + match adapter.raw.adapter.surface_capabilities(suf.unwrap()) { Some(caps) => caps, None => break E::UnsupportedQueueFamily, } @@ -2055,7 +2051,6 @@ impl Global { match unsafe { A::get_surface(surface) .unwrap() - .raw .configure(device.raw(), &hal_config) } { Ok(()) => (), diff --git a/wgpu-core/src/hal_api.rs b/wgpu-core/src/hal_api.rs index 7de5073791..179024baed 100644 --- a/wgpu-core/src/hal_api.rs +++ b/wgpu-core/src/hal_api.rs @@ -3,7 +3,7 @@ use wgt::{Backend, WasmNotSendSync}; use crate::{ global::Global, hub::Hub, - instance::{HalSurface, Instance, Surface}, + instance::{Instance, Surface}, }; pub trait HalApi: hal::Api + 'static + WasmNotSendSync { @@ -11,7 +11,7 @@ pub trait HalApi: hal::Api + 'static + WasmNotSendSync { fn create_instance_from_hal(name: &str, hal_instance: Self::Instance) -> Instance; fn instance_as_hal(instance: &Instance) -> Option<&Self::Instance>; fn hub(global: &Global) -> &Hub; - fn get_surface(surface: &Surface) -> Option<&HalSurface>; + fn get_surface(surface: &Surface) -> Option<&Self::Surface>; } impl HalApi for hal::api::Empty { @@ -25,7 +25,7 @@ impl HalApi for hal::api::Empty { fn hub(_: &Global) -> &Hub { unimplemented!("called empty api") } - fn get_surface(_: &Surface) -> Option<&HalSurface> { + fn get_surface(_: &Surface) -> Option<&Self::Surface> { unimplemented!("called empty api") } } @@ -46,8 +46,8 @@ impl HalApi for hal::api::Vulkan { fn hub(global: &Global) -> &Hub { &global.hubs.vulkan } - fn get_surface(surface: &Surface) -> Option<&HalSurface> { - surface.raw.downcast_ref() + fn get_surface(surface: &Surface) -> Option<&Self::Surface> { + surface.raw.downcast_ref::() } } @@ -67,8 +67,8 @@ impl HalApi for hal::api::Metal { fn hub(global: &Global) -> &Hub { &global.hubs.metal } - fn get_surface(surface: &Surface) -> Option<&HalSurface> { - surface.raw.downcast_ref() + fn get_surface(surface: &Surface) -> Option<&Self::Surface> { + surface.raw.downcast_ref::() } } @@ -88,8 +88,8 @@ impl HalApi for hal::api::Dx12 { fn hub(global: &Global) -> &Hub { &global.hubs.dx12 } - fn get_surface(surface: &Surface) -> Option<&HalSurface> { - surface.raw.downcast_ref() + fn get_surface(surface: &Surface) -> Option<&Self::Surface> { + surface.raw.downcast_ref::() } } @@ -110,7 +110,7 @@ impl HalApi for hal::api::Gles { fn hub(global: &Global) -> &Hub { &global.hubs.gl } - fn get_surface(surface: &Surface) -> Option<&HalSurface> { - surface.raw.downcast_ref() + fn get_surface(surface: &Surface) -> Option<&Self::Surface> { + surface.raw.downcast_ref::() } } diff --git a/wgpu-core/src/hub.rs b/wgpu-core/src/hub.rs index 7829484aa8..0f4589c8b3 100644 --- a/wgpu-core/src/hub.rs +++ b/wgpu-core/src/hub.rs @@ -109,7 +109,7 @@ use crate::{ command::{CommandBuffer, RenderBundle}, device::{queue::Queue, Device}, hal_api::HalApi, - instance::{Adapter, HalSurface, Surface}, + instance::{Adapter, Surface}, pipeline::{ComputePipeline, RenderPipeline, ShaderModule}, registry::{Registry, RegistryReport}, resource::{Buffer, QuerySet, Sampler, StagingBuffer, Texture, TextureView}, @@ -243,7 +243,7 @@ impl Hub { if let Some(device) = present.device.downcast_ref::() { let suf = A::get_surface(surface); unsafe { - suf.unwrap().raw.unconfigure(device.raw()); + suf.unwrap().unconfigure(device.raw()); //TODO: we could destroy the surface here } } @@ -260,10 +260,10 @@ impl Hub { } } - pub(crate) fn surface_unconfigure(&self, device: &Device, surface: &HalSurface) { + pub(crate) fn surface_unconfigure(&self, device: &Device, surface: &A::Surface) { unsafe { use hal::Surface; - surface.raw.unconfigure(device.raw()); + surface.unconfigure(device.raw()); } } diff --git a/wgpu-core/src/instance.rs b/wgpu-core/src/instance.rs index 5ef090defe..582571c2b8 100644 --- a/wgpu-core/src/instance.rs +++ b/wgpu-core/src/instance.rs @@ -21,11 +21,6 @@ use thiserror::Error; pub type RequestAdapterOptions = wgt::RequestAdapterOptions; type HalInstance = ::Instance; -//TODO: remove this -#[derive(Clone)] -pub struct HalSurface { - pub raw: Arc, -} #[derive(Clone, Debug, Error)] #[error("Limit '{name}' value {requested} is better than allowed {allowed}")] @@ -118,30 +113,22 @@ impl Instance { } pub(crate) fn destroy_surface(&self, surface: Surface) { - fn destroy(_: A, instance: &Option, surface: AnySurface) { + fn destroy(instance: &Option, surface: AnySurface) { unsafe { - if let Some(surface) = surface.take::() { - if let Some(suf) = Arc::into_inner(surface) { - if let Some(raw) = Arc::into_inner(suf.raw) { - instance.as_ref().unwrap().destroy_surface(raw); - } else { - panic!("Surface cannot be destroyed because is still in use"); - } - } else { - panic!("Surface cannot be destroyed because is still in use"); - } + if let Some(suf) = surface.take::() { + instance.as_ref().unwrap().destroy_surface(suf); } } } match surface.raw.backend() { #[cfg(vulkan)] - Backend::Vulkan => destroy(hal::api::Vulkan, &self.vulkan, surface.raw), + Backend::Vulkan => destroy::(&self.vulkan, surface.raw), #[cfg(metal)] - Backend::Metal => destroy(hal::api::Metal, &self.metal, surface.raw), + Backend::Metal => destroy::(&self.metal, surface.raw), #[cfg(dx12)] - Backend::Dx12 => destroy(hal::api::Dx12, &self.dx12, surface.raw), + Backend::Dx12 => destroy::(&self.dx12, surface.raw), #[cfg(gles)] - Backend::Gl => destroy(hal::api::Gles, &self.gl, surface.raw), + Backend::Gl => destroy::(&self.gl, surface.raw), _ => unreachable!(), } } @@ -182,7 +169,7 @@ impl Surface { adapter .raw .adapter - .surface_capabilities(&suf.raw) + .surface_capabilities(suf) .ok_or(GetSurfaceSupportError::Unsupported)? }; @@ -223,7 +210,7 @@ impl Adapter { // This could occur if the user is running their app on Wayland but Vulkan does not support // VK_KHR_wayland_surface. match suf { - Some(suf) => unsafe { self.raw.adapter.surface_capabilities(&suf.raw) }.is_some(), + Some(suf) => unsafe { self.raw.adapter.surface_capabilities(suf) }.is_some(), None => false, } } @@ -502,7 +489,7 @@ impl Global { ) -> Option> { inst.as_ref().map(|inst| unsafe { match inst.create_surface(display_handle, window_handle) { - Ok(raw) => Ok(AnySurface::new(HalSurface:: { raw: Arc::new(raw) })), + Ok(raw) => Ok(AnySurface::new::(raw)), Err(e) => Err(e), } }) @@ -557,19 +544,17 @@ impl Global { presentation: Mutex::new(None), info: ResourceInfo::new(""), raw: { - let hal_surface: HalSurface = self + let hal_surface = self .instance .metal .as_ref() - .map(|inst| HalSurface { - raw: Arc::new( - // we don't want to link to metal-rs for this - #[allow(clippy::transmute_ptr_to_ref)] - inst.create_surface_from_layer(unsafe { std::mem::transmute(layer) }), - ), //acquired_texture: None, + .map(|inst| { + // we don't want to link to metal-rs for this + #[allow(clippy::transmute_ptr_to_ref)] + inst.create_surface_from_layer(unsafe { std::mem::transmute(layer) }) }) .unwrap(); - AnySurface::new(hal_surface) + AnySurface::new::(hal_surface) }, }; @@ -592,15 +577,13 @@ impl Global { presentation: Mutex::new(None), info: ResourceInfo::new(""), raw: { - let hal_surface: HalSurface = self + let hal_surface = self .instance .dx12 .as_ref() - .map(|inst| HalSurface { - raw: Arc::new(unsafe { inst.create_surface_from_visual(visual as _) }), - }) + .map(|inst| unsafe { inst.create_surface_from_visual(visual as _) }) .unwrap(); - AnySurface::new(hal_surface) + AnySurface::new::(hal_surface) }, }; @@ -623,17 +606,13 @@ impl Global { presentation: Mutex::new(None), info: ResourceInfo::new(""), raw: { - let hal_surface: HalSurface = self + let hal_surface = self .instance .dx12 .as_ref() - .map(|inst| HalSurface { - raw: Arc::new(unsafe { - inst.create_surface_from_surface_handle(surface_handle) - }), - }) + .map(|inst| unsafe { inst.create_surface_from_surface_handle(surface_handle) }) .unwrap(); - AnySurface::new(hal_surface) + AnySurface::new::(hal_surface) }, }; @@ -656,17 +635,15 @@ impl Global { presentation: Mutex::new(None), info: ResourceInfo::new(""), raw: { - let hal_surface: HalSurface = self + let hal_surface = self .instance .dx12 .as_ref() - .map(|inst| HalSurface { - raw: Arc::new(unsafe { - inst.create_surface_from_swap_chain_panel(swap_chain_panel as _) - }), + .map(|inst| unsafe { + inst.create_surface_from_swap_chain_panel(swap_chain_panel as _) }) .unwrap(); - AnySurface::new(hal_surface) + AnySurface::new::(hal_surface) }, }; @@ -814,7 +791,7 @@ impl Global { surface.is_some() && exposed .adapter - .surface_capabilities(&surface.unwrap().raw) + .surface_capabilities(surface.unwrap()) .is_some() }); } diff --git a/wgpu-core/src/present.rs b/wgpu-core/src/present.rs index 2452825aea..4d8e1df73e 100644 --- a/wgpu-core/src/present.rs +++ b/wgpu-core/src/present.rs @@ -163,7 +163,6 @@ impl Global { let suf = A::get_surface(surface.as_ref()); let (texture_id, status) = match unsafe { suf.unwrap() - .raw .acquire_texture(Some(std::time::Duration::from_millis( FRAME_TIMEOUT_MS as u64, ))) @@ -339,7 +338,7 @@ impl Global { Err(hal::SurfaceError::Lost) } else if !has_work.load(Ordering::Relaxed) { log::error!("No work has been submitted for this frame"); - unsafe { suf.unwrap().raw.discard_texture(raw.take().unwrap()) }; + unsafe { suf.unwrap().discard_texture(raw.take().unwrap()) }; Err(hal::SurfaceError::Outdated) } else { unsafe { @@ -347,7 +346,7 @@ impl Global { .raw .as_ref() .unwrap() - .present(&suf.unwrap().raw, raw.take().unwrap()) + .present(suf.unwrap(), raw.take().unwrap()) } } } @@ -427,7 +426,7 @@ impl Global { has_work: _, } => { if surface_id == parent_id { - unsafe { suf.unwrap().raw.discard_texture(raw.take().unwrap()) }; + unsafe { suf.unwrap().discard_texture(raw.take().unwrap()) }; } else { log::warn!("Surface texture is outdated"); } diff --git a/wgpu-core/src/resource.rs b/wgpu-core/src/resource.rs index 07af19e14f..cc3ac2780d 100644 --- a/wgpu-core/src/resource.rs +++ b/wgpu-core/src/resource.rs @@ -1007,10 +1007,7 @@ impl Global { profiling::scope!("Surface::as_hal"); let surface = self.surfaces.get(id).ok(); - let hal_surface = surface - .as_ref() - .and_then(|surface| A::get_surface(surface)) - .map(|surface| &*surface.raw); + let hal_surface = surface.as_ref().and_then(|surface| A::get_surface(surface)); hal_surface_callback(hal_surface) }