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

chore: abstract away hashbrown #7395

Merged
merged 8 commits into from
Mar 19, 2024
Merged

chore: abstract away hashbrown #7395

merged 8 commits into from
Mar 19, 2024

Conversation

DaniPopes
Copy link
Member

@DaniPopes DaniPopes commented Mar 13, 2024

Revm is moving away from hashbrown on std, so we want to stop exporting it ourselves.

@DaniPopes DaniPopes requested a review from mattsse as a code owner March 13, 2024 17:37
@DaniPopes
Copy link
Member Author

@grandizzy any idea why the test_persist_fuzz_failure test would fail with seemingly unrelated changes?

@grandizzy
Copy link
Collaborator

grandizzy commented Mar 18, 2024

@grandizzy any idea why the test_persist_fuzz_failure test would fail with seemingly unrelated changes?

hm, it seems like test was delayed/reexecuted so failure was recorded for a different fuzzed input. Probably should make sure persist failure always deleted when test enters, I can look more into and make test robust

@grandizzy
Copy link
Collaborator

@grandizzy any idea why the test_persist_fuzz_failure test would fail with seemingly unrelated changes?

hm, it seems like test was delayed/reexecuted so failure was recorded for a different fuzzed input. Probably should make sure persist failure always deleted when test enters, I can look more into and make test robust

actually I can reproduce it locally with this branch, the failures are not properly reloaded from file hence new failures. I am looking into it, works OK in master

@grandizzy
Copy link
Collaborator

grandizzy commented Mar 19, 2024

@grandizzy any idea why the test_persist_fuzz_failure test would fail with seemingly unrelated changes?

hm, it seems like test was delayed/reexecuted so failure was recorded for a different fuzzed input. Probably should make sure persist failure always deleted when test enters, I can look more into and make test robust

actually I can reproduce it locally with this branch, the failures are not properly reloaded from file hence new failures. I am looking into it, works OK in master

the issue is that with std HashSet the state is not reconstructed similarly / ordered between runs (as hashbrown's HashSet for some reason I'm missing it did...) using BTreeSet for FuzzDictionary fix this issue see #7438 Could introduce a slightly performance penalty @DaniPopes

@DaniPopes
Copy link
Member Author

std hashmap is a wrapper around hashbrown, iteration order is never guaranteed
if order matters use vec or indexmap (crates.io)

@DaniPopes
Copy link
Member Author

I wanted to use indexset anyway for performance when iterating, since we iterate way more than inserting values. Thanks for looking into it

@DaniPopes DaniPopes merged commit a527c1c into master Mar 19, 2024
20 checks passed
@DaniPopes DaniPopes deleted the dani/rm-hashbrown branch March 19, 2024 18:50
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