From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!not-for-mail From: ludo@gnu.org (Ludovic =?iso-8859-1?Q?Court=E8s?=) Newsgroups: gmane.lisp.guile.devel Subject: Re: Locks and threads Date: Fri, 27 Mar 2009 00:12:52 +0100 Message-ID: <87tz5fho97.fsf@gnu.org> References: <87mycsd2rj.fsf@arudy.ossau.uklinux.net> <87vdqovofz.fsf@arudy.ossau.uklinux.net> <87vdqhc4oi.fsf@arudy.ossau.uklinux.net> <87mybrwqmj.fsf@arudy.ossau.uklinux.net> <87iqmfwohh.fsf@arudy.ossau.uklinux.net> <871vt1w81o.fsf@arudy.ossau.uklinux.net> <87zlf9qjgb.fsf@arudy.ossau.uklinux.net> <87bprotzsj.fsf@gnu.org> <87myb8q6yx.fsf@arudy.ossau.uklinux.net> NNTP-Posting-Host: lo.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: 8bit X-Trace: ger.gmane.org 1238109212 12005 80.91.229.12 (26 Mar 2009 23:13:32 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Thu, 26 Mar 2009 23:13:32 +0000 (UTC) To: guile-devel@gnu.org Original-X-From: guile-devel-bounces+guile-devel=m.gmane.org@gnu.org Fri Mar 27 00:14:49 2009 Return-path: Envelope-to: guile-devel@m.gmane.org Original-Received: from lists.gnu.org ([199.232.76.165]) by lo.gmane.org with esmtp (Exim 4.50) id 1LmymZ-00087P-RM for guile-devel@m.gmane.org; Fri, 27 Mar 2009 00:14:44 +0100 Original-Received: from localhost ([127.0.0.1]:55519 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1LmylC-0003LU-KQ for guile-devel@m.gmane.org; Thu, 26 Mar 2009 19:13:18 -0400 Original-Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1Lmyl6-0003LP-2j for guile-devel@gnu.org; Thu, 26 Mar 2009 19:13:12 -0400 Original-Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1Lmyl0-0003LB-H5 for guile-devel@gnu.org; Thu, 26 Mar 2009 19:13:10 -0400 Original-Received: from [199.232.76.173] (port=50761 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Lmyl0-0003L8-Ax for guile-devel@gnu.org; Thu, 26 Mar 2009 19:13:06 -0400 Original-Received: from main.gmane.org ([80.91.229.2]:43405 helo=ciao.gmane.org) by monty-python.gnu.org with esmtps (TLS-1.0:RSA_AES_256_CBC_SHA1:32) (Exim 4.60) (envelope-from ) id 1Lmykz-0003MG-P9 for guile-devel@gnu.org; Thu, 26 Mar 2009 19:13:06 -0400 Original-Received: from list by ciao.gmane.org with local (Exim 4.43) id 1Lmykw-0001fh-Pv for guile-devel@gnu.org; Thu, 26 Mar 2009 23:13:02 +0000 Original-Received: from reverse-83.fdn.fr ([80.67.176.83]) by main.gmane.org with esmtp (Gmexim 0.1 (Debian)) id 1AlnuQ-0007hv-00 for ; Thu, 26 Mar 2009 23:13:02 +0000 Original-Received: from ludo by reverse-83.fdn.fr with local (Gmexim 0.1 (Debian)) id 1AlnuQ-0007hv-00 for ; Thu, 26 Mar 2009 23:13:02 +0000 X-Injected-Via-Gmane: http://gmane.org/ Original-Lines: 163 Original-X-Complaints-To: usenet@ger.gmane.org X-Gmane-NNTP-Posting-Host: reverse-83.fdn.fr X-URL: http://www.fdn.fr/~lcourtes/ X-Revolutionary-Date: 7 Germinal an 217 de la =?iso-8859-1?Q?R=E9volution?= X-PGP-Key-ID: 0xEA52ECF4 X-PGP-Key: http://www.fdn.fr/~lcourtes/ludovic.asc X-PGP-Fingerprint: 821D 815D 902A 7EAB 5CEE D120 7FBA 3D4F EB1F 5364 X-OS: i686-pc-linux-gnu User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/23.0.90 (gnu/linux) Cancel-Lock: sha1:qc1SZklb+z6OkFJmCHcf3XpSS7U= X-detected-operating-system: by monty-python.gnu.org: GNU/Linux 2.6, seldom 2.4 (older, 4) X-BeenThere: guile-devel@gnu.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: "Developers list for Guile, the GNU extensibility library" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Original-Sender: guile-devel-bounces+guile-devel=m.gmane.org@gnu.org Errors-To: guile-devel-bounces+guile-devel=m.gmane.org@gnu.org Xref: news.gmane.org gmane.lisp.guile.devel:8334 Archived-At: Hi Neil, Neil Jerram 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'.