-
Notifications
You must be signed in to change notification settings - Fork 82
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
Use trailing zeros in bitmap to_array conversion #68
Conversation
Hey @damnMeddlingKid, I am the new maintainer of this repository and wanted to make sure that merging or testing this PR works correctly, I just updated the CI and the bors settings, so could it be possible that you rebase on master please? (don't forget to pull master before rebasing 😄) |
Hey @josephglanville, Do you have time to make sure that this is a valid change? I am not sure that only the tests can be trusted. Thank you :) |
Sure, I will take a look. |
im getting
after rebasing, is that related to the CI changes ?. |
Ho this is maybe related to the fact that the remote URL changed, not sure! |
@damnMeddlingKid It might help to update your remote to the new repository. Note that the repository's location changed. It should not be necessary... but... |
actually i have the upstream setup to this repository already. this is what i have setup
here are the steps that i ran
this throws the following error
|
@damnMeddlingKid Have you found this link already? https://techdailychronicle.com/github-actions-error-refusing-to-allow-an-oauth-app-to-create-or-update-workflow/
And if you don't find how to fix it, maybe it would be easier to create a new PR with your two commits? |
404c844
to
12e047f
Compare
thanks, that sort of helped. i realized i wasn't using the ssh paths in my remotes. |
I am waiting for a review from @josephglanville before merging. bors try |
tryBuild succeeded: |
Both behaviour and performance are identical as far as I can tell. This code is perhaps slightly more readable. |
🔒 Permission denied Existing reviewers: click here to make josephglanville a reviewer |
74: Add remove_range benchmark that triggers bitmap to array conversion r=Kerollmops a=josephglanville Wrote this to verify assumptions of #68 there are other benches that trigger bitmap to array conversion but this nicely isolates it. Co-authored-by: Joseph Glanville <jpg@jpg.id.au>
Thank you @josephglanville, I will merge your benchmarks in #74 before this one. |
Build succeeded: |
Thank you @damnMeddlingKid for your pull request, it indeed make the code simpler! 👍 |
The existing performance benchmark sets ranges in the bitmap so the performance doesn't benefit much from this optimization. I expect it would make a difference on real world data. Thanks for the review ! |
74: Add remove_range benchmark that triggers bitmap to array conversion r=Kerollmops a=josephglanville Wrote this to verify assumptions of RoaringBitmap#68 there are other benches that trigger bitmap to array conversion but this nicely isolates it. Co-authored-by: Joseph Glanville <jpg@jpg.id.au>
68: Use trailing zeros in bitmap to_array conversion r=Kerollmops a=damnMeddlingKid This PR implements an optimization on bitmap to_array conversion by utilizing the trailing_zeros method. Curious if theres a good reason to not implement it this way. Im still learning rust so let me know if this looks off. this version is faster when there are gaps between set bits and performs about the same when all 64 bits are set. cc: @Nemo157 Co-authored-by: Franklyn D'souza <franklynd@gmail.com>
This PR implements an optimization on bitmap to_array conversion by utilizing the trailing_zeros method.
Curious if theres a good reason to not implement it this way. Im still learning rust so let me know if this looks off.
this version is faster when there are gaps between set bits and performs about the same when all 64 bits are set.
cc: @Nemo157