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

Make append O(N) #132

Merged
merged 12 commits into from
Jan 12, 2022
Merged

Make append O(N) #132

merged 12 commits into from
Jan 12, 2022

Conversation

saik0
Copy link
Contributor

@saik0 saik0 commented Jan 10, 2022

Continuation on #131

Summary

  • Append is currently O(N logN) as it must binary search through containers on each insert. Inserting into an array container is also logN.
  • This converts append operation to O(N) by adding a crate public push_unchecked methods that should only be called once it's been verified that the value >= max. Eliminating both of the logN lookups

Before

Screen Shot 2022-01-09 at 11 13 35 PM

After

Screen Shot 2022-01-10 at 4 40 30 AM

@saik0
Copy link
Contributor Author

saik0 commented Jan 10, 2022

Mistakenly opened this as a PR, rather than a draft.

Treemap append needs tests that spans across bitmaps

@Kerollmops Kerollmops marked this pull request as draft January 10, 2022 14:33
@Kerollmops
Copy link
Member

Mistakenly opened this as a PR, rather than a draft.

Fixed that for you 😄 (top right, just below the reviewers).

@saik0
Copy link
Contributor Author

saik0 commented Jan 10, 2022

Mistakenly opened this as a PR, rather than a draft.

Fixed that for you 😄 (top right, just below the reviewers).

I don't believe I can change that as a non-member

@Kerollmops
Copy link
Member

I don't believe I can change that as a non-member

I just checked on a non-owned repo and yes you can, it is just that the link is not present as it is now a Draft, but I am pretty sure you can 😃

src/bitmap/container.rs Outdated Show resolved Hide resolved
src/bitmap/inherent.rs Show resolved Hide resolved
src/treemap/inherent.rs Show resolved Hide resolved
src/bitmap/store.rs Outdated Show resolved Hide resolved
@saik0
Copy link
Contributor Author

saik0 commented Jan 10, 2022

These methods are not unsafe their are just "dangerous" 😅

The unsafe keyword has a very precise meaning:

Code or interfaces whose memory safety cannot be verified by the type system.

push_unchecked does not violate memory safety, though care does need to be taken when calling it, which is why i suffixed it with unchecked.

What would you think about adding some debug_assert to the bodies to check the invariants in debug builds?

@saik0
Copy link
Contributor Author

saik0 commented Jan 10, 2022

Also it's pub(crate) so end users can't call it. Us contributors are the only ones who need to be careful.

@Kerollmops
Copy link
Member

Kerollmops commented Jan 10, 2022

Ok, it seems like you are right, for dangerous internal functions we can keep them safe. Use this comment for #127 too, don't declare them as unsafe!

@saik0
Copy link
Contributor Author

saik0 commented Jan 11, 2022

I've added debug assertions and clear docs about the validity guarantees.

@saik0 saik0 marked this pull request as ready for review January 11, 2022 21:38
src/bitmap/inherent.rs Outdated Show resolved Hide resolved
src/bitmap/container.rs Outdated Show resolved Hide resolved
src/bitmap/iter.rs Outdated Show resolved Hide resolved
src/bitmap/iter.rs Outdated Show resolved Hide resolved
src/bitmap/store.rs Outdated Show resolved Hide resolved
src/bitmap/store.rs Outdated Show resolved Hide resolved
src/bitmap/store.rs Outdated Show resolved Hide resolved
src/treemap/iter.rs Outdated Show resolved Hide resolved
src/treemap/iter.rs Outdated Show resolved Hide resolved
src/treemap/iter.rs Outdated Show resolved Hide resolved
saik0 and others added 8 commits January 12, 2022 03:27
Co-authored-by: Clément Renault <[email protected]>
Co-authored-by: Clément Renault <[email protected]>
Co-authored-by: Clément Renault <[email protected]>
Co-authored-by: Clément Renault <[email protected]>
Co-authored-by: Clément Renault <[email protected]>
Co-authored-by: Clément Renault <[email protected]>
Co-authored-by: Clément Renault <[email protected]>
src/bitmap/store.rs Outdated Show resolved Hide resolved
Copy link
Member

@Kerollmops Kerollmops left a comment

Choose a reason for hiding this comment

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

Thank you very much for your work!
bors merge

@bors
Copy link
Contributor

bors bot commented Jan 12, 2022

Build succeeded:

@bors bors bot merged commit 17d37b9 into RoaringBitmap:master Jan 12, 2022
not-jan pushed a commit to not-jan/roaring-rs that referenced this pull request Aug 31, 2022
132: Make append O(N) r=Kerollmops a=saik0

Continuation on RoaringBitmap#131 

## Summary

* Append is currently O(N logN) as it must binary search through containers on each insert. Inserting into an array container is also logN.
 * This converts append operation to O(N) by adding a crate public `push_unchecked` methods that should only be called once it's been verified that the value >= max. Eliminating both of the logN lookups

## Before
![Screen Shot 2022-01-09 at 11 13 35 PM](https://user-images.githubusercontent.com/997050/148779927-fc095082-0f22-4fdd-b7b8-dc73b333359f.png)
## After
![Screen Shot 2022-01-10 at 4 40 30 AM](https://user-images.githubusercontent.com/997050/148779952-782b9ce9-22c5-4a01-933d-7e858da3d668.png)



Co-authored-by: saik0 <[email protected]>
Co-authored-by: Joel Pedraza <[email protected]>
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.

2 participants