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

On Thu, Mar 11, 2021 at 03:54:06PM +1300, Thomas Munro wrote:
> On Thu, Mar 11, 2021 at 3:27 PM Julien Rouhaud <rjuju123@gmail.com> wrote:
> > -   /*
> > -    * 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?
> 
> I tried to write some words along those lines, but it seemed hard to
> come up with a replacement message about a thing we're not doing
> because of other currently proposed patches.  The situation could
> change, and it seemed to be a strange place to put this comment
> anyway, far away from the relevant struct.

Yeah, I agree that it's not the best place to document the size consideration.

> Ok, let me try that
> again... what do you think of this, as a new comment for BufferDesc,
> next to the existing discussion of the 64 byte rule?
> 
> --- a/src/include/storage/buf_internals.h
> +++ b/src/include/storage/buf_internals.h
> @@ -174,6 +174,10 @@ typedef struct buftag
>   * Be careful to avoid increasing the size of the struct when adding or
>   * reordering members.  Keeping it below 64 bytes (the most common CPU
>   * cache line size) is fairly important for performance.
> + *
> + * Per-buffer I/O condition variables are kept outside this struct in a
> + * separate array.  They could be moved in here and still fit under that
> + * limit on common systems, but for now that is not done.
>   */
>  typedef struct BufferDesc
>  {

I was mostly thinking about something like "leave room for now as other feature
could make a better use of that space", but I'm definitely fine with this
comment!