Introduce insert_range for RoaringTreemap
#222
Conversation
|
Hey @not-jan, Thank you very much for your work, I appreciate the About the benchmark issue, I think GitHub broke URLs like git://github.com/orga/repo.git. I don't know why it was working before but I just made #225 that you can rebase on. Sorry for the inconvenience. If it works correctly you should be able to add a benchmark 😃 (you must |
not-jan
changed the title
insert_range for RoaringTreemapinsert_range for RoaringTreemap
|
Thank you for the quick response :) I've performed the requested changes and added my benchmark. I'm not sure whether my benchmark is that effective, since I've copied it from the implementation for the RoaringBitmap. Any opinions about that? |
benchmarks/benches/lib.rs
Outdated
| @@ -650,6 +651,30 @@ fn insert_range_bitmap(c: &mut Criterion) { | |||
| } | |||
| } | |||
|
|
|||
| fn insert_range_treemap(c: &mut Criterion) { | |||
| for &size in &[10, 100, 1_000, 5_000, 10_000, 20_000] { | |||
There was a problem hiding this comment.
Could we add bigger values here?
There was a problem hiding this comment.
I've added 2 * (2^32 -1) now to ensure that we're crossing a bitmap boundary. Is there any point in adding an additional benchmark that compares the new insert_range to (0..size).collect<RoaringTreemap>()? Gut feeling says it's gonna be awfully slow with the current implementation.
There was a problem hiding this comment.
Maybe we can add two benchmarks indeed, this way we must be able to improve that in the future. That cool be good indeed!
There was a problem hiding this comment.
Hey @not-jan,
Thank you for the time you put into this PR, I would like to do a release of roaring-rs soon. I will merge your work as is and we could improve it in future PRs.
I'll try to merge it with bors but there are merge conflicts so we'll see. If it doesn't pass, could you please rebase on main (yes main, I changed the name), please?
bors merge
|
Merge conflict. |
|
Should be all done :) |
not-jan
force-pushed
the
master
branch
2 times, most recently
from
16f4896
to
f293c8b
Compare
I think I erased it from the commit history succesfully but I seem to have accidentally included 9d9b807 in my rebase. I can't seem to undo that though as my reflog refuses to return to a state before the rebase started :/ |
222: Introduce `insert_range` for RoaringTreemap r=Kerollmops a=not-jan Implemented a somewhat naive version of `insert_range` for RoaringTreemap ~~I wanted to add a separate benchmark to compare the performance of `insert_range` with collecting from an iterator but was unable to do so because `cargo bench` would fail with a connection error every time I ran it.~~ **Edit:** This is fixed now with #225 Co-authored-by: saik0 <[email protected]> Co-authored-by: not-jan <[email protected]>
|
Build failed: |
240: Introduce `insert_range` for the `Treemap` r=Kerollmops a=Kerollmops This is a follow-up of #222, I cleaned it up before merging it into the main branch. Co-authored-by: not-jan <[email protected]> Co-authored-by: Clément Renault <[email protected]>
Implemented a somewhat naive version of
insert_rangefor RoaringTreemapI wanted to add a separate benchmark to compare the performance ofinsert_rangewith collecting from an iterator but was unable to do so becausecargo benchwould fail with a connection error every time I ran it.Edit: This is fixed now with #225