Re: the state of the concurrency branch

[Stefan Monnier]

> 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;