Horton

Thread

Commits

  1. Improve various places that double the size of a buffer

  1. Use pg_nextpower2_* in a few more places

    David Rowley <dgrowleyml@gmail.com> — 2021-06-12T12:31:31Z

    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

  2. Re: Use pg_nextpower2_* in a few more places

    Zhihong Yu <zyu@yugabyte.com> — 2021-06-12T12:55:13Z

    On Sat, Jun 12, 2021 at 5:32 AM 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.
>
> 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
>
Hi,

-       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) ?

Cheers

  3. Re: Use pg_nextpower2_* in a few more places

    David Rowley <dgrowleyml@gmail.com> — 2021-06-12T13:40:13Z

    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

  4. Re: Use pg_nextpower2_* in a few more places

    Zhihong Yu <zyu@yugabyte.com> — 2021-06-12T14:13:09Z

    On Sat, Jun 12, 2021 at 6:40 AM David Rowley <dgrowleyml@gmail.com> wrote:

> 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
>
Hi,
Maybe add an assertion after the assignment, that newalloc >=
 LWLockTrancheNamesAllocated.

Cheers

  5. Re: Use pg_nextpower2_* in a few more places

    David Rowley <dgrowleyml@gmail.com> — 2021-06-12T14:35:08Z

    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

  6. Re: Use pg_nextpower2_* in a few more places

    Zhihong Yu <zyu@yugabyte.com> — 2021-06-12T14:44:18Z

    On Sat, Jun 12, 2021 at 7:35 AM David Rowley <dgrowleyml@gmail.com> wrote:

> 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
>
Hi,
Interesting, the quoted if () line was not shown in the patch.
Pardon my not checking this line.

In that case, the assertion is not needed.

  7. Re: Use pg_nextpower2_* in a few more places

    David Rowley <dgrowleyml@gmail.com> — 2021-06-30T12:24:19Z

    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