Re: Replace buffer I/O locks with condition variables (reviving an old patch)

On Thu, Mar 11, 2021 at 10:48:40AM +1300, Thomas Munro wrote:
> On Tue, Mar 9, 2021 at 6:24 PM Julien Rouhaud <rjuju123(at)gmail(dot)com> wrote:
> >
> > +1 for adding the cv into BufferDesc. That brings the struct size to exactly
> > 64 bytes on x86 64 bits architecture. This won't add any extra overhead to
> > LOCK_DEBUG cases, as it was already exceeding the 64B threshold, if that even
> > was a concern.
>
> I also checked that it's 64B on an Arm box. Not sure about POWER.
> But... despite the fact that it looks like a good change in isolation,
> I decided to go back to the separate array in this initial commit,
> because the AIO branch also wants to add a new BufferDesc member[1].

Ok!

> I may come back to that change, if we can make some more space (seems
> entirely doable, but I'd like to look into that separately).

- /*
- * It would be nice to include the I/O locks in the BufferDesc, but that
- * would increase the size of a BufferDesc to more than one cache line,
- * and benchmarking has shown that keeping every BufferDesc aligned on a
- * cache line boundary is important for performance. So, instead, the
- * array of I/O locks is allocated in a separate tranche. Because those
- * locks are not highly contended, we lay out the array with minimal
- * padding.
- */
- size = add_size(size, mul_size(NBuffers, sizeof(LWLockMinimallyPadded)));
+ /* size of I/O condition variables */
+ size = add_size(size, mul_size(NBuffers,
+ sizeof(ConditionVariableMinimallyPadded)));

Should we keep for now some similar comment mentionning why we don't put the cv
in the BufferDesc even though it would currently fit the 64B target size?

> Thanks for the review. And of course to Robert for writing the patch. Pushed.

Great!