At Sat, 22 Jan 2022 12:56:14 +0500, Andrey Borodin <x4...@yandex-team.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
