Re: BufferAlloc: don't take two simultaneous locks

At Sat, 22 Jan 2022 12:56:14 +0500, Andrey Borodin <x4mmm(at)yandex-team(dot)ru> wrote in
> I've took a look into the patch. The idea seems reasonable to me:
> clearing\evicting old buffer and placing new one seem to be
> different units of work, there is no need to couple both partition
> locks together. And the claimed performance impact is fascinating!
> Though I didn't verify it yet.

The need for having both locks came from, I seems to me, that the
function was moving a buffer between two pages, and that there is a
moment where buftable holds two entries for one buffer. It seems to
me this patch is trying to move a victim buffer to new page via
"unallocated" state and to avoid the buftable from having duplicate
entries for the same buffer. The outline of the story sounds
reasonable.

> On a first glance API change in BufTable does not seem obvious to
> me. Is void *oldelem actually BufferTag * or maybe BufferLookupEnt
> *? What if we would like to use or manipulate with oldelem in
> future?
>
> And the name BufTableFreeDeleted() confuses me a bit. You know, in C
> we usually free(), but in C++ we delete [], and here we do
> both... Just to be sure.

Honestly, I don't like the API change at all as the change allows a
dynahash to be in a (even tentatively) broken state and bufmgr touches
too much of dynahash details. Couldn't we get a good extent of
benefit without that invasive changes?

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center