chore: Replace fxhash with ahash#1674
Conversation
|
If we are switching the hash algorithm we could also use ahash instead! |
da4d75a to
1a3663f
Compare
49d6efb to
4e4db82
Compare
fe84a68 to
3ec8a94
Compare
|
Now using the standard library’s Unlike Not sure whether there may be other unintended side effects that the tests haven’t caught... |
| let mut visited = HashSet::default(); | ||
|
|
||
| let mut package_names: Vec<_> = packages.keys().collect(); | ||
| package_names.sort(); |
There was a problem hiding this comment.
Another thing I had to change because of the no longer deterministic ordering of the HashSet.
There was a problem hiding this comment.
I prefer we use IndexMap for these types to make sure they are deterministic instead of reordering which is more expensive.
There was a problem hiding this comment.
FAIL [ 0.040s] rattler_conda_types repo_data::topological_sort::tests::test_topological_sort::case_6
stdout ───
running 1 test
test repo_data::topological_sort::tests::test_topological_sort::case_6 ... FAILED
failures:
failures:
repo_data::topological_sort::tests::test_topological_sort::case_6
test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 324 filtered out; finished in 0.03s
stderr ───
thread 'repo_data::topological_sort::tests::test_topological_sort::case_6' panicked at crates/rattler_conda_types/src/repo_data/topological_sort.rs:292:21:
attempting to install panel (package 336 of 339) but dependency holoviews is not yet installed
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
@baszalmstra Even with IndexMap / IndexSet that particular test keeps failing if I don't sort the package names manually.
Could it be possible, that there's a bug which didn't get triggered with FxHashMap as it had a different random ordering - but what happened to be accidentally the correct order for the test to be successful?
62ad0b0 to
65e06a6
Compare
| let mut visited = HashSet::default(); | ||
|
|
||
| let mut package_names: Vec<_> = packages.keys().collect(); | ||
| package_names.sort(); |
There was a problem hiding this comment.
I prefer we use IndexMap for these types to make sure they are deterministic instead of reordering which is more expensive.
|
@baszalmstra I think it would be good if you could quickly glance at Felix' comment @haecker-felix could you please solve the merge conflicts? |
|
Rebased, merge conflicts resolved, and tests are passing now (windows test is an unrelated network error, should be fine when running again). Looks like my issue in #1674 (comment) has been resolved by another change through rebasing. |
Hofer-Julian
left a comment
There was a problem hiding this comment.
Thank you, Felix ✨
|
Ah Im sorry I didnt didnt find the time to properly comment here. I was still noodling what was bothering me and why we now sometimes need to resort the entries. The current PR changes the functionality of the library in unexpected way, as fields are no longer sorted before serialization. I think we should add back the serialize_with code to ensure that when writing the repodata files they are properly sorted. |
|
I think we should fix that before we release because it might break repodata indexing. |
The
fxhashcrate is no longer maintained. See https://rustsec.org/advisories/RUSTSEC-2025-0057.htmlThe
rustc-hashis pretty much a drop-in replacement forfxhash. It has the same type names, but uses a different hashing algorithm under the hood: