Re: [AD] review timed-wait changes

[Peter Wang <novalazy@xxxxxxxxxx>]

On 2008-04-07, David Capello <davidcapello@xxxxxxxxxx> wrote:
> Hi Peter
> 
> On Sun, Apr 6, 2008 at 11:42 AM, Peter Wang <novalazy@xxxxxxxxxx> wrote:
> >  2. change the interface of _al_cond_timedwait() to take an absolute
> >  timeout rather than a relative one.  There's no really compelling reason
> >  for this but the old interface made the pthread implementation a bit
> >  convoluted, converting structured time representations to floats then
> >  back again.  The new interface makes the Win32 implementation a little
> >  convoluted, but hopefully less so.  Could somebody apply the patch and
> >  check that exnew_timedwait works in Windows? Also check if the code to
> >  handle timeGetTime() overflows makes sense.
> 
> I applied the patch. It gives some warnings in the compilation:
> 
> src/win/wxthread.c: In function `_al_cond_timeout_init':
> src/win/wxthread.c:307: warning: comparison of unsigned expression < 0
> is always false
> src/win/wxthread.c:308: warning: implicit declaration of function `timeGetTime'
> src/win/wxthread.c: In function `_al_cond_timedwait':
> src/win/wxthread.c:332: warning: comparison between signed and unsigned

Thanks for that.

> Also the exnew_timedwait.c needs a little change
> -      if (al_wait_for_event(queue, &event, 100)) {
> +      if (al_wait_for_event_timed(queue, &event, 0.100)) {
> 
> 
> About the overflows, I think that we could simplified the
> implementation in wxthread.c using this:
> 
> /* it isn't included, but timeGetTime is defined there */
> #include <mmsystem.h>
> ...
> void _al_cond_timeout_init(_AL_COND_TIMEOUT *timeout, unsigned int rel_msecs)
> {
>    timeout->abstime = timeGetTime() + rel_msecs;
> }
> ...
> int _al_cond_timedwait(_AL_COND *cond, _AL_MUTEX *mtxExternal,
>    const _AL_COND_TIMEOUT *timeout)
> {
>    DWORD now;
>    DWORD rel_msecs;
> 
>    ASSERT(cond);
>    ASSERT(mtxExternal);
> 
>    now = timeGetTime();
>    rel_msecs = timeout->abstime - now;
>    if (rel_msecs == INFINITE) {
>       rel_msecs--;
>    }
> 
>    return cond_wait(cond, mtxExternal, rel_msecs);
> }

I think you're right.

> I think that we should call one time at the beginning the
> timeBeginPeriod and timeEndPeriod routines to get more
> precision with timeGetTime in Win2K.

Yes, we do that when the system driver initialises.

> >  Question: should al_wait_for_event_timed() take an absolute timeout
> >  specification, or should we provide both?  Relative timeouts are
> >  convenient for one off calls, but absolute timeouts would be much better
> >  when waiting for multiple events in a loop, e.g.
> >
> >     float abst = al_current_time() + X;
> >     while (al_wait_for_event_timed(queue, &ev, abst)) {
> >         handle ev
> >     }
> >     /* time's up */
> 
> Could be convenient, anyway the relative-timeout is the more
> common option in all APIs. I think that we should provide both
> options. The absolute-timeout routine could be called
> al_wait_for_event_until() or something.

Peter Hull's example makes a convincing case for including the absolute
version :-)

Implementing on Windows might be trouble. The code above, 

    rel_msecs = timeout->abstime - now;

is incorrect if 'timeout->abstime' isn't guaranteed to be at least as
late as 'now'.

Peter