Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

Looks like CI / build-wasm-atomics doesn't like this change.

Here's one of many similar errors:

error[E0277]: `(dyn std::any::Any + 'static)` cannot be shared between threads safely
   --> crates/bevy_render/src/view/window/mod.rs:51:40
    |
51  |             render_app.init_resource::<ScreenshotToScreenPipeline>();
    |                        -------------   ^^^^^^^^^^^^^^^^^^^^^^^^^^ `(dyn std::any::Any + 'static)` cannot be shared between threads safely
    |                        |
    |                        required by a bound introduced by this call
    |
    = help: the trait `Sync` is not implemented for `(dyn std::any::Any + 'static)`, which is required by `ScreenshotToScreenPipeline: bevy_ecs::system::Resource`
    = help: the trait `bevy_ecs::system::Resource` is implemented for `ScreenshotToScreenPipeline`
    = note: required for `Unique<(dyn std::any::Any + 'static)>` to implement `Sync`
note: required because it appears within the type `Box<(dyn std::any::Any + 'static)>`
   --> /home/runner/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/alloc/src/boxed.rs:233:12
    |
233 | pub struct Box<
    |            ^^^
note: required because it appears within the type `wgpu::BindGroupLayout`
   --> /home/runner/.cargo/registry/src/index.crates.io-6f17d22bba15001f/wgpu-22.1.0/src/lib.rs:796:12
    |
