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

Check if Bitmap is empty in Contains #413

Closed

Conversation

anacrolix
Copy link
Contributor

I stumbled on this change in a call to Contains in a very tight loop. I believe Contains unnecessarily calls through to some relatively much more expensive contains on each of the container types when the empty state is readily available and avoids this cost for very little overhead.

I wasn't able to find a benchmark in roaring that demonstrated this advantage, however in my production system it shaved off a good 25% CPU.

Verified

This commit was signed with the committer’s verified signature.
Zaczero Kamil Monicz
@lemire
Copy link
Member

lemire commented Feb 9, 2024

I am skeptical about this change. If your bitmaps are usually empty, there might be something wrong with your design. It should be an exceptional case to have empty bitmaps. Meanwhile the test you added is likely to make all other cases (when the bitmap is not empty) slightly more expensive.

@lemire
Copy link
Member

lemire commented Feb 9, 2024

Performance aside, this change makes the code slightly more complicated.

@lemire
Copy link
Member

lemire commented Feb 9, 2024

I am going to close this. If you want to argue for this change, I'd encourage you to provide substantial empirical evidence of the benefits in a realistic settings.

@lemire lemire closed this Feb 9, 2024
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