Removing a counterproductive optimization.

[lemire]

This is related to PR

#374

cc @incaseoftrouble

[lemire]

@richardstartin : can you have a look? This PR will undo one of your optimizations.

I think it is counterproductive to check the cardinality first as it forces two traversal through the bitmap.

[richardstartin]

Makes sense, actually I just ported this logic from CRoaring in the first place. Might be worth running the fuzz tests before removing it.

[coveralls]

Coverage increased (+0.03%) to 91.538% when pulling e9f79ec on dlemire/issue374 into a03f716 on master.

[lemire]

@richardstartin I think you need to click on "approve" (as I formally asked you to approve this PR).

@incaseoftrouble can you check whether this fixes your issue? I suspect that it will.

[lemire]

@richardstartin The code in C does not have this optimization...

bool roaring_bitmap_is_subset(const roaring_bitmap_t *ra1,
                              const roaring_bitmap_t *ra2) {
    const int length1 = ra1->high_low_container.size,
              length2 = ra2->high_low_container.size;

    int pos1 = 0, pos2 = 0;

    while (pos1 < length1 && pos2 < length2) {
        const uint16_t s1 = ra_get_key_at_index(&ra1->high_low_container, pos1);
        const uint16_t s2 = ra_get_key_at_index(&ra2->high_low_container, pos2);

        if (s1 == s2) {
            uint8_t container_type_1, container_type_2;
            void *c1 = ra_get_container_at_index(&ra1->high_low_container, pos1,
                                                 &container_type_1);
            void *c2 = ra_get_container_at_index(&ra2->high_low_container, pos2,
                                                 &container_type_2);
            bool subset =
                container_is_subset(c1, container_type_1, c2, container_type_2);
            if (!subset) return false;
            ++pos1;
            ++pos2;
        } else if (s1 < s2) {  // s1 < s2
            return false;
        } else {  // s1 > s2
            pos2 = ra_advance_until(&ra2->high_low_container, s1, pos2);
        }
    }
    if (pos1 == length1)
        return true;
    else
        return false;
}

[lemire]

Got around the bureaucratic check.

[lemire]

Merging. Will release soon.

[richardstartin]

@lemire you're right, but I definitely don't remember adding this as an optimisation, possibly for correctness or just absent-mindedness. I'll run the some of the fuzz tests on this to check.

[incaseoftrouble]

@lemire Thanks for pushing this so fast! 👍

[richardstartin]

@lemire this was added in 470500087b6ac3462718a88854beb823bc6c078a and was papering over cracks later fixed in cfafb50644d195ca7e22db4f0ff141caa9852c24, not an optimisation, so was absolutely safe to remove. The relevant fuzz tests didn't find any problems.

[lemire]

@richardstartin Thank you.

Note the benchmark that I added... this optimization definitively made things much, much slower (like hundreds of times) in some cases. So I have to thank @incaseoftrouble.

[richardstartin]

Once again, it wasn’t an optimisation 👍

[lemire]

it wasn’t an optimisation

Ok. Fair enough. But it looked like one when I stared at the code.

BTW it is not obvious why it would be bad. You have to sit down and think hard.

[richardstartin]

In general whenever the bitmaps are passed over twice it costs (could also improve xorCardinality by calculating the cardinality containerwise rather than bitmap wise to avoid multiple bitmap passes) when the bitmaps are large... on this occasion it wasn’t an unprofiled optimisation obtained by a process of divination, but a hack.

[richardstartin]

Also, this is a problem whenever you don’t have access to modify the source code... any boolean circuit which could be calculated in a single pass over the containers can end up passing over bitmaps several times, and it has a cost.

[lemire]

Also, this is a problem whenever you don’t have access to modify the source code... any boolean circuit which could be calculated in a single pass over the containers can end up passing over bitmaps several times, and it has a cost.

Right. There might be clever techniques to get around this without forcing people to hack the internals.