Hi Neil,
Neil Jerram <n...@ossau.uklinux.net> 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'.
