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

Remove exposed C symbols from renderpass/computepass recording #5409

Merged
merged 5 commits into from
Apr 17, 2024

Conversation

Wumpf
Copy link
Member

@Wumpf Wumpf commented Mar 17, 2024

Connections

Description
There was a seemingly random assortment of C symbols exported from wgpu around command encoding. The C methods were already hardly usable since they first require initialization of objects that can only be created from Rust.
I noticed this a few weeks prior as unintentionally exported symbols on applications and libraries that use wgpu and the issue described in #3105 is caused by this.
The wgpu crate's wgpu-core implementation as well as wgpu-native rely on these methods, but not the fact that they're exported and #[no_mangle].

Removing the FFI opens a few small clean-up opportunities and less unsafe code here.

EDIT: This PR leaves intentionally leaves out bundles because firefox still relies on this. See comment below

Testing
It compiles!

The real test here is the review - am I missing something and these need to stick around?

Checklist

  • Run cargo fmt.
  • Run cargo clippy. If applicable, add:
    • --target wasm32-unknown-unknown
    • --target wasm32-unknown-emscripten
  • Run cargo xtask test to run tests.
  • Add change to CHANGELOG.md. See simple instructions inside file.

@Wumpf Wumpf requested a review from a team as a code owner March 17, 2024 21:25
@cwfitzgerald
Copy link
Member

I think Firefox relies on this, so we can't back this out without confirming with them.

@Wumpf
Copy link
Member Author

Wumpf commented Mar 18, 2024

Pulled firefox source now to have a look at: Firefox has its own ffi layer https://hg.mozilla.org/mozilla-central/file/tip/gfx/wgpu_bindings/src/command.rs which again falls the methods from compute_ffi/render_ffi but does not rely on them being exported on their own.

But best for some Mozilla folk to chime in and confirm. @jimblandy maybe since you commented on that ticket before?

wgpu-core/src/command/bundle.rs Outdated Show resolved Hide resolved
wgpu-core/src/command/render.rs Outdated Show resolved Hide resolved
@jimblandy
Copy link
Member

@Wumpf Thanks for the ping. Since @nical changed command recording to never use wgpu in the content process, I think we may not need these any more, but let me check.

@jimblandy
Copy link
Member

jimblandy commented Mar 24, 2024

The #[no_mangle] declarations in wgpu-core/src/command/render.rs and compute.rs are no longer in use, and can be removed.

However, it seems like Firefox is still using wgpu-core/src/command/bundle.rs's #[no_mangle] functions from C++ code like this. It should be straightforward to fix these in the same way the render and compute recording function uses were removed.

@jimblandy
Copy link
Member

I assume resolving this issue requires all #[no_mangle] attributes to be removed from wgpu-core. I've filed Bug 1887574 for getting that done.

@Wumpf
Copy link
Member Author

Wumpf commented Mar 24, 2024

thank you! I have some ongoing work that requires changes to the compute functions specifically in compute & render (follow-up to #5432 ) so I'll change this PR to clean up only those and we can take bundles at a later point in time.

@Wumpf Wumpf marked this pull request as draft March 24, 2024 22:06
@Wumpf Wumpf changed the title Remove exposed C symbols from bundle/renderpass/computepass recording Remove exposed C symbols from renderpass/computepass recording Mar 24, 2024
@Wumpf Wumpf marked this pull request as ready for review March 24, 2024 22:24
@Wumpf
Copy link
Member Author

Wumpf commented Mar 24, 2024

all done and cleaned-up. Removed all FFI code now from compute & render, but didn't touch the bundle interface.
Removing ffi scaffolding resulted in quite a bit of streamlining!

Copy link
Member

@cwfitzgerald cwfitzgerald left a comment

Choose a reason for hiding this comment

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

Amazing!

@cwfitzgerald
Copy link
Member

Blocked on #5549

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants