Skip to content
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

faster iandnot between bitmap containers and run containers #418

Merged
merged 3 commits into from
Apr 8, 2024

Conversation

neena
Copy link
Contributor

@neena neena commented Apr 2, 2024

Rather than converting a run container to a bitmap container and then performing iandnot, call removeRange directly.

name                                          old time/op  new time/op  delta
AndNot/inPlace=true/left=bitmap/right=run-10  6.71µs ±32%  3.08µs ±45%  -54.05%  (p=0.000 n=20+17)

wordRangeStart := rc.iv[0].start / 64
wordRangeEnd := (rc.iv[len(rc.iv)-1].last()) / 64 // inclusive

cardinalityChange := popcntSlice(bc.bitmap[wordRangeStart : wordRangeEnd+1]) // before cardinality - after cardinality (for word range)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not understand this part. I don't understand how you can update the cardinality with a single popcntSlice.

I suspect it is not correct... I am troubled by the fact that our test pass... but my suspicions is that the code is not correct. Can you convince me?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh. Wait. I see what you are doing. Ok. It looks good.

@lemire
Copy link
Member

lemire commented Apr 2, 2024

This is good and I expect to merge. I will give it some time in case someone wants to review it further.

@neena
Copy link
Contributor Author

neena commented Apr 4, 2024

@lemire I just copied this change over to a benchmarking tool I have that tests a prod workload that uses this function and it seems to break something -- could you hold on merging until I get to the bottom what's happening?

@lemire lemire marked this pull request as draft April 5, 2024 04:09
@lemire
Copy link
Member

lemire commented Apr 5, 2024

draft mode

@joenall joenall marked this pull request as ready for review April 5, 2024 14:36
@neena
Copy link
Contributor Author

neena commented Apr 8, 2024

all fixed! thanks for your patience ☺️ the issue was an overflow at the end of the container -- I fixed and made it so that my test failed without the fix

@lemire
Copy link
Member

lemire commented Apr 8, 2024

Merging.

@lemire lemire merged commit 2bf931c into RoaringBitmap:master Apr 8, 2024
8 checks passed
@neena neena deleted the neena/faster-iandnot-bm branch April 8, 2024 20:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants