Re: Marginal performance improvement: replace bms_first_member loops

On Fri, Nov 28, 2014 at 8:20 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

>
> The attached proposed patch adds bms_next_member() and replaces
> bms_first_member() calls where it seemed to make sense. I've had a
> hard time measuring much speed difference for this patch in isolation,
> but in principle it should be less code and less cycles. It also seems
> safer and more natural to not use destructive looping techniques.
>
>
I've had a quick read of the patch and it seems like a good idea.

I have to say I don't really like the modifying of the loop iterator that's
going on here:

col = -1;
while ((col = bms_next_member(rte->modifiedCols, col)) >= 0)
{
col += FirstLowInvalidHeapAttributeNumber;
/* do stuff */
col -= FirstLowInvalidHeapAttributeNumber;
}

Some other code is doing this:

col = -1;
while ((col = bms_next_member(cols, col)) >= 0)
{
/* bit numbers are offset by FirstLowInvalidHeapAttributeNumber */
AttrNumber attno = col + FirstLowInvalidHeapAttributeNumber;

Which seems less prone to future breakage and possibly slightly less cycles.

A while back when I was benchmarking the planner time during my trials with
anti/semi join removals, I wrote a patch to change the usage pattern for
cases
such as:

if (bms_membership(a) != BMS_SINGLETON)
return; /* nothing to do */
singleton = bms_singleton_member(a);
...

Into:

if (!bms_get_singleton(a, &singleton))
return; /* nothing to do */
...

Which means 1 function call and loop over the bitmapset, rather than 2
function
calls and 2 loops over the set when the set is a singleton.

This knocked between 4 and 22% off of the time the planner spent in the join
removals path.

The patch to implement this and change all suitable calls sites is attached.

Regards

David Rowley