Re: Strange Bitmapset manipulation in DiscreteKnapsack()

Thanks for having a look at this again.

On Thu, 18 Jan 2024 at 15:22, Richard Guo <guofenglinux(at)gmail(dot)com> wrote:
> Do you think we can use 'memcpy(a, b, BITMAPSET_SIZE(b->nwords))'
> directly in the new bms_replace_members() instead of copying the
> bitmapwords one by one, like:
>
> - i = 0;
> - do
> - {
> - a->words[i] = b->words[i];
> - } while (++i < b->nwords);
> -
> - a->nwords = b->nwords;
> + memcpy(a, b, BITMAPSET_SIZE(b->nwords));
>
> But I'm not sure if this is an improvement or not.

I considered this earlier but felt it was going against the method
used in other places in the file. However, on relooking I do see
bms_copy() does a memcpy().

I'm still in favour of keeping it the way the v2 patch does it for 2 reasons:

1) Ignoring bms_copy(), we use do/while in all other functions where
we operate on all words in the set.
2) memcpy isn't that fast for small numbers of bytes when that number
of bytes isn't known at compile-time.

The do/while method can take advantage of knowing that the Bitmapset
will have at least 1 word allowing a single loop check when the set
only has a single word, which I expect most Bitmapsets do.

Of course, memcpy() is fewer lines of code and this likely isn't that
performance critical, so there's certainly arguments for memcpy().
However, it isn't quite as few lines as the patch you pasted. We
still need to overwrite a->nwords to ensure we grow the set or shrink
it to trim off any trailing zero words (which I didn't feel any need
to actually set to 0).

David