[Top][All Lists]
[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 (¤t_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
- Re: srfi-18 requirements, Neil Jerram, 2008/02/02
- Re: srfi-18 requirements, Julian Graham, 2008/02/05
- Re: srfi-18 requirements, Neil Jerram, 2008/02/07
- Re: srfi-18 requirements, Julian Graham, 2008/02/07
- Re: srfi-18 requirements, Julian Graham, 2008/02/11
- Re: srfi-18 requirements,
Neil Jerram <=
- Re: srfi-18 requirements, Julian Graham, 2008/02/19
- Re: srfi-18 requirements, Neil Jerram, 2008/02/21
- Re: srfi-18 requirements, Julian Graham, 2008/02/21
- Re: srfi-18 requirements, Neil Jerram, 2008/02/24
- Re: srfi-18 requirements, Julian Graham, 2008/02/24
- Re: srfi-18 requirements, Neil Jerram, 2008/02/24