-
Notifications
You must be signed in to change notification settings - Fork 164
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: Use twox-hash 2.0 xxhash64 oneshot api instead of custom implementation #1041
chore: Use twox-hash 2.0 xxhash64 oneshot api instead of custom implementation #1041
Conversation
I am unsure if license.txt and Notice.txt also need to be updated |
Cargo Bench results: hash/xxhash64/8192 time: [414.63 µs 415.13 µs 415.67 µs]
change: [-6.6364% -6.3063% -5.9526%] (p = 0.00 < 0.05)
Performance has improved.
Found 8 outliers among 100 measurements (8.00%)
5 (5.00%) high mild
3 (3.00%) high severe |
Yes, the twox_hash references should be removed from those files as part of this PR |
How should I proceed with the pipeline failure which comes from a new clippy rule (manual_pattern_char_comparison) introduced in 1.81.0? |
I have added a new commit which removes the references |
I think it would be fine to apply the change in this PR. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1041 +/- ##
=========================================
Coverage 34.46% 34.47%
- Complexity 888 889 +1
=========================================
Files 113 113
Lines 43580 43580
Branches 9658 9658
=========================================
+ Hits 15021 15024 +3
+ Misses 25507 25505 -2
+ Partials 3052 3051 -1 ☔ View full report in Codecov by Sentry. |
I got slightly different cargo bench results, but I saw no regression in overall TPC-H performance.
|
Interesting, I just reran the bench and I get the same improvements as before. Should I compare the library implementation with the DataFusion custom one to see if there is a difference? Shall I add the following command to the docs: |
It is possible that the results could vary depending on hardware. Out of interest what platform are you testing on? I am testing on an AMD Ryzen 9 CPU with Linux. 5% is a small price to pay to remove the custom implementation and go back to using the dependency, IMO.
That would be great as a separate PR, thank you. |
Thank you @andygrove for reviewing the PR and starting the CI.
I will do that later |
Which issue does this PR close?
Closes #1032
Rationale for this change
What changes are included in this PR?
How are these changes tested?