Improve add-type operations

[kosak]

1. Improve the efficiency of various "add"-style operations by reducing the number of duplicative outer map lookups.
2. Add a private helper method lookupOrCreateInner to reduce the number of repetitions of the setCopyOnWrite call.
3. Carefully document and rewrite addRangeClosed so it is clearer what is going on. Also, for the "middle section", where the method is creating completely full inner Roaring bitmaps, create one "the hard way" and then copy it if you need more. (Note: if there is some more efficient way to create a completely full 32 bit Roaring, please let me know)
4. Change addMany(size_t, const uint64_t*) to cache its last outer map lookup and perhaps save a few lookups if there is locality that can be exploited.

[Dr-Emann]

Is there any reason to consider doing addRangeClosed backwards, so we could use std::map::insert(hint, value), passing the previously inserted value iterator as the hint? I imagine it would be extra complexity for dubious gain.

[kosak]

@Dr-Emann that's a cool idea! Just thinking about it offhand, I would say that

1. I don't think there's a need rewrite the code to go backwards. If one wants to insert right after some iterator it, I think one can just increment the iterator and voila, now you're inserting just before it.
2. Also addRangeClosed has the property that it's iterating over contiguous numeric keys. So you could have code like

advance iterator to next
if you find the key you expect, then the inner 32 bit map already exists, so modify it in place
otherwise do a trick like `hint = std::next(it); map.insert(hint, empty_roaring)` or similar

Nice idea! Hmm, let me do a dry-run of doing it that way now. I wouldn't want to do it if it makes the code look too ugly, but if the code is still readable, I think it would be reasonable to implement your idea.

[kosak]

I've rebased and fixed a couple of snake cases. I think I'll save the suggestion from @Dr-Emann until a future PR.
Reviewers, I think this is ready for you to take a look.

[SLieve]

Looks good to me!

[kosak]

Can you give me another chance to look over this one?
I want to see if I can use the technique that (I feel) worked so well in #402
to eliminate the outer lookups even further (using basically @Dr-Emann 's idea).
If you agree, kindly give me a day or two to come back to you on this one. (Or, if you prefer, merge this one and we can take another pass at it in a new PR)

[SLieve]

In this PR we have a straightforward improvement: simplification and some potential speedup, and I think it's ready to merge. With the technique of #402 the benefits are performance at the cost of some increase in complexity. For that reason, I'd suggest to do that in a separate PR so we can evaluate whether the performance is worth the complexity. Whether in this PR or in another, I'd suggest benchmarking.

[kosak]

Its your judgment call. I would point out that the way I would do it is pull out the ensureRangePopulated() into a helper method, as I have done just now now in #402

Then both flipClosed() in that PR and addRangeClosed() in this PR could call out to this same helper method. I would also remove the helper method lookupOrCreateInner from this PR as it would no longer be needed.

Then, the main bodies of addRangeClosed() and flipClosed() would be quite short and IMO pretty readable again.

[SLieve]

I see, that does look much better! If you'd like let's give it a try in this PR.

[kosak]

OK, this latest version is how it looks with ensureRangePopulated(). You may decide that it's ok, or it's a bridge too far and we can back out.

I also took the liberty of rebasing against the latest master branch.

Above I said we could remove lookupOrCreateInner() but I was wrong; the other add operations still use it.

[SLieve]

@kosak I agree with you that this is just as readable as before, and likely a performance improvement too. Looks great, just a few nits.

[SLieve]

Nit: This is somewhat hard to read, it would make it easier to read if it was a numbered list:

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

[kosak]

I rewrote it. Kindly let me know if this explanation reads better?

[kosak]

Responded to review feedback, and rebased onto latest master

[SLieve]

Looks good!

[kosak]

Fixed typo. Please take another look.

[lemire]

Merging.