On Fri, Nov 28, 2014 at 8:20 AM, Tom Lane <tgl@sss.pgh.pa.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 */
AttrNumberattno = 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.
