-
Notifications
You must be signed in to change notification settings - Fork 8
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
Clearer byte range definition in get_range
and get_ranges
#156
Conversation
get_range
and get_ranges
cc @maxrjones A Rust match on two optional integers should be extremely cheap, and much cheaper than matching Python objects. I guess it really depends on the Python overhead of translating keyword arguments into rust. In terms of the Rust overhead of the option match, I'd expect that should be on the order of nanoseconds. Maybe any additional Python overhead would add up to a microsecond extra? @maxrjones Would you be able to rerun your zarr benchmark from this branch? |
Happy to. Can you provide guidance on how to install the crate + bindings from git? usually I'd use |
I think you need |
Just checked and
worked for me |
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.
Sorry for the delay, took me a second to be able to use compiled code on the VEDA Hub and I wanted to run the benchmarks in us-west-2. Anyways, the new rate for loading a ~3.7GB array with Zarr is ~0.36% slower which IMO is acceptable.
No worries! Thanks for running it!
So it was 7.92s before this change and 7.948s after this change? I figure that could just be noise? |
I used a instance with half as many cores so it was ~2x that time for each. If you'd be concerned about that relative difference, if real, I could record the individual timings to determine if there's a statistically significant difference. |
Nah, that's good. Thanks for checking that benchmark! |
Closes #155