796 | pub struct BindGroupLayout {
    |            ^^^^^^^^^^^^^^^
    = note: required for `Arc<wgpu::BindGroupLayout>` to implement `std::marker::Send`
note: required because it appears within the type `bind_group_layout::BindGroupLayout`
   --> crates/bevy_render/src/render_resource/bind_group_layout.rs:8:12

Funnily enough, I can't reproduce it on my mac with latest nightly 🤔

cargo +nightly check --target wasm32-unknown-unknown -Z build-std=std,panic_abort

Looks like CI / build-wasm-atomics doesn't like this change.

I think (?) we need to add WgpuWrapper around a bunch of these, which is gross but what I expected from removing the macro.

It was a bit of an ick, but i wrapped everything in WgpuWrapper now...

since it seems like extra functionality (beyond the compile time optimizations) have been added to the render_resource_wrapper/ErasedXXX types, i'm wondering if we should just remove the debug version and continue to use the release version. or maybe we could wrap up the functionality into a single struct which encapsulates the current macro's release-mode behaviour.

i don't think removing the compile-time optimizations should be resulting in any call-site changes though...

since it seems like extra functionality (beyond the compile time optimizations) have been added to the render_resource_wrapper/ErasedXXX types, i'm wondering if we should just remove the debug version and continue to use the release version. or maybe we could wrap up the functionality into a single struct which encapsulates the current macro's release-mode behaviour.

i don't think removing the compile-time optimizations should be resulting in any call-site changes though...

This will work for everything except wgpu::Device which I need to be shared between the application and Bevy. Basically what you're asking for is what was in the PR to begin with, I think?

But I'm wondering what extra functionality you mean? Deref, or the atomics stuff, or both?

Could you check the compilation duration with this PR? The erased types were introduce for compilation speed, see #5950

Basically what you're asking for is what was in the PR to begin with, I think?

maybe? sorry i'm not sure what was in the pr to begin with (i looked when it only addressed the RenderDevice but not since then).

what extra functionality you mean? Deref, or the atomics stuff, or both?

both - i think the call sites should all be unchanged. The macro can (i think) be replaced with a struct that encapsulates the extra behaviour that's been added on top of the compile time optimizations (which were only ever in the debug version of the macro).

both - i think the call sites should all be unchanged. The macro can (i think) be replaced with a struct that encapsulates the extra behaviour that's been added on top of the compile time optimizations (which were only ever in the debug version of the macro).

Any external user that depends on Bevy and wants to use a shared wgpu::Device would need to use this new Bevy-defined struct instead of WgpuWrapper, which doesn't change much, I think? Except that we can implement Deref for it and it doesn't change the call sites.

Do you mean something like this (just a sketch):

struct BevyArc<T> where T: Send + Sync {
    inner: Arc<send_wrapper::SendWrapper<T>>,
}

impl<T: Send + Sync> Deref for BevyArc<T> { //...

And then remove WgpuWrapper?

It'a bit depressing that we can't just use an Arc, but I'm also not familiar with the problem send_wrapper solves for Bevy, so I am keeping an open mind here.

It'a bit depressing that we can't just use an Arc, but I'm also not familiar with the problem send_wrapper solves for Bevy, so I am keeping an open mind here.

I'm not familiar with the history here, I also agree it's a bit unfortunate we can't just use the arc'd types as well as wgpu's ids. But there's backstory here I'm sure.

I would love some guidance on what to do here. I just wanted a slightly better API for RenderCreation:: Manual 😅

then we just need to run compile timings - are you happy to do that or shall i do it?

I can do it if you teach me how to, but I also think that you doing it yourself might lead to better results. If you don't have an M-series mac on hand, I can do it for mac.

It’s just cargo build --timings, check before and after. If the issue persists (or rather is back, since I did check it was gone a few months ago) you’ll see a ~30% slowdown in debug builds. Release would be unaffected since the macro was only type-erasing in debug.

#5950 had a particularly bad userspace example in the top comment that would get like 95% worse as well.

I’ll run windows tomorrow if nobody else does it in the meantime

MacBook Pro M2 Max - 32GB RAM

Before my changes - commit 5fcbdc1

$ cargo clean
     Removed 148120 files, 123.9GiB total
$ cargo build --timings
...
Finished `dev` profile [unoptimized + debuginfo] target(s) in 47.31s

MacBook Pro M2 Max - 32GB RAM

After my changes - commit 533fa23

$ cargo clean
     Removed 19794 files, 6.0GiB total
$ cargo build --timings
...
Finished `dev` profile [unoptimized + debuginfo] target(s) in 43.66s

Meaning - On my machine, compilation speed decreased by 4 seconds for the debug build (7.7% shorter)

For release (cargo build --release --timings) the numbers are:
Before - Finished release profile [optimized] target(s) in 1m 33s
After - Finished release profile [optimized] target(s) in 1m 32s

Looks like the wasm issue is back. I think that’s because WgpuWrapper derives Deref even though its contents differ by platform. It should probably have a manual impl with target=T in the wasm case, though I don’t know what the knock on effects would be.

Looks like the wasm issue is back. I think that’s because WgpuWrapper derives Deref even though its contents differ by platform. It should probably have a manual impl with target=T in the wasm case, though I don’t know what the knock on effects would be.

I think the fix is easy, go back to the coersion i used before: layout: layout.as_ref().map(|layout| -> &PipelineLayout { layout }),. If you think it's unclear, I can add comments above to explain.

As I wrote in the review comment:

Your suggestion doesn't work, since WgpuWrapper has a different type signature between wasm32 and everything else, so your specific number of derefs &** only works for the non-wasm32-version.

My version works reliably for both, because the compiler will insert the correct number of derefs at compile time.

What do you want me to do?

Yes go ahead, your original way makes sense. Sorry!

Yes go ahead, your original way makes sense. Sorry!

Great! Updated! Let me know how to proceed.

Hmm this changeset costs 10 seconds on current main for me on linux (1 min 2 seconds vs 52 seconds). Perhaps this optimization only comes into play on linux?

rerunning on windows with rustc 1.85
pr is 1m31s, df23b93 (the prior commit) is 1m34s

so win still looks good. pretty surprising, i thought this was a compiler trait-bound rabbit hole which shouldn't be platform dependent at all...

I also see a slowdown with this PR on macos on a m4, a bit on a smaller scale that what Cart is reporting, but with more variation (running on a laptop, and efficiency/performance cores)

To run a good benchmark for this kind of things, you should use something like hyperfine.

git checkout 72aaa41603d28d5d6ed6906f57f6c596ba3868c3 # this PR
hyperfine --prepare 'sleep 1; cargo clean' 'RUSTC_WRAPPER= cargo build --release'
git checkout HEAD~1
hyperfine --prepare 'sleep 1; cargo clean' 'RUSTC_WRAPPER= cargo build --release'

This PR:

  Time (mean ± σ):     81.554 s ±  4.136 s    [User: 1059.526 s, System: 38.421 s]
  Range (min … max):   73.708 s … 85.835 s    10 runs

main just before:

  Time (mean ± σ):     77.674 s ±  7.937 s    [User: 1019.559 s, System: 37.408 s]
  Range (min … max):   62.272 s … 83.375 s    10 runs