Thread: Use pg_nextpower2_* in a few more places

Back in f0705bb62, we added pg_nextpower2_32 and pg_nextpower2_64 to
efficiently obtain the next power of 2 of a given number using an
intrinsic function to find the left-most 1 bit.

In d025cf88b and 02a2e8b44, I added some usages of these new functions
but I didn't quite get all of them done.   The attached replaces all
of the remaining ones that I'm happy enough to go near.

The ones that I left behind are ones in the form of:

while (reqsize >= buflen)
{
   buflen *= 2;
   buf = repalloc(buf, buflen);
}

The reason I left those behind is that I was too scared that I might
introduce an opportunity to wrap buflen back around to zero again.  At
the moment the repalloc() would prevent that as we'd go above
MaxAllocSize before we wrapped buflen back to zero again.  All the
other places I touched does not change the risk of that happening.

It would be nice to get rid of doing that repalloc() in a loop, but it
would need a bit more study to ensure we couldn't wrap or we'd need to
add some error checking code that raised an ERROR if it did wrap.  I
don't want to touch those as part of this effort.

I've also fixed up a few places that were just doubling the size of a
buffer but used a "while" loop to do this when a simple "if" would
have done.  Using an "if" is ever so slightly more optimal since the
condition will be checked once rather than twice when the buffer needs
to increase in size.

I'd like to fix these for PG15.

David

Thanks for having a look.

On Sun, 13 Jun 2021 at 00:50, Zhihong Yu <zyu@yugabyte.com> wrote:
> -       newalloc = Max(LWLockTrancheNamesAllocated, 8);
> -       while (newalloc <= tranche_id)
> -           newalloc *= 2;
> +       newalloc = pg_nextpower2_32(Max(8, tranche_id + 1));
>
> Should LWLockTrancheNamesAllocated be included in the Max() expression (in case it gets to a high value) ?

I think the new code will produce the same result as the old code in all cases.

All the old code did was finding the next power of 2 that's >= 8 and
larger than tranche_id.  LWLockTrancheNamesAllocated is just a hint at
where the old code should start searching from.  The new code does not
need that hint. All it seems to do is save the old code from having to
start the loop at 8 each time we need more space.

David

On Sun, 13 Jun 2021 at 02:08, Zhihong Yu <zyu@yugabyte.com> wrote:
> Maybe add an assertion after the assignment, that newalloc >=  LWLockTrancheNamesAllocated.

I don't quite see how it would be possible for that to ever fail.  I
could understand adding an Assert() if some logic was outside the
function and we wanted to catch something outside of the function's
control, but that's not the case here.  All the logic is within a few
lines.

Maybe it would help if we look at the if condition that this code
executes under:

/* If necessary, create or enlarge array. */
if (tranche_id >= LWLockTrancheNamesAllocated)

So since we're doing:

+       newalloc = pg_nextpower2_32(Max(8, tranche_id + 1));

assuming pg_nextpower2_32 does not give us something incorrect, then I
don't quite see why Assert(newalloc >=  LWLockTrancheNamesAllocated)
could ever fail.

Can you explain why you think it might?

David

On Sun, 13 Jun 2021 at 00:31, David Rowley <dgrowleyml@gmail.com> wrote:
>
> Back in f0705bb62, we added pg_nextpower2_32 and pg_nextpower2_64 to
> efficiently obtain the next power of 2 of a given number using an
> intrinsic function to find the left-most 1 bit.
>
> In d025cf88b and 02a2e8b44, I added some usages of these new functions
> but I didn't quite get all of them done.   The attached replaces all
> of the remaining ones that I'm happy enough to go near.

> I'd like to fix these for PG15.

I had another look over this patch and it looks ok to me. I plan to
push it in the next day or so.

David