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(at)gmail(dot)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!