guile-devel
[Top][All Lists]
Advanced

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

Re: Locks and threads


From: Ludovic Courtès
Subject: Re: Locks and threads
Date: Thu, 26 Mar 2009 10:10:36 +0100
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/23.0.90 (gnu/linux)

Hi Neil,

Neil Jerram <address@hidden> writes:

> - it runs for a duration specified by environment variable
>   GUILE_TEST_DEFINE_RACE_DURATION
>
> - it defaults to just 10s if that variable isn't defined.

Even 10s is going to be slightly annoying.  Another option would be to
create tens of guile-definer threads instead of just 4; it might trigger
the problem earlier.

> #2 makes the symbols hash thread-safe, and it appears that this
> completely fixes the define-race problem.  I don't understand why we
> apparently don't need patch #3 as well - but that's what my results
> indicate.

Roughly #2 is a specialized version of #3 as it addresses only the case
of the symbol hash table.  So it should be enough for this very problem.

#3 is probably the way to go in the longer run, but it changes the
API/ABI so it can't go in 1.8.

I'll skip #3 for now.

> * test-suite/standalone/test-define-race.c: New test.

Could you run `indent' on this file?

> +TESTS += test-define-race

It needs to be conditionalized on `--with-threads'.

>  scm_i_rehash (SCM table,
>             unsigned long (*hash_fn)(),
>             void *closure,
> -           const char* func_name)
> +           const char* func_name,
> +           scm_i_pthread_mutex_t *mutex)

This function assumes that MUTEX is locked.  I would either write it
prominently in a comment above or change the semantics so that MUTEX is
acquired in `scm_i_rehash ()', not in the caller.

The latter might be achieved by integrating tests about whether TABLE
needs to be rehashed into `scm_i_rehash ()' itself.  I.e., this:

  if (SCM_HASHTABLE_N_ITEMS (table) < SCM_HASHTABLE_LOWER (table)
      || SCM_HASHTABLE_N_ITEMS (table) > SCM_HASHTABLE_UPPER (table))
    scm_i_rehash (...);

would become:

  scm_i_rehash (...);

I haven't reviewed all uses to see whether it's actually possible and
whether it would lead to race conditions, though.

> -static SCM symbols;
> +SCM scm_i_symbols;

I don't think this is needed, is it?

> +static SCM
> +intern_symbol (SCM symbol, const char *name, size_t len, size_t raw_hash)

Since all users of `intern_symbol ()' perform a
`lookup_interned_symbol ()' right before, I'd rather change users so
that they lock before `lookup' and unlock after `intern':

  lock (&symbols_mutex);

  sym = lookup_interned_symbol (name);
  if (scm_is_false (sym))
    {
      sym = scm_i_c_make_symbol (...);
      intern_symbol (sym);
    }

  unlock (&symbols_mutex);

Is it doable?

Thanks,
Ludo'.





reply via email to

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