Skip to content
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.

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

[Merged by Bors] - improve compile time by type-erasing wgpu structs #5950

Closed
wants to merge 10 commits into from
Closed
Show file tree
Hide file tree
Changes from 6 commits
Commits
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
9 changes: 6 additions & 3 deletions crates/bevy_render/src/render_resource/bind_group.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,12 @@ use crate::{
texture::FallbackImage,
};
use bevy_reflect::Uuid;
use std::{ops::Deref, sync::Arc};
use std::ops::Deref;
use wgpu::BindingResource;

use crate::render_resource::resource_macros::*;
render_resource_wrapper!(ErasedBindGroup, wgpu::BindGroup);

/// A [`BindGroup`] identifier.
#[derive(Copy, Clone, Hash, Eq, PartialEq, Debug)]
pub struct BindGroupId(Uuid);
Expand All @@ -25,7 +28,7 @@ pub struct BindGroupId(Uuid);
#[derive(Clone, Debug)]
pub struct BindGroup {
id: BindGroupId,
value: Arc<wgpu::BindGroup>,
value: ErasedBindGroup,
}

impl BindGroup {
Expand All @@ -40,7 +43,7 @@ impl From<wgpu::BindGroup> for BindGroup {
fn from(value: wgpu::BindGroup) -> Self {
BindGroup {
id: BindGroupId(Uuid::new_v4()),
value: Arc::new(value),
value: ErasedBindGroup::new(value),
}
}
}
Expand Down
9 changes: 6 additions & 3 deletions crates/bevy_render/src/render_resource/bind_group_layout.rs
Original file line number Diff line number Diff line change
@@ -1,13 +1,16 @@
use crate::render_resource::resource_macros::*;
use bevy_reflect::Uuid;
use std::{ops::Deref, sync::Arc};
use std::ops::Deref;

#[derive(Copy, Clone, Hash, Eq, PartialEq, Debug)]
pub struct BindGroupLayoutId(Uuid);

render_resource_wrapper!(ErasedBindGroupLayout, wgpu::BindGroupLayout);

#[derive(Clone, Debug)]
pub struct BindGroupLayout {
id: BindGroupLayoutId,
value: Arc<wgpu::BindGroupLayout>,
value: ErasedBindGroupLayout,
}

impl PartialEq for BindGroupLayout {
Expand All @@ -32,7 +35,7 @@ impl From<wgpu::BindGroupLayout> for BindGroupLayout {
fn from(value: wgpu::BindGroupLayout) -> Self {
BindGroupLayout {
id: BindGroupLayoutId(Uuid::new_v4()),
value: Arc::new(value),
value: ErasedBindGroupLayout::new(value),
}
}
}
Expand Down
13 changes: 7 additions & 6 deletions crates/bevy_render/src/render_resource/buffer.rs
Original file line number Diff line number Diff line change
@@ -1,16 +1,17 @@
use bevy_utils::Uuid;
use std::{
ops::{Bound, Deref, RangeBounds},
sync::Arc,
};
use std::ops::{Bound, Deref, RangeBounds};

use crate::render_resource::resource_macros::*;

#[derive(Copy, Clone, Hash, Eq, PartialEq, Debug)]
pub struct BufferId(Uuid);

render_resource_wrapper!(ErasedBuffer, wgpu::Buffer);

#[derive(Clone, Debug)]
pub struct Buffer {
id: BufferId,
value: Arc<wgpu::Buffer>,
value: ErasedBuffer,
}

impl Buffer {
Expand Down Expand Up @@ -42,7 +43,7 @@ impl From<wgpu::Buffer> for Buffer {
fn from(value: wgpu::Buffer) -> Self {
Buffer {
id: BufferId(Uuid::new_v4()),
value: Arc::new(value),
value: ErasedBuffer::new(value),
}
}
}
Expand Down
1 change: 1 addition & 0 deletions crates/bevy_render/src/render_resource/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ mod buffer_vec;
mod pipeline;
mod pipeline_cache;
mod pipeline_specializer;
pub mod resource_macros;
mod shader;
mod storage_buffer;
mod texture;
Expand Down
16 changes: 11 additions & 5 deletions crates/bevy_render/src/render_resource/pipeline.rs
Original file line number Diff line number Diff line change
@@ -1,24 +1,28 @@
use crate::render_resource::{BindGroupLayout, Shader};
use bevy_asset::Handle;
use bevy_reflect::Uuid;
use std::{borrow::Cow, ops::Deref, sync::Arc};
use std::{borrow::Cow, ops::Deref};
use wgpu::{
BufferAddress, ColorTargetState, DepthStencilState, MultisampleState, PrimitiveState,
VertexAttribute, VertexFormat, VertexStepMode,
};

use crate::render_resource::resource_macros::*;

/// A [`RenderPipeline`] identifier.
#[derive(Copy, Clone, Hash, Eq, PartialEq, Debug)]
pub struct RenderPipelineId(Uuid);

render_resource_wrapper!(ErasedRenderPipeline, wgpu::RenderPipeline);

/// A [`RenderPipeline`] represents a graphics pipeline and its stages (shaders), bindings and vertex buffers.
///
/// May be converted from and dereferences to a wgpu [`RenderPipeline`](wgpu::RenderPipeline).
/// Can be created via [`RenderDevice::create_render_pipeline`](crate::renderer::RenderDevice::create_render_pipeline).
#[derive(Clone, Debug)]
pub struct RenderPipeline {
id: RenderPipelineId,
value: Arc<wgpu::RenderPipeline>,
value: ErasedRenderPipeline,
}

impl RenderPipeline {
Expand All @@ -32,7 +36,7 @@ impl From<wgpu::RenderPipeline> for RenderPipeline {
fn from(value: wgpu::RenderPipeline) -> Self {
RenderPipeline {
id: RenderPipelineId(Uuid::new_v4()),
value: Arc::new(value),
value: ErasedRenderPipeline::new(value),
}
}
}
Expand All @@ -50,14 +54,16 @@ impl Deref for RenderPipeline {
#[derive(Copy, Clone, Hash, Eq, PartialEq, Debug)]
pub struct ComputePipelineId(Uuid);

render_resource_wrapper!(ErasedComputePipeline, wgpu::ComputePipeline);

/// A [`ComputePipeline`] represents a compute pipeline and its single shader stage.
///
/// May be converted from and dereferences to a wgpu [`ComputePipeline`](wgpu::ComputePipeline).
/// Can be created via [`RenderDevice::create_compute_pipeline`](crate::renderer::RenderDevice::create_compute_pipeline).
#[derive(Clone, Debug)]
pub struct ComputePipeline {
id: ComputePipelineId,
value: Arc<wgpu::ComputePipeline>,
value: ErasedComputePipeline,
}

impl ComputePipeline {
Expand All @@ -72,7 +78,7 @@ impl From<wgpu::ComputePipeline> for ComputePipeline {
fn from(value: wgpu::ComputePipeline) -> Self {
ComputePipeline {
id: ComputePipelineId(Uuid::new_v4()),
value: Arc::new(value),
value: ErasedComputePipeline::new(value),
}
}
}
Expand Down
28 changes: 17 additions & 11 deletions crates/bevy_render/src/render_resource/pipeline_cache.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,17 @@ use bevy_utils::{
tracing::{debug, error},
Entry, HashMap, HashSet,
};
use std::{hash::Hash, iter::FusedIterator, mem, ops::Deref, sync::Arc};
use std::{hash::Hash, iter::FusedIterator, mem, ops::Deref};
use thiserror::Error;
use wgpu::{
BufferBindingType, PipelineLayoutDescriptor, ShaderModule,
VertexBufferLayout as RawVertexBufferLayout,
BufferBindingType, PipelineLayoutDescriptor, VertexBufferLayout as RawVertexBufferLayout,
};

use crate::render_resource::resource_macros::*;

render_resource_wrapper!(ErasedShaderModule, wgpu::ShaderModule);
render_resource_wrapper!(ErasedPipelineLayout, wgpu::PipelineLayout);

/// A descriptor for a [`Pipeline`].
///
/// Used to store an heterogenous collection of render and compute pipeline descriptors together.
Expand Down Expand Up @@ -103,7 +107,7 @@ impl CachedPipelineState {
#[derive(Default)]
struct ShaderData {
pipelines: HashSet<CachedPipelineId>,
processed_shaders: HashMap<Vec<String>, Arc<ShaderModule>>,
processed_shaders: HashMap<Vec<String>, ErasedShaderModule>,
resolved_imports: HashMap<ShaderImport, Handle<Shader>>,
dependents: HashSet<Handle<Shader>>,
}
Expand All @@ -124,7 +128,7 @@ impl ShaderCache {
pipeline: CachedPipelineId,
handle: &Handle<Shader>,
shader_defs: &[String],
) -> Result<Arc<ShaderModule>, PipelineCacheError> {
) -> Result<ErasedShaderModule, PipelineCacheError> {
let shader = self
.shaders
.get(handle)
Expand Down Expand Up @@ -201,7 +205,7 @@ impl ShaderCache {
return Err(PipelineCacheError::CreateShaderModule(description));
}

entry.insert(Arc::new(shader_module))
entry.insert(ErasedShaderModule::new(shader_module))
}
};

Expand Down Expand Up @@ -273,7 +277,7 @@ impl ShaderCache {

#[derive(Default)]
struct LayoutCache {
layouts: HashMap<Vec<BindGroupLayoutId>, wgpu::PipelineLayout>,
layouts: HashMap<Vec<BindGroupLayoutId>, ErasedPipelineLayout>,
}

impl LayoutCache {
Expand All @@ -288,10 +292,12 @@ impl LayoutCache {
.iter()
.map(|l| l.value())
.collect::<Vec<_>>();
render_device.create_pipeline_layout(&PipelineLayoutDescriptor {
bind_group_layouts: &bind_group_layouts,
..default()
})
ErasedPipelineLayout::new(render_device.create_pipeline_layout(
&PipelineLayoutDescriptor {
bind_group_layouts: &bind_group_layouts,
..default()
},
))
})
}
}
Expand Down
102 changes: 102 additions & 0 deletions crates/bevy_render/src/render_resource/resource_macros.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,102 @@
#[cfg(debug_assertions)]
#[macro_export]
macro_rules! render_resource_wrapper {
robtfm marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think its worth adding some documentation for the rationale behind this wrapper (and the criteria for removing it in the future?)

($wrapper_type:ident, $wgpu_type:ty) => {
#[derive(Clone, Debug)]
pub struct $wrapper_type(Option<std::sync::Arc<Box<()>>>);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm confused by the Option here. Why is this needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is so that we can take it in try_unwrap without having to do anything special for the Drop impl to work.

If we don't use an option, we have to std::mem::forget and/or ManuallyDrop<> (on self or self.0 or both..?), or std::mem::replace(self.0) (which creates a new Arc<Box<()>> just to drop it again in Drop).

I can do one of those if you prefer, I will need to dig around to get comfortable with it, but since this is #[cfg(debug_assertions)] code perhaps it's not necessary?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok that makes sense to me. We could solve the try_unwrap problem with another layer of wrapper types (so we can implement drop on the internal type, making it possible to move out on try_unwrap()). But implementing drop for that internal type is still problematic.


impl $wrapper_type {
pub fn new(value: $wgpu_type) -> Self {
Copy link
Contributor

@coreh coreh Nov 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If $wgpu_type is !Send or !Sync, won't this (and other fn's that do mem::transmute) produce potentially unsound results, since () is Send and Sync?

Not sure if it would work, but perhaps something like:

Suggested change
pub fn new(value: $wgpu_type) -> Self {
pub fn new(value: $wgpu_type) -> Self where $wgpu_type: Send + Sync {

Could avoid that someone inadvertently uses this for something that isn't Send + Sync down the line?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch, thanks!

unsafe {
Self(Some(std::sync::Arc::new(std::mem::transmute(Box::new(
value,
)))))
}
}

pub fn try_unwrap(mut self) -> Option<$wgpu_type> {
let inner = self.0.take();
if let Some(inner) = inner {
match std::sync::Arc::try_unwrap(inner) {
Ok(untyped_box) => {
let typed_box = unsafe {
std::mem::transmute::<Box<()>, Box<$wgpu_type>>(untyped_box)
};
Some(*typed_box)
}
Err(inner) => {
let _ = unsafe {
std::mem::transmute::<
std::sync::Arc<Box<()>>,
std::sync::Arc<Box<$wgpu_type>>,
>(inner)
};
None
}
}
} else {
None
}
}
}

impl std::ops::Deref for $wrapper_type {
type Target = $wgpu_type;

fn deref(&self) -> &Self::Target {
let untyped_box = self
.0
.as_ref()
.expect("BlackBox inner value has already been taken (via drop or try_unwrap")
.as_ref();

let typed_box =
unsafe { std::mem::transmute::<&Box<()>, &Box<$wgpu_type>>(untyped_box) };
typed_box.as_ref()
}
}

impl Drop for $wrapper_type {
fn drop(&mut self) {
let inner = self.0.take();
if let Some(inner) = inner {
let _ = unsafe {
std::mem::transmute::<
Copy link
Member

@james7132 james7132 Jan 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is potentially UB. std::sync::Arc is repr(Rust), so we can't be sure that this transmute is sound.

Same thing holds true for the other transmutes in this file.

std::sync::Arc<Box<()>>,
std::sync::Arc<Box<$wgpu_type>>,
>(inner)
};
}
}
}
};
}

#[cfg(not(debug_assertions))]
#[macro_export]
macro_rules! render_resource_wrapper {
($wrapper_type:ident, $wgpu_type:ty) => {
#[derive(Clone, Debug)]
pub struct $wrapper_type(std::sync::Arc<$wgpu_type>);

impl $wrapper_type {
pub fn new(value: $wgpu_type) -> Self {
Self(std::sync::Arc::new(value))
}

pub fn try_unwrap(self) -> Option<$wgpu_type> {
std::sync::Arc::try_unwrap(self.0).ok()
}
}

impl std::ops::Deref for $wrapper_type {
type Target = $wgpu_type;

fn deref(&self) -> &Self::Target {
self.0.as_ref()
}
}
};
}

pub use render_resource_wrapper;
Loading