-
Notifications
You must be signed in to change notification settings - Fork 320
Add benchmarks to monitor lookup hit/miss performances depending on load factor #637
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
Conversation
|
Unrelated clippy false positive: |
src/map.rs
Outdated
| #[allow(clippy::no_effect, clippy::unnecessary_operation)] // false positive lint | ||
| map[&4]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is clippy happy if you do something like this instead?
| #[allow(clippy::no_effect, clippy::unnecessary_operation)] // false positive lint | |
| map[&4]; | |
| _ = map[&4]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, thanks.
There is still an issue with MSRV it seems:
error: package rayon-core v1.13.0 cannot be built because it requires rustc 1.80 or newer, while the currently active rustc version is 1.65.0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The MSRV test is using -Z direct-minimal-versions, but that doesn't help the indirect rayon-core. This could be worked around by adding environment CARGO_RESOLVER_INCOMPATIBLE_RUST_VERSIONS=fallback to that command. Then we'll get minimal direct versions and compatible versions of everything else.
Adding rayon v1.2.0 (available: v1.10.0)
Adding rayon-core v1.12.1 (available: v1.13.0, requires Rust 1.80)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's probably worth a separate PR for these fixes that are unrelated to your benchmarks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree. I'll drop the Clippy fix too eventually, just wanted to see if I could get the CI to run.
Should we ping someone to look into it?
|
CI should hopefully be fixed by #638. |
|
Removed clippy fix. Ready to merge. |
Current implementation incurs a performance penalty as table gets full, in particular for lookup miss.
This is a good way to quantify and monitor this behavior so we can try improving it in the future.
Typical results look like this: