Update flip-type operations of Roaring64Map #402

kosak · 2022-11-10T05:22:55Z

Executive summary:

Provided extra entry points for flip on both the 32-bit and 64-bit side to make it uniform with addRange and removeRange
Inspired by an idea from @Dr-Emann in Improve add-type operations #397 , used emplace_hint to minimize map lookups. The resultant code does only one lookup for the whole operation; everything else is just linear iteration through the specified map range, which is cool.
Also flip() does the same "remove bitmaps if they become empty" logic that the other operations do now.
Added unit tests

I like how this one turned out. Kindly let me know what you think.

Some rationale on adding the entry points:

First, provides API uniformity for the end-user, which is always nice.

Second, provides uniformity for the code reader. The style of all of these is to have a couple of "easy" methods and then a longer one that does all the work. But here, the workhorse methods are addRangeClosed(uint64_t, uint64_t) and removeRangeClosed(uint64_t uint64_t) which work with closed intervals, but then we have flip(uint64_t, uint64_t) working with half-open intervals. When the convention changes like that, it's a little harder to analyze.

Also, flip() had a couple of logic errors:

Missed a 'setCopyOnWrite' case when start_high == end_high
When given a half-open interval right at the end of a bitmap, e.g. [0xFFFFFFFF, 0x100000000), the code should see this as falling into a single inner Roaring but it doesn't. This is a (micro) performance problem, not a correctness problem.

I feel rewriting flip() in a style coherent with all the others hopefully makes it easier to understand and use. Kindly let me know what you think.

lemire · 2022-11-11T14:34:27Z

@SLieve Would you review ?

SLieve · 2022-11-11T14:37:36Z

Sure!

cpp/roaring64map.hh

SLieve

I agree with adding flipClosed to the public api, @lemire what do you think?

SLieve · 2022-11-12T11:33:47Z

cpp/roaring64map.hh

+        // Since min and max are uint32_t, highbytes(min or max) == 0. The inner
+        // bitmap we are looking for, if it exists, will be at the first slot of
+        // 'roarings'.
+        if (iter == roarings.end() || iter->first != 0) {


I don't think this is correct. If I have an empty Roaring64Map and I call flipClosed(0, 10), I would expect the range from 0 to 10 to be set. Let's add a test for this case too.

Arrrgh. Great catch!

cpp/roaring64map.hh

SLieve · 2022-11-12T11:44:32Z

cpp/roaring64map.hh

-        roarings[start_high].setCopyOnWrite(copyOnWrite);
+        // 2. Flip intermediate bitmaps completely...
+        for (uint32_t i = 0; i != num_intermediate_bitmaps; ++i) {
+            auto &bitmap = start_iter->second;


Optional: Could we add a small comment here which mentions why we can assume all bitmaps in the range exist? Something like:

// We can directly use the iterator for the entire range, because we made sure it was populated above.

I added a comment above the call to ensureRangePopulated(), making it consistent with the corresponding comment in addRangeClosed() pending in #397. Let me know if you think this is good enough.

tests/cpp_unit.cpp

SLieve · 2022-11-12T11:51:03Z

tests/cpp_unit.cpp

+    // For example (assuming num_slots_to_test = 5), we:
+    // create a Roaring64Map, (do nothing), flip 5 slots, and check
+    // Then we:
+    // create a Roaring64Map, set a bit in slot 0, flip 5 slots, and check
+    // Then we:
+    // create a Roaring64Map, set a bit in slot 1, flip 5 slots, and check
+    // Then we:
+    // create a Roaring64Map, set a bit in slots 0 and 1, flip 5 slots, and check
+    // etc.


Optional: Same comment as in #397, this may read better as a numbered list.

Redid the comment to look like the (redone) comment in #397. Please take another look.

cpp/roaring64map.hh

SLieve · 2022-11-13T21:49:28Z

@kosak I think you forgot to push.

kosak · 2022-11-13T21:59:17Z

Sorry about that. I didn't actually "forget" per se but I clicked through the comments and then I thought I could finish up the code before you got here. I will make sure to do it in the other order next time 😃 Please take another look.

…alls completely into slot 0, delegate to 32-bit flipClosed() rather than 64-bit flipClosed().

SLieve

Looks good to me! @lemire for feedback on adding flipClosed to the API (see above).

kosak · 2022-11-13T22:23:25Z

Sorry, I added one more thing (see 7662b3a )

The rationale here is to put half-open flip() on equal footing with closed-interval flipClosed(uint32, uint32). Meaning they both delegate to a slightly faster version if they end up operating on slot 0.

Let me know if you think this is going too far. Another consistent view is that neither should be a special case, and both should just delegate to the "workhorse" method void flipClosed(uint64_t min, uint64_t max). In other words:

    void flip(uint64_t min, uint64_t max) {
        if (min >= max) {
            return;
        }
        flipClosed(min, max - 1);
    }

    void flipClosed(uint32_t min, uint32_t max) {
      flipClosed(uint64_t(min), uint64_t(max));
    }

Put another way, I'm arguing that for consistency's sake they should either both have this special-case optimization, or neither should. My opinion is they both should.

SLieve · 2022-11-14T22:33:39Z

I can see the symmetry argument, but on the other hand it's also a bit more code, and with flipClosed we have a promise about the input, which means that the two methods are not quite equivalent IMO. I don't feel strongly about this, I think it's readable and understandable with or without this additional change.

…e that falls" This reverts commit 7662b3a.

kosak · 2022-11-14T23:35:45Z

Yeah, I see your point. Let's revert that change. (done)

lemire · 2022-11-15T00:54:30Z

flipClosed is fine as far as I am concerned.

kosak · 2022-11-15T04:02:58Z

Added another commit to fix a logic typo.

lemire · 2022-11-15T23:23:11Z

@SLieve Do you recommend merging this?

lemire · 2022-11-15T23:23:51Z

Merging.

kosak force-pushed the kosak_improve-flip branch from 05f92b9 to ff8dec4 Compare November 10, 2022 05:23

Update flip-type operations of Roaring64Map

3c060a3

kosak force-pushed the kosak_improve-flip branch from ff8dec4 to 3c060a3 Compare November 10, 2022 05:25

kosak mentioned this pull request Nov 10, 2022

Improve add-type operations #397

Merged

Create private helper method 'ensureRangePopulated'

bd994ae

kosak force-pushed the kosak_improve-flip branch from ad48699 to bd994ae Compare November 10, 2022 18:51

typo

c7303ac

kosak commented Nov 11, 2022

View reviewed changes

cpp/roaring64map.hh Show resolved Hide resolved

SLieve reviewed Nov 12, 2022

View reviewed changes

Respond to review feedback.

e9211ec

kosak force-pushed the kosak_improve-flip branch from 38f11d9 to e9211ec Compare November 13, 2022 22:01

If the caller invokes (half-open interval) flip() with a range that f…

7662b3a

…alls completely into slot 0, delegate to 32-bit flipClosed() rather than 64-bit flipClosed().

SLieve approved these changes Nov 13, 2022

View reviewed changes

Revert "If the caller invokes (half-open interval) flip() with a rang…

cda8e05

…e that falls" This reverts commit 7662b3a.

typo

b794888

lemire merged commit ff8aca1 into RoaringBitmap:master Nov 15, 2022

kosak mentioned this pull request Nov 16, 2022

Fix build: remove duplicate 'ensureRangePopulated()' #411

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update flip-type operations of Roaring64Map #402

Update flip-type operations of Roaring64Map #402

kosak commented Nov 10, 2022

lemire commented Nov 11, 2022

SLieve commented Nov 11, 2022

SLieve left a comment

SLieve Nov 12, 2022

kosak Nov 13, 2022

SLieve Nov 12, 2022

kosak Nov 13, 2022

SLieve Nov 12, 2022

kosak Nov 13, 2022

SLieve commented Nov 13, 2022

kosak commented Nov 13, 2022

SLieve left a comment

kosak commented Nov 13, 2022

SLieve commented Nov 14, 2022

kosak commented Nov 14, 2022

lemire commented Nov 15, 2022

kosak commented Nov 15, 2022

lemire commented Nov 15, 2022

lemire commented Nov 15, 2022

Update flip-type operations of Roaring64Map #402

Update flip-type operations of Roaring64Map #402

Conversation

kosak commented Nov 10, 2022

lemire commented Nov 11, 2022

SLieve commented Nov 11, 2022

SLieve left a comment

Choose a reason for hiding this comment

SLieve Nov 12, 2022

Choose a reason for hiding this comment

kosak Nov 13, 2022

Choose a reason for hiding this comment

SLieve Nov 12, 2022

Choose a reason for hiding this comment

kosak Nov 13, 2022

Choose a reason for hiding this comment

SLieve Nov 12, 2022

Choose a reason for hiding this comment

kosak Nov 13, 2022

Choose a reason for hiding this comment

SLieve commented Nov 13, 2022

kosak commented Nov 13, 2022

SLieve left a comment

Choose a reason for hiding this comment

kosak commented Nov 13, 2022

SLieve commented Nov 14, 2022

kosak commented Nov 14, 2022

lemire commented Nov 15, 2022

kosak commented Nov 15, 2022

lemire commented Nov 15, 2022

lemire commented Nov 15, 2022