guile-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: srfi-18 requirements


From: Neil Jerram
Subject: Re: srfi-18 requirements
Date: Tue, 19 Feb 2008 22:48:47 +0000
User-agent: Gnus/5.110006 (No Gnus v0.6) Emacs/21.4 (gnu/linux)

"Julian Graham" <address@hidden> writes:

> Okay, find attached a patch against HEAD containing the aforementioned
> changes to the core for supporting SRFI-18.  I'm still looping my test
> code, but I thought I should get something out to you guys this
> evening.  In addition to the code changes, the patch includes relevant
> Changelog, doc, and threads.test updates.  Let me know what you think.

Looking good!  Many thanks for your continuing work on this, and sorry
for my delay (once again!) in reviewing.  I have a few comments, as
follows.

>  @c begin (texi-doc-string "guile" "join-thread")
> address@hidden {Scheme Procedure} join-thread thread
> address@hidden {Scheme Procedure} join-thread thread [timeout]
>  @deffnx {C Function} scm_join_thread (thread)
> address@hidden {C Function} scm_join_thread_timed (thread, timeout)

Didn't we agree to add a timeout-val parameter here?

> +static scm_t_timespec
> +scm_to_timespec (SCM t)

For static functions it's nice to omit the scm_ prefix, because they
don't need it, and it makes it clearer to the casual reader that
they're not part of the API.

Also, can the signature be void to_timespec (SCM t, scm_t_timespec *),
to avoid relying on support for struct return?

