Re: StrategyGetBuffer optimization, take 2

On Mon, Aug 19, 2013 at 5:17 PM, Jeff Janes <jeff(dot)janes(at)gmail(dot)com> wrote:
> On Wed, Aug 7, 2013 at 7:40 AM, Merlin Moncure <mmoncure(at)gmail(dot)com> wrote:
>
>> I agree; at least then it's not unambiguously better. if you (in
>> effect) swap all contention on allocation from a lwlock to a spinlock
>> it's not clear if you're improving things; it would have to be proven
>> and I'm trying to keep things simple.
>>
>> Attached is a scaled down version of the patch that keeps the freelist
>> lock but still removes the spinlock during the clock sweep. This
>> still hits the major objectives of reducing the chance of scheduling
>> out while holding the BufFreelistLock and mitigating the worst case
>> impact of doing so if it does happen. An even more scaled down
>> version would keep the current logic exactly as is except for
>> replacing buffer lock in the clock sweep with a trylock (which is
>> IMNSHO a no-brainer).
>
> Since usage_count is unsigned, are you sure that changing the tests
> from "buf->usage_count == 0" to "buf->usage_count <= 0" accomplishes
> what you need it to? If usage_count gets decremented when it already
> zero, it will wrap around to 65,535, at least on some compilers some
> of the time, won't it?
>
> It seem safer just not to decrement if we can't get the lock.

Hurk -- well, maybe it should be changed to signed in this
implementation (adjustment w/o lock).

Safer maybe, but you lose a part of the optimization: not having to
spam cache line locks as you constantly spinlock your way around the
clock. Maybe that doesn't matter much -- I'm inclined to test it both
ways and see -- plus maybe a 3rd variant that manages the freelist
itself with a spinlock as well.

variant A: buffer spinlock -> trylock (1 liner!)
variant B: as above, but usage_count manipulation occurs outside of lock
variant C: as above, but dispense with lwlock wholly or in part (plus
possibly stuff the freelist etc).

merlin