From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!not-for-mail From: Stefan Monnier Newsgroups: gmane.emacs.devel Subject: Re: the state of the concurrency branch Date: Mon, 26 Aug 2013 17:29:03 -0400 Message-ID: References: <87vc2t7erx.fsf@fleche.redhat.com> <83txicffpe.fsf@gnu.org> NNTP-Posting-Host: plane.gmane.org Mime-Version: 1.0 Content-Type: text/plain X-Trace: ger.gmane.org 1377552570 6280 80.91.229.3 (26 Aug 2013 21:29:30 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Mon, 26 Aug 2013 21:29:30 +0000 (UTC) Cc: tromey@redhat.com, Juanma Barranquero , emacs-devel@gnu.org To: Eli Zaretskii Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Mon Aug 26 23:29:30 2013 Return-path: Envelope-to: ged-emacs-devel@m.gmane.org Original-Received: from lists.gnu.org ([208.118.235.17]) by plane.gmane.org with esmtp (Exim 4.69) (envelope-from ) id 1VE4Lp-0007Kv-KJ for ged-emacs-devel@m.gmane.org; Mon, 26 Aug 2013 23:29:29 +0200 Original-Received: from localhost ([::1]:53119 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VE4Lp-0003xq-4l for ged-emacs-devel@m.gmane.org; Mon, 26 Aug 2013 17:29:29 -0400 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:52683) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VE4Lg-0003x3-1v for emacs-devel@gnu.org; Mon, 26 Aug 2013 17:29:27 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VE4LY-0004uq-Mc for emacs-devel@gnu.org; Mon, 26 Aug 2013 17:29:19 -0400 Original-Received: from ironport2-out.teksavvy.com ([206.248.154.182]:58641) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VE4LQ-0004t6-RU; Mon, 26 Aug 2013 17:29:04 -0400 X-IronPort-Anti-Spam-Filtered: true X-IronPort-Anti-Spam-Result: Av8EABK/CFHO+KK6/2dsb2JhbABEuzWDWRdzgh4BAQQBJy8jBQsLNBIUGA0kiB4GwS2RCgOkeoFegxM X-IPAS-Result: Av8EABK/CFHO+KK6/2dsb2JhbABEuzWDWRdzgh4BAQQBJy8jBQsLNBIUGA0kiB4GwS2RCgOkeoFegxM X-IronPort-AV: E=Sophos;i="4.84,565,1355115600"; d="scan'208";a="24484259" Original-Received: from 206-248-162-186.dsl.teksavvy.com (HELO ceviche.home) ([206.248.162.186]) by ironport2-out.teksavvy.com with ESMTP/TLS/ADH-AES256-SHA; 26 Aug 2013 17:28:56 -0400 Original-Received: by ceviche.home (Postfix, from userid 20848) id 32FBC66091; Mon, 26 Aug 2013 17:29:03 -0400 (EDT) In-Reply-To: <83txicffpe.fsf@gnu.org> (Eli Zaretskii's message of "Mon, 26 Aug 2013 21:51:41 +0300") User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3.50 (gnu/linux) X-detected-operating-system: by eggs.gnu.org: Genre and OS details not recognized. X-Received-From: 206.248.154.182 X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: "Emacs development discussions." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Original-Sender: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Xref: news.gmane.org gmane.emacs.devel:163042 Archived-At: > In general, I'd be grateful if Someone(TM) could explain the main > changes in process.c, as it's pretty basic stuff, and some of the > changes would originally not even compile, because some variables > involved in the changes were declared conditionally. I would also welcome some documentation of the implementation. I see that the thread-scoping of let-bindings is obtained by unwinding&rewinding the specpdl stack during context switches. That should be mentioned in some file/comment somewhere (probably thread.c?). Same for the handling of other per-thread variables (gcprolist, catchlist, current_buffer, ...). Also some comment is needed around flush_stack_call_func. And of course process.c needs a fair bit of comments since the code seems to have changed substantially, and I wasn't able to figure out what was the driving design behind it. See below some notes (in the form of a patch) I took while browsing through your patch. Stefan === modified file 'lisp/subr.el' --- lisp/subr.el 2013-08-20 03:53:07 +0000 +++ lisp/subr.el 2013-08-26 20:09:21 +0000 @@ -4795,14 +4795,18 @@ When CONDITION is signalled, check TEST again. This is the simplest safe way to invoke `condition-wait'." + ;; FIXME: How can this work? I mean, OK when it returns the test is + ;; satisfied, but we don't have the mutex any more, so it can be + ;; changed again at any time. Don't we need an "&rest body" to run + ;; some code once the test is satisfied and while we still hold + ;; the mutex? + ;; IOW, I'm not sure this is stable enough to be in subr.el. (let ((cond-sym (make-symbol "condition"))) `(let ((,cond-sym ,condition)) (with-mutex (condition-mutex ,cond-sym) (while (not ,test) (condition-wait ,cond-sym)))))) - - ;;; Misc. (defconst menu-bar-separator '("--") "Separator for menus.") === modified file 'src/buffer.c' --- src/buffer.c 2013-08-20 03:53:07 +0000 +++ src/buffer.c 2013-08-26 20:51:45 +0000 @@ -1723,6 +1723,7 @@ return Qnil; if (thread_check_current_buffer (b)) + /* FIXME: We should emit a message/warning here. */ return Qnil; /* Run hooks with the buffer to be killed the current buffer. */ === modified file 'src/data.c' --- src/data.c 2013-08-20 03:53:07 +0000 +++ src/data.c 2013-08-26 20:52:39 +0000 @@ -545,20 +545,14 @@ doc: /* Return t if OBJECT is a thread. */) (Lisp_Object object) { - if (THREADP (object)) - return Qt; - else - return Qnil; + return (THREADP (object) ? Qt : Qnil); } DEFUN ("mutexp", Fmutexp, Smutexp, 1, 1, 0, doc: /* Return t if OBJECT is a mutex. */) (Lisp_Object object) { - if (MUTEXP (object)) - return Qt; - else - return Qnil; + return (MUTEXP (object) ? Qt : Qnil); } DEFUN ("condition-variable-p", Fcondition_variable_p, Scondition_variable_p, @@ -566,10 +560,7 @@ doc: /* Return t if OBJECT is a condition variable. */) (Lisp_Object object) { - if (CONDVARP (object)) - return Qt; - else - return Qnil; + return (CONDVARP (object) ? Qt : Qnil); } /* Extract and set components of lists. */ === modified file 'src/eval.c' --- src/eval.c 2013-08-20 03:53:07 +0000 +++ src/eval.c 2013-08-26 20:54:04 +0000 @@ -1119,6 +1119,7 @@ c.next = catchlist; c.tag = tag; c.val = Qnil; + /* FIXME: Why "f_"? */ c.f_handlerlist = handlerlist; c.f_lisp_eval_depth = lisp_eval_depth; c.pdlcount = SPECPDL_INDEX (); @@ -3171,14 +3172,6 @@ return 0; } -static Lisp_Object -binding_symbol (union specbinding *bind) -{ - if (!CONSP (specpdl_symbol (bind))) - return specpdl_symbol (bind); - return XCAR (specpdl_symbol (bind)); -} - void do_specbind (struct Lisp_Symbol *sym, union specbinding *bind, Lisp_Object value) @@ -3209,7 +3202,7 @@ } } - set_internal (binding_symbol (bind), value, Qnil, 1); + set_internal (specpdl_symbol (bind), value, Qnil, 1); break; default: @@ -3253,6 +3246,8 @@ specpdl_ptr->let.old_value = SYMBOL_VAL (sym); specpdl_ptr->let.saved_value = Qnil; grow_specpdl (); + /* FIXME: Are we really sure the compiler will turn this + into the same code as what we had before? */ do_specbind (sym, specpdl_ptr - 1, value); break; case SYMBOL_LOCALIZED: @@ -3286,6 +3281,8 @@ { specpdl_ptr->let.kind = SPECPDL_LET_DEFAULT; grow_specpdl (); + /* FIXME: This was Fset_default (symbol, value) and + I don't see why this new code is equivalent. */ do_specbind (sym, specpdl_ptr - 1, value); return; } @@ -3294,6 +3291,8 @@ specpdl_ptr->let.kind = SPECPDL_LET; grow_specpdl (); + /* FIXME: Are we really sure the compiler will turn this + into the same code as what we had before? */ do_specbind (sym, specpdl_ptr - 1, value); break; } @@ -3350,7 +3349,7 @@ Lisp_Object value = specpdl_saved_value (bind); bind->let.saved_value = Qnil; - do_specbind (XSYMBOL (binding_symbol (bind)), bind, value); + do_specbind (XSYMBOL (specpdl_symbol (bind)), bind, value); } } } @@ -3497,10 +3496,11 @@ union specbinding *bind; for (bind = specpdl_ptr; bind != specpdl; --bind) - { + { /* FIXME: I think this makes assumptions that are not compatible + with the hack used in backtrace_eval_unrewind. */ if (bind->kind >= SPECPDL_LET) { - bind->let.saved_value = find_symbol_value (binding_symbol (bind)); + bind->let.saved_value = find_symbol_value (specpdl_symbol (bind)); do_one_unbind (bind, 0); } } === modified file 'src/lisp.h' --- src/lisp.h 2013-08-25 20:25:59 +0000 +++ src/lisp.h 2013-08-26 21:03:23 +0000 @@ -535,6 +535,7 @@ ptrdiff_t size; }; +/* FIXME: Including thread.h here is odd, we normally don't do that. */ #include "thread.h" /* Convert a Lisp_Object to the corresponding EMACS_INT and vice versa. === modified file 'src/search.c' --- src/search.c 2013-08-20 03:53:07 +0000 +++ src/search.c 2013-08-26 21:15:38 +0000 @@ -42,6 +42,7 @@ struct regexp_cache { struct regexp_cache *next; + /* FIXME: Why "f_"? */ Lisp_Object regexp, f_whitespace_regexp; /* Syntax table for which the regexp applies. We need this because of character classes. If this is t, then the compiled pattern is valid === modified file 'src/thread.c' --- src/thread.c 2013-08-26 14:53:26 +0000 +++ src/thread.c 2013-08-26 21:23:54 +0000 @@ -804,6 +804,8 @@ return thread_alive_p (tstate) ? Qt : Qnil; } +/* FIXME: I'd prefer to give it a double-dashed name, since it sounds like + something that shouldn't be used in a normal program. */ DEFUN ("thread-blocker", Fthread_blocker, Sthread_blocker, 1, 1, 0, doc: /* Return the object that THREAD is blocking on. If THREAD is blocked in `thread-join' on a second thread, return that @@ -882,7 +884,7 @@ -int +bool thread_check_current_buffer (struct buffer *buffer) { struct thread_state *iter; @@ -893,10 +895,10 @@ continue; if (iter->m_current_buffer == buffer) - return 1; + return true; } - return 0; + return false; } === modified file 'src/thread.h' --- src/thread.h 2013-07-07 05:18:58 +0000 +++ src/thread.h 2013-08-26 21:21:27 +0000 @@ -24,6 +24,8 @@ #include "sysselect.h" /* FIXME */ #include "systime.h" /* FIXME */ +/* FIXME: Why "m_"? */ + struct thread_state { struct vectorlike_header header; @@ -199,6 +201,7 @@ { struct vectorlike_header header; + /* FIXME: Why give it a name? */ /* The name of the mutex, or nil. */ Lisp_Object name; @@ -214,6 +217,7 @@ /* The associated mutex. */ Lisp_Object mutex; + /* FIXME: Why give it a name? */ /* The name of the condition variable, or nil. */ Lisp_Object name; === modified file 'src/window.c' --- src/window.c 2013-08-25 20:25:59 +0000 +++ src/window.c 2013-08-26 21:24:58 +0000 @@ -5398,6 +5398,7 @@ struct vectorlike_header header; Lisp_Object selected_frame; Lisp_Object current_window; + /* Why "f_"? */ Lisp_Object f_current_buffer; Lisp_Object minibuf_scroll_window; Lisp_Object minibuf_selected_window; @@ -5414,7 +5415,7 @@ int frame_tool_bar_lines; }; -/* This is saved as a Lisp_Vector */ +/* This is saved as a Lisp_Vector. */ struct saved_window { struct vectorlike_header header;