Re: Making empty Bitmapsets always be NULL

On Tue, 13 Jun 2023 at 00:32, Yuya Watari <watari(dot)yuya(at)gmail(dot)com> wrote:
> In March, I reported that David's patch caused a degradation in
> planning performance. I have investigated this issue further and found
> some bugs in the patch. Due to these bugs, Bitmapset operations in the
> original patch computed incorrect results. This incorrect computation
> resulted in unexpected behavior, which I observed as performance
> degradation. After fixing the bugs, David's patch showed significant
> performance improvements. In particular, it is worth noting that the
> patch obtained a good speedup even when most Bitmapsets have only one
> word.

Thank you for looking at this again and finding and fixing the two
bugs and running some benchmarks.

I've incorporated fixes for the bugs in the attached patch. I didn't
quite use the same approach as you did. I did the fix for 0003
slightly differently and added two separate paths. We've no need to
track the last non-zero word when 'a' has more words than 'b' since we
can't truncate any zero-words off for that case. Not having to do
that makes the do/while loop pretty tight.

For the fix in the 0004 patch, I think we can do what you did more
simply. I don't think there's any need to perform the loop to find
the last non-zero word. We're only deleting a member from a single
word here, so we only need to check if that word is the last word and
remove it if it's become zero. If it's not the last word then we
can't remove it as there must be some other non-zero word after it.

I also made a small adjustment to bms_get_singleton_member() and
bms_singleton_member() to have them Assert fail if result is < 0 after
looping over the set. This should no longer happen so I thought it
would make more compact code if that check was just removed. We'd
likely do better if we got reports of Assert failures here than, in
the case of bms_get_singleton_member, some code accidentally doing the
wrong thing based on a corrupt Bitmapset.

David