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

Minimize moves by introducing WeakVec #6419

Merged
merged 2 commits into from
Oct 18, 2024
Merged

Conversation

teoxoy
Copy link
Member

@teoxoy teoxoy commented Oct 17, 2024

This should improve the situation in bevyengine/bevy#15893.

Follow-up to #5874.

@DGriffin91 could you give this PR a try?

@teoxoy teoxoy requested a review from a team as a code owner October 17, 2024 12:12
Copy link
Member

@ErichDonGubler ErichDonGubler left a comment

Choose a reason for hiding this comment

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

Conditionally approving based on user confirmation that this does, indeed, solve a problem for them. I'm open to merging in the absence of that feedback, but I think waiting a day or so to confirm would be good.

wgpu-core/src/device/resource.rs Outdated Show resolved Hide resolved
wgpu-core/src/weak_vec.rs Show resolved Hide resolved
@DGriffin91
Copy link

Unfortunately it seems like bevy still has the issue with this PR. It might be slightly faster though. I have the bevy branch with this pr up if you wanted to play with it.

First frame time with cargo run --example many_cubes --release -- --vary-material-data-per-instance

119619ms: bevy w/ minimize-moves
129969ms: bevy main

vtune filtered in on just the part that is hanging w/ minimize-moves:
image

@tychedelia
Copy link

The problem here as far as I can tell is that we are creating 160k unique bind groups that all share the same texture in a loop. Even without the call to retain, WeakVec.push(..) iterates over self.inner referencing all the bind groups we've created so far, which ends up looking exponential:
image

@teoxoy
Copy link
Member Author

teoxoy commented Oct 18, 2024

The problem here as far as I can tell is that we are creating 160k unique bind groups that all share the same texture in a loop.

This is worst case scenario for this code path. We can alleviate this issue further by running the code every X invocations of push rather than on every invocation - I will give it a try.

@teoxoy
Copy link
Member Author

teoxoy commented Oct 18, 2024

I pushed another commit that should be a good balance between perf and memory use.

@DGriffin91 & @tychedelia could you give it a try?

@tychedelia
Copy link

@teoxoy Yay! Validated on Metal and Vulkan That seems to do it. 🎉✨

@teoxoy teoxoy merged commit f669024 into gfx-rs:trunk Oct 18, 2024
27 checks passed
@teoxoy teoxoy deleted the minimize-moves branch October 18, 2024 15:24
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.

4 participants