Hi Thomas,
Thanks for reviewing!
On Wed, Mar 13, 2019 at 12:40:57PM +1300, Thomas Munro wrote:
> Can we just refer to the other function's documentation for this? I
> don't want two copies of this blurb (and this copy-paste already
> failed to include "Timed" in the example function name).
Hah - point well taken. Fixed.
> One difference compared to pthread_cond_timedwait() is that pthread
> uses an absolute time and here you use a relative time (as we do in
> WaitEventSet API). The first question is which makes a better API,
> and the second is what the semantics of a relative timeout should be:
> start again every time we get a spurious wake-up, or track time
> already waited? Let's see...
>
> - (void) WaitEventSetWait(cv_wait_event_set, -1, &event, 1,
> + ret = WaitEventSetWait(cv_wait_event_set, timeout, &event, 1,
> wait_event_info);
>
> Here you're restarting the timeout clock every time through the loop
> without adjustment, and I think that's not a good choice: spurious
> wake-ups cause bonus waiting.
Agree. In my testing WaitEventSetWait did the work for calculating the
right timeout remaining. It's a bummer that we have to repeat the same
pattern at the ConditionVariableTimedSleep() but I guess anyone who
loops in such cases will have to adjust their values accordingly.
BTW, I am curious why Andres in 98a64d0bd71 didn't just create an
artificial event with WL_TIMEOUT and return that from
WaitEventSetWait(). Would have made it cleaner than checking checking
return values for -1.
Updated v2 patch attached.
--
Shawn Debnath
Amazon Web Services (AWS)