-
Notifications
You must be signed in to change notification settings - Fork 1k
Load FXC dynamically to remove the dependency on d3dcompiler_47.dll
#7588
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
base: trunk
Are you sure you want to change the base?
Conversation
Yeah the lifetime is there to make sure the library isn't unloaded while the function pointer is hanging around. It's perfectly sound to pull a pointer and continue using it for the life of the module. if something needs to move the instructions around, it would expose a trampoline function with a stable address. |
… repeated usage. Fixed clippy warnings. Fixed merge conflicts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One small comment, will do a more proper review later
unsafe { | ||
let lib = crate::dx12::DynLib::new(Self::PATH) | ||
.map_err(|e| GetContainerError::FailedToLoad(FxcLib::PATH, e))?; | ||
let d3dcompile_fn: D3DCompileFn = *lib.get::<D3DCompileFn>(c"D3DCompile".to_bytes())?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this use pD3DCompile instead of needing to ahve the definition ourselves?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pD3DCompile is an Option<fn>
but we are using the fn
directly, not sure if there is a way to use it.
Connections
Implements #7531.
Description
Currently, wgpu statically links
d3dcompiler_47.dll
, even though it is only required for FXC, which is probably very rarely used. This increases the size of the executable and reduces compatibility, even if only slightly.This PR loads
d3dcompiler_47.dll
dynamically, removing the dependence on it.To achieve this, we change
dxc_container
to a genericcompiler_container
which is an enum that holds the relevant data for each compiler variant: Fxc, DynamicDxc, and StaticDxc. Only the DX12 shader compiler variant specified by the user is loaded.Previously, the
shader_compilation
module exposed multiple functions to create a container and compile shaders.Now, everything has been unified under the new
CompilerContainer
enum which exposesnew_*
functions to create the appropriate compiler container, and acompile
function which will call the appropriate implementation for each of the different compilers.There is one thing I am unsure about and would like to get your feedback on: In this PR, every call to
D3DCompile
reloads the function from the dll usinglibloading
due to the lifetime imposed bylibloading
. I chose this route because it is safe, but it is clearly not very performant. A better option would be to obtain a pointer to the function, and reuse that pointer every time. The thing that stopped me from going down that route is that I don't know if the library is pinned in memory once it is loaded. If it is, we can simply keep a pointer to the function in the same struct that holds the library; If the library is not pinned in memory, it would be incorrect to store the pointer.Testing
Tested using
cargo test
and running my wgpu/bevy based application, both work as expected.Additionally, using windows'
dumpbin /dependents <executable>
no longer showsd3dcompiler_47.dll
as a dependent.Squash or Rebase?
Squash
Checklist
cargo fmt
.cargo test
.cargo xtask test
to run tests.CHANGELOG.md
entry.