Repository navigation
Conversation
| let mut tail = vec.split_off(start).into_iter(); | ||
| let mut prev = start; | ||
| for (index, value) in self.deferred { | ||
| vec.extend(tail.by_ref().take(index - prev)); |
There was a problem hiding this comment.
This seems to depend on deferred being in sorted order: otherwise, index could be less than prev (and would panic in debug mode, do weird stuff in release mode).
There was a problem hiding this comment.
Indeed, @Dr-Emann is right here. In the insert method, you push a tuple of index and value that are arbitrary, and the first index could be larger than the second one pushed. Then, in the finish method, you iterate over all the deferred entries and seem to try to implement some sort of merge join, but the deferred values are always pushed back (never inserted, obviously). To implement a merge join, deferred values must be in sorted order.
I ran the fuzzer for ten minutes but didn't find anything, yet...
One bitmap has containers at even keys and the other one at odd keys, so every container of one side is missing from the other side.
BitOrAssign and the MultiOps union and symmetric difference insert each right-hand container missing on the left with Vec::insert. Every insert shifts the containers after it, so when the right side brings many containers interleaved with the left ones, the operation is quadratic in the number of containers. Containers are still inserted directly when that shifts fewer than 64 containers, and for the first eight insertions that shift more. The other ones are deferred and merged into the vector in a single pass, which only moves the containers that follow the first deferred one.
b360af8 to
e460b17
Compare
| match lhs.binary_search_by_key(&rhs.key, |c| c.key) { | ||
| Err(loc) => lhs.insert(loc, rhs), | ||
| Err(loc) => inserts.insert(lhs, loc, rhs), |
There was a problem hiding this comment.
I'm not even sure to understand how it can work because a container that is not found during the first loop run must be inserted and the next loop will be able to find it and will therefore enter the Ok branch hand this time, triggering the op function to merge both containers.
This currently works because you use the FEW_INSERTS and SHORT_SHIFT parameters to still insert in the target containers (and find the freshly inserted container) but it will no longer work if you inserted too many containers already or if you happen to insert too far on the left. I thought these container mergings could have been implement in the SortedInserts::finish method but as far as I read it I don't see anything like that.
I find the idea to avoid shifts really good but the implementation doesn't seems correct to me.
|
Hey @fjmaco 👋 Please read my last comment. It seems to me that the implementation is wrong. That's not a big deal and I'm happy that @Dr-Emann and I found out that there was something odd in the code. However, I'm concerned about the fact that our CI, benchmarks, nor fuzzer found those bugs. It seems to me that the bug described in my last comment can only be triggered when merging multiple bitmaps simultaneously and that happen when using the Do you think you could find some time to create a test to trigger the bug in a dedicated commit and once you got the right test I can merge it on main, please? Have a nice day 🌵 |
a |= binserts every container ofbthat is missing fromawithVec::insert, and so do theMultiOpsunion and symmetric difference. Each insert shifts all the containers after it. When the right-hand side brings many containers that fall between the left-hand ones, the operation is quadratic in the number of containers:For comparison,
&even | &oddalready builds a new vector in one pass, and takes about as long as|=does with this PR.What this touches:
BitOrAssign<RoaringBitmap>andBitOrAssign<&RoaringBitmap>;BitOrimpls that call them:a | b,a | &band&a | b;union()andsymmetric_difference()on iterators of owned or borrowed bitmaps, throughmerge_container_ownedandmerge_container_ref.BitXorAssignalready merges withPairs, and&a | &bbuilds a new vector, so neither needs this.RoaringTreemapkeeps its bitmaps in aBTreeMap, so it doesn't have the problem at its own level. Its unions call these bitmap operations for each high key, so it gets the fix there.The new benchmark group goes right after
successive_orinbenchmarks/benches/lib.rs, where #361 and #343 also add benchmarks, so whichever lands second needs a trivial rebase there.Approach
The binary search for each right-hand container stays as it is. The insertions now go through a small
SortedInsertshelper inbitmap/util.rs:Vec::insertwhen that shifts fewer than 64 containers. That covers appending, small bitmaps and insertions near the end.finishthen merges the deferred ones into the vector in a single pass. The pass only moves the containers after the first deferred position, so its cost is in proportion to whatVec::insertwould have shifted for that one container.The containers and the operations applied to them are the same as before, and the inserted containers end up at the same positions. Up to 8 long shifts, the code does the same
Vec::insertcalls as main.Tests
tests/union_with.rsandtests/symmetric_difference_with.rs:|,|=,^and^=, andMultiOpsunion and symmetric difference over 2 and 3 bitmaps, against aBTreeSet. The inputs are spread over up to 512 containers, so they can be interleaved, overlapping, disjoint or empty;SortedInsertsthat mixes direct and deferred insertions.serialize_intobytes and the samestatistics().Benchmarks
AMD Ryzen 9 7900X, rustc 1.99.0, pinned to two cores with
taskset -c 10-11,RUSTFLAGS="-C target-cpu=native -C opt-level=3". Each table shows the median, with min–max in parentheses. Other jobs were running on the machine, sometimes on the SMT siblings of these two cores, so differences under ~10% are within noise.The first commit adds an
interleaved_containerscriterion group: one bitmap has containers at even keys and the other at odd keys. 3 runs per side, alternating main and this branch:The existing
pairwise_orandSuccessive Orgroups on the real datasets, 3 runs per side, alternating, with--warm-up-time 1 --measurement-time 3:Successive Or: between −2.5% and +3.8% per benchmark, and every sub-group's geomean is within +2% (within ±1% with functions aligned on both builds).pairwise_or: in a default build the changed operations came out +3% to +6% (geomean), but so didref_ref(&a | &b, +4%), whose code this PR doesn't change. With-C llvm-args=-align-all-functions=6on both builds, everypairwise_orvariant is within ±1.5% (geomean over the 8 datasets), and an in-process A/B on the same data agrees (0.998–1.012). So that difference is code placement, not this change.Default-layout runs from a busier machine; see the note on code placement above for
pairwise_or.All dataset results
Small inputs are where the extra bookkeeping would show. I measured them with main, an identical copy of main and this branch all linked into one binary. The binary alternates between them in every round (9 rounds, median of per-round medians, timer resolution about 10 ns). The copy of main shows how much two builds of the same code differ:
Small and same-key cases
Choosing the thresholds
I picked 8 with a sweep: insert
knew containers into a bitmap ofncontainers with|=(borrowed, vector with spare capacity), placed between the existing ones, in front of them, or after them. I compared main with the count set to 0 (always defer), 4, 8 and 16. Times are in µs, medians of 7 alternating rounds:Sweep
The shift threshold keeps small vectors away from the merge, whose allocations cost more than shifting a few dozen containers. Measured with borrowed
|=(9 alternating rounds), with the shift threshold at 0 / 64 / 256:Limitations
|=of 8 + 8 went from 160 ns to 171 ns, and ownedunion()of 32 + 32 from 902 ns to 1.0 µs.