Re: Locks and threads

[Ludovic Courtès]

Hi Neil,

Neil Jerram <address@hidden> writes:

> That's an interesting idea, but to be honest I'd prefer to finish this
> work and move onto something else.  Would it be OK just to reduce the
> default runtime to 2 seconds?  (It wouldn't make any practical
> difference, because 10 seconds isn't normally enough to see a define
> race anyway.  It'll just be enough to catch any obvious bitrotting,
> and people who want to run a real test can use
> GUILE_TEST_DEFINE_RACE_DURATION.)

Yes, that'd be OK.

> The key points about #2 are that it uses a straight pthreads mutex to
> protect the symbol hash, and that the symbol hash is a weak hash
> table.  Using a pthreads mutex means that we have to take care not to
> do any allocations (or anything else that could block) while the mutex
> is locked.  The weakness means that as well as the obvious lookup and
> intern operations, we have to allow for the hash table being accessed
> and resized from an after-GC hook.

OK, got it. (A limitation that vanishes with BDW-GC.)

> #3, on the other hand, uses a fat mutex, and can be applied to any
> non-weak hash table.  Using a fat mutex means that it's fine to
> allocate or block while the mutex is locked, and means that the code
> can use scm_dynwind_lock_mutex, which is quite neat.  On the other
> hand, it would cause a problem if the hash table concerned could be
> accessed during GC, because
>
> - if the GC didn't try to lock the fat mutex itself, it would be
>   modifying the hash table under the feet of the thread that has the
>   mutex locked
>
> - if the GC does try to lock the fat mutex, it won't be able to and so
>   will block... deadlock!
>
> Hence the restriction of #3 to non-weak hash tables.

OK.

> If #3 was needed, I wouldn't see any problem with its API/ABI changes.
> The API change is addition of one new primitive, and the ABI change is
> to a structure that isn't intended for user code to access.  Am I
> missing something?

I was concerned about the `scm_t_hashtable' change, but it doesn't
appear to be accessed by public inlines/macros, so that should be OK.

> On the other hand, if it isn't needed - as appears to be true -
> there's no case for adding it to 1.8.

Agreed.

>> It needs to be conditionalized on `--with-threads'.
>
> It's already inside an "if BUILD_PTHREAD_SUPPORT".  Do I need to do
> more than that?

No, I had overlooked this.

>>>  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.
>
> I think I prefer the prominent comment.  My first thought was for
> scm_i_rehash () to acquire the mutex.  I changed to the current code
> because then all of the lock..unlock pairs are in symbols.c - which
> seemed more consistent to me.

Fair enough.

>> 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.
>
> I think that could work, but it would add another lock/unlock to the
> mainline.  (We need to hold the lock even just to evaluate the
> rehashing condition.)  The change doesn't feel compelling enough to me
> to justify that.

Contention-free locks are "cheap" in that there's no syscall involved
(when using futexes); OTOH there's at least one function call, which
we'd rather avoid.

>>> -static SCM symbols;
>>> +SCM scm_i_symbols;
>>
>> I don't think this is needed, is it?
>
> Yes, so that rehash_after_gc () (in hashtab.c) can identify the symbol
> hash and call scm_i_rehash_symbols_after_gc () for it.

Oops, right.

>>> +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?
>
> The problem with this is allocation (scm_i_c_make_symbol) while
> holding the mutex.  The overall arrangement of the symbol code is
>
>   lookup - allocate - intern
>
> and I assume that is so as to avoid allocation in the case where the
> symbol is already interned; otherwise the arrangement could be
>
>   allocate - lookup/intern
>
> with the lookup/intern being implemented as a single operation.
>
> I've made my changes so as to leave this overall pattern the same.
> The code certainly could be simpler if we always allocated upfront,
> but I think it is better (for performance) not to do that.

Yes, you're right.  I hadn't grasped all the implications here.

One nitpick: `intern_symbol ()' shouldn't need NAME, LEN and RAW_HASH as
it can get them "for free" from SYMBOL.

> On the other hand, I suspect the code could still be simplified to
> remove the duplication between lookup_interned_symbol and
> intern_symbol.  I'll have a look at that.

Possible.

Thanks for looking into this!

Ludo'.