Code review comment for lp:~akopytov/percona-server/bug1131189

Revision history for this message
Alexey Kopytov (akopytov) wrote :

Hi Laurynas,

Thanks for the review.

On Mon, 25 Mar 2013 08:25:24 -0000, Laurynas Biveinis wrote:
> Review: Needs Information
>
> It's a good patch, just couple of relatively small questions and comments:
>
> - The only thing I'd look into doing differently would be not
> doing linear search at all in trx_reserve_descriptor(), and just
> adding a debug assertion that the array is sorted after the
> insert (which would be just the comparison with the n-1th
> element). Would save a dozen code bytes in a presumably very hot
> code path.
>

I'd like to keep that code. Assuming that invariant will always hold
and enforcing it with a debug-only assertion looks a bit fragile to me.

> - I must be missing something here, but why there is padding
> inserted between descr_n_max and descr_n_used in trx_sys? It
> seems that these fields are accessed at close times.
>

They are accessed at close times, but not always modified at close
times. So by splitting them into separate cache lines we avoid false
sharing in some (actually most) cases.

> - File read0read.h adds the include of trx0sys.h. But the diff of
> read0read.h does not show anything new external used there.
> Rather, it is read0read.ic which starts using
> trx_find_descriptor(), thus the trx0sys.h include should be
> moved to read0read.ic.
>

It was originally in read0read.ic, but that was breaking debug builds
due to some tricky inter-dependency between read0read.h, read0read.ic
and something else. Details elude me now, but my conclusion was that
adding that include to .h was the only clean way to resolve them.

> - Spurious whitespace changes in diff lines 381, 382, 645,
> 1155-1157, 1220.
>

Fixed those and a few other ones.

> - Why does trx_reserve_decriptor() loop: n_used++; while (n_used >
> n_max) n_max *= 2 ? A second iteration of this loop can never
> happen, it looks maybe a bit too defensive for me,
> and this is next to a very hot code path, isn't it? Since there
> is if (n_used > n_max) just above, one unconditional n_max *= 2;
> should be enough.
>

Probably remnants from previous implementation iterations. Fixed.

> - Why does trx_free_prepared() free its view now?
>

Because we are going to free trx a few lines later. Technically it's a
follow-up to the fix for bug #1131187 to avoid a memory leak, but they
both were originally in the same tree (with multiple revisions per
each), and it was tricky to figure what belongs to what. There's
actually another change that logically would belong to that fix.

« Back to merge proposal