Re: Patch: Optimize memory allocation in function 'bringetbitmap'

Hi,

On 10/16/2015 08:00 PM, Jinyu Zhang wrote:
>
> Update the patch_brin_optimze_mem according to your comment.

I've reviewed the last version of the patch, and I do have a few
comments. I haven't done any performance testing at this point, as the
machine I'm using for that is chewing on something else at the moment.

1) bringetbitmap(PG_FUNCTION_ARGS)

+ /*
+ * Estimate the approximate size of btup and allocate memory for btup.
+ */
+ btupInitSize = bdesc->bd_tupdesc->natts * 16;
+ btup = palloc(btupInitSize);

What is the reasoning behind this estimate? I mean, this only accounts
for the values, not the NULL bitmap or BrinTuple fields. Maybe it does
not really matter much, but I'd like to see some brief description in
the docs, please.

This also interacts with our memory allocator in a rather annoying way,
because palloc() always allocates memory in 2^k chunks, but sadly the
estimate ignores that. So for natts=3 we get btupInitSize=48, but
internally allocate 64B (and ignore the top 16B).

2) bringetbitmap(PG_FUNCTION_ARGS)

if (tup)
{
if(size <= btupInitSize)
memcpy(btup, tup, size);
else
btup = brin_copy_tuple(tup, size);
LockBuffer(buf, BUFFER_LOCK_UNLOCK);
}

Is there any particular reason why not to update btupInitSize to the
size value? I mean, if the estimate is too low, then we'll call
brin_copy_tuple() for all BRIN tuples, no?

In other words, I think the code should do this:

if (tup)
{
if(size <= btupInitSize)
memcpy(btup, tup, size);
else
{
btup = brin_copy_tuple(tup, size);
brinInitSize = size;
}
LockBuffer(buf, BUFFER_LOCK_UNLOCK);
}

Another option might be the usual "doubling" strategy, i.e. do something
like this instead:

if (tup)
{
while (btupInitSize < size)
btupInitSize *= 2;

btup = repalloc(btup, btupInitSize);
memcpy(btup, tup, size);

LockBuffer(buf, BUFFER_LOCK_UNLOCK);
}

Possibly with only doing the repalloc when actually needed.

3) brin_new_memtuple(...)

The changes in this function seem wrong to me. Not only you've removed
the code that initialized all the bv_* attributes, you're also using
palloc() so there could easily be random data. This might be OK for our
code as we call brin_memtuple_initialize() right after this function,
and that seems to do the init, but AFAIK brin_new_memtuple() is a part
of our public API at it's in a header file. And these changes surely
break the contract documented in the comment above the function:

* Create a new BrinMemTuple from scratch, and initialize it to an empty
* state.

So I think this is wrong and needs to be reverted.

It's possible that we'll immediately reset the attributes, but that's
negligible cost - we expect to do brin_memtuple_initialize() many times
for the tuple, so it should not make any difference.

Another thing is that the three arrays (bt_values, bt_allnulls and
bt_hasnulls) may be allocated as a single chunk and then sliced.

char * ptr = palloc0(space for all three arrays);
bt_values = ptr;
bt_allnulls = ptr + (size of bt_values)
bt_bt_hasnulls = ptr + (size of bt_values + bt_allnulls)

which is essentially what the original code did with bv_values. This
reduces palloc() overhead and fragmentation.

4) brin_memtuple_initialize(...)

It's possible that the new code needs to reset more fields, but I don't
really see why it should mess with dtuple->bt_columns[i].bv_values for
example.

5) brin_deform_tuple(..)

The comment before the function probably should explain the purpose of
the new parameter (that it can either be NULL or an existing tuple).

regards

--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services