Re: condition variables - Mailing list pgsql-hackers

On Tue, Nov 22, 2016 at 3:05 PM, Kyotaro HORIGUCHI
<horiguchi.kyotaro@lab.ntt.co.jp> wrote:
> Hello,
>
> At Mon, 21 Nov 2016 15:57:47 -0500, Robert Haas <robertmhaas@gmail.com> wrote in
<CA+TgmobFjwcFEiq8j+fvH5CdXHdVJffmemNLq8MqFesg2+4Gwg@mail.gmail.com>
>> On Thu, Aug 11, 2016 at 5:47 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>> > So, in my
>> > implementation, a condition variable wait loop looks like this:
>> >
>> > for (;;)
>> > {
>> >     ConditionVariablePrepareToSleep(cv);
>> >     if (condition for which we are waiting is satisfied)
>> >         break;
>> >     ConditionVariableSleep();
>> > }
>> > ConditionVariableCancelSleep();
>>
>> I have what I think is a better idea.  Let's get rid of
>> ConditionVariablePrepareToSleep(cv) and instead tell users of this
>> facility to write the loop this way:
>>
>> for (;;)
>> {
>>     if (condition for which we are waiting is satisfied)
>>         break;
>>     ConditionVariableSleep(cv);
>> }
>> ConditionVariableCancelSleep();
>
> It seems rather a common way to wait on a condition variable, in
> shorter,
>
> | while (condition for which we are waiting is *not* satisfied)
> |     ConditionVariableSleep(cv);
> | ConditionVariableCancelSleep();

Ok, let's show it that way.

>> ConditionVariableSleep(cv) will check whether the current process is
>> already on the condition variable's waitlist.  If so, it will sleep;
>> if not, it will add the process and return without sleeping.
>>
>> It may seem odd that ConditionVariableSleep(cv) doesn't necessary
>> sleep, but this design has a significant advantage: we avoid
>> manipulating the wait-list altogether in the case where the condition
>> is already satisfied when we enter the loop.  That's more like what we
>
> The condition check is done far faster than maintaining the
> wait-list for most cases, I believe.
>
>> already do in lwlock.c: we try to grab the lock first; if we can't, we
>> add ourselves to the wait-list and retry; if we then get the lock
>> after all we have to recheck whether we can get the lock and remove
>> ourselves from the wait-list if so.  Of course, there is some cost: if
>> we do have to wait, we'll end up checking the condition twice before
>> actually going to sleep.  However, it's probably smart to bet that
>> actually needing to sleep is fairly infrequent, just as in lwlock.c.
>>
>> Thoughts?
>
> FWIW, I agree to the assumption.

Here's a version that works that way, though it allows you to call
ConditionVariablePrepareToSleep *optionally* before you enter your
loop, in case you expect to have to wait and would rather avoid the
extra loop.  Maybe there isn't much point in exposing that though,
since your condition test should be fast and waiting is the slow path,
but we don't really really know what your condition test is.  I
thought about that because my use case (barrier.c) does in fact expect
to hit the wait case more often than not.  If that seems pointless
then perhaps ConditionVariablePrepareToSleep should become static and
implicit.  This version does attempt to suppress spurious returns, a
bit, using proclist_contains.  No more cvSleeping.

It's possible that future users will want a version with a timeout, or
multiplexed with IO, in which case there would be some interesting
questions about how this should interact with WaitEventSet.  It also
seems like someone might eventually want to handle postmaster death.
Perhaps there shoul eventually be a way to tell WaitEventSet that
you're waiting for a CV so these things can be multiplexed without
exposing the fact that it's done internally with latches.

--
Thomas Munro
http://www.enterprisedb.com