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

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. 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
{