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

[wgpu-core] Replace usage of PreHashedMap with FastHashMap #6541

Merged
merged 1 commit into from
Nov 14, 2024

Conversation

jamienicol
Copy link
Contributor

@jamienicol jamienicol commented Nov 13, 2024

Connections
Fixes #6373

Description
PreHashedMap was introduced as a performance optimization. It is, however, potentially unsound as it cannot distinguish between keys that are not equal yet have the same hash. This patch replaces its usage with a simple FastHashMap, which should be good enough. If this is found to cause real life performance issues down the line then we can figure out a better solution.

Testing
cargo xtask test passes

Checklist

  • Run cargo fmt.
  • Run taplo format.
  • 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.

@jamienicol jamienicol requested a review from a team as a code owner November 13, 2024 21:23
@ErichDonGubler ErichDonGubler added type: bug Something isn't working area: correctness We're behaving incorrectly labels Nov 13, 2024
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.

LGTM!

@ErichDonGubler
Copy link
Member

@gfx-rs/wgpu: I'm not sure if we need a CHANGELOG entry here. Might be worth nothing as a General bug fix? 🤔

@Wumpf
Copy link
Member

Wumpf commented Nov 14, 2024

yeah would be nice to have it listed as a bugfix!

@jamienicol
Copy link
Contributor Author

Added the changelog entry (and consolidated the duplicated General categories while I was there)

@ErichDonGubler ErichDonGubler enabled auto-merge (squash) November 14, 2024 12:48
@ErichDonGubler
Copy link
Member

Just needs conflicts resolved, and auto-merge should do the rest.

PreHashedMap was introduced as a performance optimization. It is,
however, potentially unsound as it cannot distinguish between keys
that are not equal yet have the same hash. This patch replaces its
usage with a simple FastHashMap, which should be good enough. If this
is found to cause real life performance issues down the line then we
can figure out a better solution.
@jamienicol
Copy link
Contributor Author

Just needs conflicts resolved, and auto-merge should do the rest.

done

and consolidated the duplicated General categories while I was there

heh, this was already done I just hadn't rebased

@ErichDonGubler ErichDonGubler merged commit 4fb3217 into gfx-rs:trunk Nov 14, 2024
25 checks passed
@jamienicol jamienicol deleted the preHashedMap branch November 14, 2024 13:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: correctness We're behaving incorrectly type: bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Uses of PreHashedMap are probably not sound
3 participants