On Thu, Mar 11, 2021 at 10:48:40AM +1300, Thomas Munro wrote: > On Tue, Mar 9, 2021 at 6:24 PM Julien Rouhaud <rjuju...@gmail.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!