> -SCM_DEFINE (scm_join_thread, "join-thread", 1, 0, 0,
> -         (SCM thread),
> +SCM scm_join_thread (SCM thread)
> +{
> +  return scm_join_thread_timed (thread, SCM_BOOL_F);

You should use SCM_UNDEFINED to indicate an absent parameter, rather
than SCM_BOOL_F.

> +}
> +#undef FUNC_NAME

Last #undef line is extraneous.

> +
> +SCM_DEFINE (scm_join_thread_timed, "join-thread", 1, 1, 0,
> +         (SCM thread, SCM timeout),

What about the timeout_val parameter ...

>  "Suspend execution of the calling thread until the target @var{thread} "
>  "terminates, unless the target @var{thread} has already terminated. ")
> -#define FUNC_NAME s_scm_join_thread
> +#define FUNC_NAME s_scm_join_thread_timed
>  {
> +  int timed_out = 0;
>    scm_i_thread *t;
> -  SCM res;
> +  scm_t_timespec ctimeout, *timeout_ptr = NULL;
> +  SCM res = SCM_BOOL_F;
>  
>    SCM_VALIDATE_THREAD (1, thread);
>    if (scm_is_eq (scm_current_thread (), thread))
> @@ -1005,11 +1068,23 @@
>    t = SCM_I_THREAD_DATA (thread);
>    scm_i_scm_pthread_mutex_lock (&t->admin_mutex);
>  
> +  if (! SCM_UNBNDP (timeout))
> +    {
> +      ctimeout = scm_to_timespec (timeout);
> +      timeout_ptr = &ctimeout;
> +    }
> +
>    if (!t->exited)
>      {
>        while (1)
>       {
> -       block_self (t->join_queue, thread, &t->admin_mutex, NULL);
> +       int err = block_self (t->join_queue, thread, &t->admin_mutex, 
> +                             timeout_ptr);
> +       if (err == ETIMEDOUT)
> +         {
> +           timed_out = 1;

... which I would expect to be assigned to res here.

> +           break;
> +         }
>         if (t->exited)

Do res = t->result here, rather than below, to make clear that it goes
with the t->exited case?

> +static SCM
> +fat_mutex_lock (SCM mutex, scm_t_timespec *timeout, int *ret)
>  {
>    fat_mutex *m = SCM_MUTEX_DATA (mutex);
> +
>    SCM thread = scm_current_thread ();
> -  char *msg = NULL;
> +  scm_i_thread *t = SCM_I_THREAD_DATA (thread);
> +
> +  SCM err = SCM_BOOL_F;
> +
> +  struct timeval current_time;
>  
>    scm_i_scm_pthread_mutex_lock (&m->lock);
>    if (scm_is_false (m->owner))
> -    m->owner = thread;
> +    {
> +      m->owner = thread;
> +      scm_i_pthread_mutex_lock (&t->admin_mutex);
> +      if (scm_is_null (t->mutexes))
> +        t->mutexes = scm_list_1 (mutex);
> +      else
> +        t->mutexes = scm_cons (mutex, t->mutexes);

Just "t->mutexes = scm_cons (mutex, t->mutexes);" is sufficient for
both cases.

> +      scm_i_pthread_mutex_unlock (&t->admin_mutex);
> +      *ret = 1;
> +    }
>    else if (scm_is_eq (m->owner, thread))
>      {
>        if (m->level >= 0)
>       m->level++;
>        else
> -     msg = "mutex already locked by current thread";
> +     err = scm_cons (scm_misc_error_key,
> +                     scm_from_locale_string ("mutex already locked by "
> +                                             "current thread"));
> +      *ret = 0;
>      }
>    else
>      {
> +      int first_iteration = 1;
>        while (1)
>       {
> -       block_self (m->waiting, mutex, &m->lock, NULL);
> -       if (scm_is_eq (m->owner, thread))
> -         break;
> -       scm_i_pthread_mutex_unlock (&m->lock);
> -       SCM_TICK;
> -       scm_i_scm_pthread_mutex_lock (&m->lock);
> +       if (scm_is_eq (m->owner, thread) || scm_c_thread_exited_p (m->owner))
> +         {
> +           scm_i_pthread_mutex_lock (&t->admin_mutex);
> +           if (scm_is_null (t->mutexes))
> +             t->mutexes = scm_list_1 (mutex);
> +           else
> +             t->mutexes = scm_cons (mutex, t->mutexes);

Same again here.

> +       else if (!first_iteration)
> +         {
> +           if (timeout != NULL) 
> +             {
> +               gettimeofday (&current_time, NULL);
> +               if (current_time.tv_sec > timeout->tv_sec ||
> +                   (current_time.tv_sec == timeout->tv_sec &&
> +                    current_time.tv_usec * 1000 > timeout->tv_nsec))
> +                 {
> +                   *ret = 0;
> +                   break;
> +                 }

Is timeout an absolute time, or relative to when join-thread was
called?  Before getting to this code, I thought it was relative - but
then I don't see how the code above can be correct, because it is
comparing against the absolute gettimeofday() ...?

> -SCM_DEFINE (scm_lock_mutex, "lock-mutex", 1, 0, 0,
> -         (SCM mx),
> +SCM scm_lock_mutex (SCM mx)
> +{
> +  return scm_lock_mutex_timed (mx, SCM_BOOL_F);

Should be SCM_UNDEFINED.

> -static char *
> -fat_mutex_unlock (fat_mutex *m)
> +static void
> +fat_mutex_unlock (SCM mx)
>  {
> -  char *msg = NULL;
> -
> +  fat_mutex *m = SCM_MUTEX_DATA (mx);
>    scm_i_scm_pthread_mutex_lock (&m->lock);
> -  if (!scm_is_eq (m->owner, scm_current_thread ()))
> +  if (m->level > 0)
> +    m->level--;
> +  else 

It looks like there is a significant change to the semantics here: any
thread can unlock a mutex, not just the thread that locked it.  Is
that the intention, or am I misunderstanding?

> +static int
> +fat_cond_timedwait (SCM, SCM, const scm_t_timespec *);
>  
> -  return msg;
> +SCM scm_unlock_mutex (SCM mx)
> +{
> +  return scm_unlock_mutex_timed (mx, SCM_UNDEFINED, SCM_UNDEFINED);
>  }
>  
> -SCM_DEFINE (scm_unlock_mutex, "unlock-mutex", 1, 0, 0,
> -         (SCM mx),
> +SCM_DEFINE (scm_unlock_mutex_timed, "unlock-mutex", 1, 2, 0,
> +         (SCM mx, SCM cond, SCM timeout),
>  "Unlocks @var{mutex} if the calling thread owns the lock on "
>  "@var{mutex}.  Calling unlock-mutex on a mutex not owned by the current "
>  "thread results in undefined behaviour. Once a mutex has been unlocked, "
> @@ -1240,18 +1358,39 @@
>  "lock.  Every call to @code{lock-mutex} by this thread must be matched "
>  "with a call to @code{unlock-mutex}.  Only the last call to "
>  "@code{unlock-mutex} will actually unlock the mutex. ")
> -#define FUNC_NAME s_scm_unlock_mutex
> +#define FUNC_NAME s_scm_unlock_mutex_timed
>  {
> -  char *msg;
> +  SCM ret = SCM_BOOL_T;
> +
>    SCM_VALIDATE_MUTEX (1, mx);
> -  
> -  msg = fat_mutex_unlock (SCM_MUTEX_DATA (mx));
> -  if (msg)
> -    scm_misc_error (NULL, msg, SCM_EOL);
> -  return SCM_BOOL_T;
> +  if (! (SCM_UNBNDP (cond)))
> +    {
> +      SCM_VALIDATE_CONDVAR (2, cond);
> +      scm_t_timespec cwaittime, *waittime = NULL;
> +
> +      if (! (SCM_UNBNDP (timeout)))
> +     {
> +       cwaittime = scm_to_timespec (timeout);
> +       waittime = &cwaittime;
> +     }
> +      if (! fat_cond_timedwait (cond, mx, waittime))
> +     ret = SCM_BOOL_F;
> +    }

Call scm_timed_wait_condition_variable() here, instead of duplicating
the code?

Actually, that strongly says to me that we don't need the `cond' part
of this API to be implemented in C.  Can we move that to the SRFI-18
Scheme code, and leave the C API as a plain unlock-mutex operation?

Regards,
       Neil





reply via email to

[Prev in Thread] Current Thread [Next in Thread]