the state of the concurrency branch

the state of the concurrency branch

[Tom Tromey]

I thought I'd send a note about the concurrency branch.

Referring back to my to-do list from this time last year:

    http://lists.gnu.org/archive/html/emacs-devel/2012-08/msg00324.html

I've implemented what I consider the must-have items from that list:

* Documentation
* Condition variables
* Process changes
* Thread switch on select

I've also been periodically merging from trunk and running the test
suite.  This hasn't been too bad; though the changes to specpdl this
year caused a few hiccups.

I've looked a little bit into making it so "emacsclient -t" runs the new
terminal in its own thread.  This is a bit trickier than I thought it
would be; I think it means changing top-level so that it can be entered
by multiple threads, plus making it so that the terminal's fd is
thread-locked.  So, I haven't done this yet.

I haven't written ChangeLog entries yet.


What would it take to merge the branch to trunk?

Tom

Re: the state of the concurrency branch

[Paul Eggert]

Tom Tromey wrote:
> What would it take to merge the branch to trunk?

I don't know, but here are a few questions off
the top of my head:

* How do you debug?  Both Elisp code, and the C substrate.

* What are the implications of combining this with
  the Guile version of Emacs that Brad is working on?

* Is it governed by a --with-something-or-other
  flag at configure-time?  Would make it easier to sell,
  I expect.

Re: the state of the concurrency branch

[Tom Tromey]

Paul> * How do you debug?  Both Elisp code, and the C substrate.

In C it is just as easy as anything else.

For elisp, yeah, that's a problem.  There really isn't a decent way, I
think.  Just printfs.

Paul> * What are the implications of combining this with
Paul>   the Guile version of Emacs that Brad is working on?

You'd have to ask him.

Paul> * Is it governed by a --with-something-or-other
Paul>   flag at configure-time?  Would make it easier to sell,
Paul>   I expect.

It's unconditional.  If you want it to be optional, could you say why?
I don't think that would be particularly useful.

It hasn't been ported to many hosts yet.  That isn't something I can do.

Tom

Re: the state of the concurrency branch

[Paul Eggert]

Tom Tromey wrote:
> If you want it to be optional, could you say why?

It's going to need to be optional anyway, for
the benefit of hosts that don't support threads,
so it might as well be a knob that the builder
can turn.

Re: the state of the concurrency branch

[Tom Tromey]

Tom> If you want it to be optional, could you say why?

Paul> It's going to need to be optional anyway, for
Paul> the benefit of hosts that don't support threads,
Paul> so it might as well be a knob that the builder
Paul> can turn.

I've implemented this on the branch now.

Tom

Re: the state of the concurrency branch

[Tom Tromey]

Paul> * How do you debug?  Both Elisp code, and the C substrate.
Tom> In C it is just as easy as anything else.

Tom> For elisp, yeah, that's a problem.  There really isn't a decent way, I
Tom> think.  Just printfs.

Let me rephrase that a bit.

Right now I think nothing prevents any other thread from starting the
debugger.  So maybe "non-main" threads can be debugged in the ordinary
way.  I am not totally sure -- I will try it.

Tom

Re: the state of the concurrency branch

[Stefan Monnier]

> I've also been periodically merging from trunk and running the test
> suite.

Thanks.

> This hasn't been too bad; though the changes to specpdl this
> year caused a few hiccups.

I'd have gladly helped.

> What would it take to merge the branch to trunk?

It sounds like it's close to ready for merge, indeed.
I'll take a closer look ASAP, but normally for such "core" changes, my
main concerns are:
- Backward compatibility: it's OK if some things break when you start
  using the new feature, but things should work as before as long as you
  don't use it.  Usually, to be convinced this is the case, the code
  should make it obvious.  The simplest way is sometimes conditional
  compilation, where we get back "the exact same code" as before if the
  flag is not set, but if we can get the "obvious backward
  compatibility" without resorting the conditional compilation, it's
  even better.
- The general design looks sound, so there's a good chance we'll be able
  to fix the problems that will show up without having to scrap it all
  and start over.
  

        Stefan

Re: the state of the concurrency branch

[Juanma Barranquero]

On Mon, Aug 26, 2013 at 6:48 PM, Stefan Monnier
<monnier@iro.umontreal.ca> wrote:

>> What would it take to merge the branch to trunk?
>
> It sounds like it's close to ready for merge, indeed.

Well, it breaks the Windows build.

gcc  -std=gnu99 -c -mtune=pentium4  -Ic:/Devel/emacs/include
-DUSE_CRT_DLL=1 -I /c/Devel/emacs/repo/concurrency/nt/inc -Demacs  -I.
-I/c/Devel/emacs/repo/concurrency/src  -I../lib
-I/c/Devel/emacs/repo/concurrency/src/../lib -mtune=pentium4
-Ic:/Devel/emacs/build/include/libxml2   -MMD -MF deps/vm-limit.d -MP
-Ic:/Devel/emacs/build/include
-Ic:/Devel/emacs/build/include/p11-kit-1       -O0 -g3  vm-limit.c
In file included from lisp.h:538:0,
                 from vm-limit.c:21:
thread.h:235:31: error: unknown type name 'SELECT_TYPE'
thread.h:235:46: error: unknown type name 'SELECT_TYPE'
thread.h:235:61: error: unknown type name 'SELECT_TYPE'
thread.h:238:21: error: unknown type name 'select_func'
thread.h:238:53: error: unknown type name 'SELECT_TYPE'
thread.h:239:7: error: unknown type name 'SELECT_TYPE'
thread.h:239:26: error: unknown type name 'SELECT_TYPE'
Makefile:358: recipe for target `vm-limit.o' failed
make[1]: *** [vm-limit.o] Error 1
make[1]: Leaving directory `/c/Devel/emacs/repo/concurrency/src'

Adding

#ifdef WINDOWSNT
#include "w32.h"
#endif

to thread.h to add the SELECT_TYPE declaration fixes that, but there
are new breakages a little further. So it is not close to ready for
merge.

   J

Re: the state of the concurrency branch

[Tom Tromey]

Juanma> Well, it breaks the Windows build.

Not a huge surprise to me.
I don't have a Windows machine.

Juanma> to thread.h to add the SELECT_TYPE declaration fixes that, but there
Juanma> are new breakages a little further. So it is not close to ready for
Juanma> merge.

Please send me the "make -k" build log and I will see what I can do.

Tom

Re: the state of the concurrency branch

[Eli Zaretskii]

> From: Juanma Barranquero <lekktu@gmail.com>
> Date: Mon, 26 Aug 2013 19:04:46 +0200
> Cc: Tom Tromey <tromey@redhat.com>, Emacs discussions <emacs-devel@gnu.org>
> 
> On Mon, Aug 26, 2013 at 6:48 PM, Stefan Monnier
> <monnier@iro.umontreal.ca> wrote:
> 
> >> What would it take to merge the branch to trunk?
> >
> > It sounds like it's close to ready for merge, indeed.
> 
> Well, it breaks the Windows build.

I fixed the compilation, but the stuff in process.c needs work: if you
invoke "M-x grep", Emacs aborts after Grep exits, here:

  void
  delete_read_fd (int fd)
  {
    eassert (fd < MAXDESC);
    eassert (fd <= max_desc); <<<<<<<<<<<<<<<<<

because max_desc is zero.

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.  Since I couldn't
grasp the rationale for the changes in the few minutes I had to spare,
I'm not sure I fixed the code correctly.

Finally, there's a warning in regex.c:

  regex.c: In function `re_set_whitespace_regexp':
  regex.c:1250: warning: assignment discards qualifiers from pointer target type

but I don't think it's Windows specific.

In sum, I'd say this branch still "needs work".

Thanks.

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))))))
 
-
-\f
 ;;; 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);
 }
 \f
 /* 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 @@
 
 \f
 
-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;
 }
 
 \f

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

Re: the state of the concurrency branch

[Tom Tromey]

Stefan> I would also welcome some documentation of the implementation.

Ok.  I will see what I can write up.

I've tried to answer some things here as well.

Stefan> I see that the thread-scoping of let-bindings is obtained by
Stefan> unwinding&rewinding the specpdl stack during context switches.
Stefan> That should be mentioned in some file/comment somewhere (probably
Stefan> thread.c?).  Same for the handling of other per-thread variables
Stefan> (gcprolist, catchlist, current_buffer, ...).

Ok.  I can at least add a comment above struct thread_state explaining
the general approach.

I'll make the other comment additions you suggest.
I'll send some mail when I think it's ready for a re-read.

Stefan> And of course process.c needs a fair bit of comments since the code
Stefan> seems to have changed substantially, and I wasn't able to figure out
Stefan> what was the driving design behind it.

Ok.

The basic issue is that only one thread can select on a given fd at a
time.  This means we have to track which threads are currently selecting
on which fds; and also it means we must recompute the various select
masks dynamically.

Stefan>  This is the simplest safe way to invoke `condition-wait'."
Stefan> +  ;; FIXME: How can this work?  I mean, OK when it returns the test is
Stefan> +  ;; satisfied, but we don't have the mutex any more, so it can be
Stefan> +  ;; changed again at any time.  Don't we need an "&rest body" to run
Stefan> +  ;; some code once the test is satisfied and while we still hold
Stefan> +  ;; the mutex?
Stefan> +  ;; IOW, I'm not sure this is stable enough to be in subr.el.
Stefan>    (let ((cond-sym (make-symbol "condition")))
Stefan>      `(let ((,cond-sym ,condition))
Stefan>         (with-mutex (condition-mutex ,cond-sym)
Stefan>           (while (not ,test)
Stefan>  	   (condition-wait ,cond-sym))))))

Yeah, I can zap that.  Not sure what I was thinking then.

Stefan> -  if (THREADP (object))
Stefan> -    return Qt;
Stefan> -  else
Stefan> -    return Qnil;
Stefan> +  return (THREADP (object) ? Qt : Qnil);

It's fine with me if you want to just check in things like this.

Stefan> --- src/eval.c	2013-08-20 03:53:07 +0000
Stefan> +++ src/eval.c	2013-08-26 20:54:04 +0000
Stefan> @@ -1119,6 +1119,7 @@
Stefan>    c.next = catchlist;
Stefan>    c.tag = tag;
Stefan>    c.val = Qnil;
Stefan> +  /* FIXME: Why "f_"?  */
Stefan>    c.f_handlerlist = handlerlist;

The field needs some different name, because "handlerlist" is now a
#define in thread.h.

If you don't like f_, pick something else and I will change it.

I think I picked "f_" for "field".

Stefan> +      /* FIXME: Are we really sure the compiler will turn this
Stefan> +	 into the same code as what we had before?  */

I'll look into these.

Stefan> === modified file 'src/lisp.h'
Stefan> --- src/lisp.h	2013-08-25 20:25:59 +0000
Stefan> +++ src/lisp.h	2013-08-26 21:03:23 +0000
Stefan> @@ -535,6 +535,7 @@
Stefan>      ptrdiff_t size;
Stefan>    };
 
Stefan> +/* FIXME: Including thread.h here is odd, we normally don't do that.  */
Stefan>  #include "thread.h"
 
Yeah.  The ordering is funky due to the #define hack.
This could be done a different way; but the define approach at least
makes merges simple; a big consideration for me given my extremely
limited free time... a more invasive change would make merges very hard.

Stefan> +/* FIXME: I'd prefer to give it a double-dashed name, since it sounds like
Stefan> +   something that shouldn't be used in a normal program.  */
Stefan>  DEFUN ("thread-blocker", Fthread_blocker, Sthread_blocker, 1, 1, 0,

Ok.

Stefan> -int
Stefan> +bool

Somehow I wasn't aware that Emacs used bool.
Is that new(-ish)?

Stefan> +/* FIXME: Why "m_"?  */

I don't recall why "m" in particular, but it does need some prefix due
to the #define approach.

Stefan> +  /* FIXME: Why give it a name?  */
Stefan>    /* The name of the mutex, or nil.  */
Stefan>    Lisp_Object name;

It's a debugging feature.  The name is optional for those who don't need it.

Tom

Re: the state of the concurrency branch

[Eli Zaretskii]

> From: Tom Tromey <tromey@redhat.com>
> Cc: Eli Zaretskii <eliz@gnu.org>, Juanma Barranquero <lekktu@gmail.com>,
>         emacs-devel@gnu.org
> Date: Mon, 26 Aug 2013 20:30:46 -0600
> 
> The basic issue is that only one thread can select on a given fd at a
> time.  This means we have to track which threads are currently selecting
> on which fds; and also it means we must recompute the various select
> masks dynamically.

What about the keyboard fd? is it selected by all threads, or just by
some?  If the latter, by which one(s)?

Also, what about the special inputs, such as file notifications etc.,
which currently just stuff some events into the keyboard queue -- how,
if at all, shall that change when more than one thread could be
running and watching those events?

And a related question: what about triggering redisplay, which is done
as part of waiting for some input -- will it be triggered by more than
one thread now?

> Stefan> --- src/lisp.h	2013-08-25 20:25:59 +0000
> Stefan> +++ src/lisp.h	2013-08-26 21:03:23 +0000
> Stefan> @@ -535,6 +535,7 @@
> Stefan>      ptrdiff_t size;
> Stefan>    };
>  
> Stefan> +/* FIXME: Including thread.h here is odd, we normally don't do that.  */
> Stefan>  #include "thread.h"
>  
> Yeah.  The ordering is funky due to the #define hack.

It was the main reason for breaking the Windows compilation, btw.  We
have now an unfortunate situation whereby lisp.h cannot be included
before some of the other headers, due to this.

> Stefan> +/* FIXME: Why "m_"?  */
> 
> I don't recall why "m" in particular

It probably stands for "member".  This is a widely used coding
convention, except that Emacs never used it -- until now.  We use foo_
in other places for similar reasons, perhaps we should do that here as
well, for consistency, if nothing else.

Here are a few more questions/comments, based on some reading of the
code.  Some of the below is relevant to how the infrastructure in
systhread.c might be ported to Windows.  In no particular order:

 . The compute_*_wait_mask functions in process.c could use some more
   meaningful names, to more clearly indicate their semantics from the
   caller's POV.  Right now, the names simply state which bits are
   tested in the fd's flags (and even that is not 100% accurate, since
   e.g. the FOR_READ flag is not mentioned).  That is hard on mnemonic
   memory, especially since some of the functions define their purpose
   in negative form (compute_NON_keyboard_wait_mask).

 . Why is systhread.c on a separate file?  Wouldn't it be better to
   have this code in thread.c instead?  It's not like thread.c can be
   compiled in without also compiling systhread.c, right?

 . Will the handling of SIGCHLD be thread-specific or global?  IOW, if
   a thread fires up a subprocess, which exits while another thread is
   running, which thread(s) will get the signal?  If the signal
   arrives at some other thread, how will that thread know to handle
   it, if it doesn't watch the corresponding fd's?

 . I'm not sure I understand the rationale for the synchronization
   primitives that were implemented.  AFAIU, we have mutexes, and we
   have condition variables, and their basic functionalities are
   exposed all the way to the Lisp level.  But mutexes are implemented
   as condition variables under the hood.  Why is that? is that only
   to be able to interrupt a wait for mutex without relinquishing the
   mutex?  If the latter, then what is the real difference between
   these two, since both condition variable is also released by
   signaling it?  (Btw, the lispref manual mentions
   'condition-signal', does it mean 'thread-signal' instead? there's
   no mention of 'condition-signal' anywhere else, AFAICS.)

 . The issue with signals to thread is unclear to me.  threads.texi
   says:

     @defun thread-signal thread error-symbol data
     Like @code{signal} (@pxref{Signaling Errors}), but the signal is
     delivered in the thread @var{thread}.  If @var{thread} is the current
     thread, then this just calls @code{signal} immediately.
     @code{thread-signal} will cause a thread to exit a call to
     @code{mutex-lock}, @code{condition-wait}, or @code{thread-join}.
     @end defun

   A call to 'signal' throws to a handler or to top level, but what
   does the latter mean in a thread, where (AFAIU) there's no top
   level? will the thread be forced to exit (there's a hint that "a
   thread cannot be exited, but [...] other threads can be signaled")?
   if not, what will happen to it, after it exits the blocking wait
   calls?  And how is this thread signaling a generalization of the
   current single-threaded 'signal'?

 . threads.texi mentions the "current thread", but never explains what
   that is.  For Lisp code, that's probably the thread which runs the
   code, but what, if anything, does "current thread" mean on the C
   level?  Since several threads could potentially be running at the
   same time, is there any meaning to talk about "current thread"
   except in the context of some Lisp code?

   And what about the main thread, btw, the one created when Emacs
   starts?  Is that thread "more equal than others", or is it just
   like the others?

 . If a thread dies (because the underlying thread implementation hits
   some fatal error) while it holds a mutex, will the mutex be
   released?

 . Finally, if condition variables are unavailable (they are a pain on
   Windows, since only the latest versions support them natively), is
   it still possible to have threads, just without some
   synchronization features?  Or are condition variables currently a
   must for the threading mechanism itself?

Well, that's enough for now.  Thanks in advance for reading and
commenting.

Re: the state of the concurrency branch

[Paul Eggert]

On 08/27/13 09:08, Eli Zaretskii wrote:
>  . Why is systhread.c on a separate file?  Wouldn't it be better to
>    have this code in thread.c instead?  It's not like thread.c can be
>    compiled in without also compiling systhread.c, right?

Good point, and more generally, we should redo the code
so that there's not a separate sys_* level above a pthread_ level,
as the extra level's complexity isn't buying us anything.
I'll work on a patch along those lines.

Re: the state of the concurrency branch

[Tom Tromey]

>>>>> "Paul" == Paul Eggert <eggert@cs.ucla.edu> writes:

Paul> On 08/27/13 09:08, Eli Zaretskii wrote:
>> . Why is systhread.c on a separate file?  Wouldn't it be better to
>> have this code in thread.c instead?  It's not like thread.c can be
>> compiled in without also compiling systhread.c, right?

Paul> Good point, and more generally, we should redo the code
Paul> so that there's not a separate sys_* level above a pthread_ level,
Paul> as the extra level's complexity isn't buying us anything.
Paul> I'll work on a patch along those lines.

I did this intentionally to separate out the system-dependent bits from
the lisp-level bits.  I think it makes the porting simpler and it
prevents system oddities from infiltrating the lisp layer.

This latter bit is important because the lisp layer has specific
semantics, and there isn't always a 1:1 mapping from a lisp construct to
the underlying OS construct.  E.g., lisp mutexes are interruptible,
whereas system ones are not.

Could you say what is wrong with it as it stands?

If you're concerned about the layer of indirection... well, first, don't
be, it hardly matters; but otherwise, you can just inline most of the
stuff in systhread.h if you really care.  That would be ok with me.

Tom

Re: the state of the concurrency branch

[Paul Eggert]

On 08/27/13 11:23, Tom Tromey wrote:
> Could you say what is wrong with it as it stands?

It's that there are two sets of functions
where there needs to be just one.  Where there's a need for a
separate level for the Lisp layer (e.g., the stuff in sys_thread_create)
that should be kept.  But there's no need to distinguish (say)
sys_thread_equal from pthread_equal.

If there's a need for Emacs to port to OS threads that are not
compatible with pthreads, we should use GNU Pth, rather than
reinventing the wheel.

> If you're concerned about the layer of indirection... well, first, don't
> be, it hardly matters; but otherwise, you can just inline most of the
> stuff in systhread.h if you really care.  That would be ok with me.

It's not the inefficiency I'm worried about, it's just that the extra
layer makes the code harder to read and understand, and doesn't
buy us anything.  But I hear what you're saying about the system-dependent
bits and will try to preserve that in the proposed patch.

> I presume the modified code will still leave a place for a
> non-pthreads implementation, right?

Sure, platforms without pthreads will still work.

Re: the state of the concurrency branch

[Tom Tromey]

Paul> If there's a need for Emacs to port to OS threads that are not
Paul> compatible with pthreads, we should use GNU Pth, rather than
Paul> reinventing the wheel.

If that's what Emacs maintainers want, it is fine by me.

The reason I chose the route I did is that I knew that Emacs would have
relatively minimal requirements from the thread layer.  So requiring a
whole new dependency just for this seemed like needless pain for
porters.  However, it isn't my pain, so I really don't care that much.

What is your plan for the no-threads case?

Tom

Re: the state of the concurrency branch

[Paul Eggert]

On 08/27/13 11:46, Tom Tromey wrote:
> What is your plan for the no-threads case?

Use the gnulib pthreads module.  It arranges for
"#include <pthreads.h" to work on all platforms, with
a no-op implementation for platforms that don't support
pthreads.  It's similar to what is in systhread.h now,
except it doesn't impose an extra naming layer.

Re: the state of the concurrency branch

[Eli Zaretskii]

> Date: Tue, 27 Aug 2013 11:52:58 -0700
> From: Paul Eggert <eggert@cs.ucla.edu>
> CC: Eli Zaretskii <eliz@gnu.org>, lekktu@gmail.com, 
>  monnier@iro.umontreal.ca, emacs-devel@gnu.org
> 
> On 08/27/13 11:46, Tom Tromey wrote:
> > What is your plan for the no-threads case?
> 
> Use the gnulib pthreads module.  It arranges for
> "#include <pthreads.h" to work on all platforms, with
> a no-op implementation for platforms that don't support
> pthreads.  It's similar to what is in systhread.h now,
> except it doesn't impose an extra naming layer.

I'd like to be at liberty to implement the few threads calls needed
for Emacs in a way that doesn't use pthreads ported to Windows.  Pth
doesn't seem to support Windows (and the latest release was in 2006,
not an encouraging sign).

Gnulib pthreads is OK for a no-op implementation, but that's not what
I have in mind.

Please make the changes to the current code keeping in mind a separate
non-pthread implementation for MS-Windows.  (I'm okay with adding a
separate file, say w32thread.c, which would emulate pthread calls, if
that's the model we want to follow exclusively.  Alternatively, we
could have a separate HAVE_PTHREADS and HAVE_W32THREADS branches of
the same few low-level functions.)

Re: the state of the concurrency branch

[Paul Eggert]

On 08/27/13 12:08, Eli Zaretskii wrote:
> I'm okay with adding a
> separate file, say w32thread.c, which would emulate pthread calls, if
> that's the model we want to follow exclusively

Yes, that sounds like a good idea; I'll compose a patch
along those lines.

Re: the state of the concurrency branch

[Eli Zaretskii]

> Date: Tue, 27 Aug 2013 11:05:01 -0700
> From: Paul Eggert <eggert@cs.ucla.edu>
> CC: Tom Tromey <tromey@redhat.com>, lekktu@gmail.com, 
>  monnier@iro.umontreal.ca, emacs-devel@gnu.org
> 
> we should redo the code so that there's not a separate sys_* level
> above a pthread_ level, as the extra level's complexity isn't buying
> us anything.

I presume the modified code will still leave a place for a
non-pthreads implementation, right?

Re: the state of the concurrency branch

[Tom Tromey]

Eli>  . Finally, if condition variables are unavailable (they are a pain on
Eli>    Windows, since only the latest versions support them natively), is
Eli>    it still possible to have threads, just without some
Eli>    synchronization features?  Or are condition variables currently a
Eli>    must for the threading mechanism itself?

I think it would be better if all platforms supported the same primitives.

Condition variables can be implemented in a number of ways.
libjava already has a Windows port you can lift from.

Tom

Re: the state of the concurrency branch

[Eli Zaretskii]

> From: Tom Tromey <tromey@redhat.com>
> Cc: monnier@iro.umontreal.ca, lekktu@gmail.com, emacs-devel@gnu.org
> Date: Tue, 27 Aug 2013 13:14:24 -0600
> 
> Eli>  . Finally, if condition variables are unavailable (they are a pain on
> Eli>    Windows, since only the latest versions support them natively), is
> Eli>    it still possible to have threads, just without some
> Eli>    synchronization features?  Or are condition variables currently a
> Eli>    must for the threading mechanism itself?
> 
> I think it would be better if all platforms supported the same primitives.

I agree.  The question was more an attempt to understand how deeply
does the threading implementation depend on Lisp mutexes and condition
variables.

> Condition variables can be implemented in a number of ways.
> libjava already has a Windows port you can lift from.

You mean libjava that is part of GCC?  Thanks, I will look at that.
(I already saw a few solutions, and have an idea of an implementation,
but the more ideas the better.)

Re: the state of the concurrency branch

[Tom Tromey]

>> I think it would be better if all platforms supported the same primitives.

Eli> I agree.  The question was more an attempt to understand how deeply
Eli> does the threading implementation depend on Lisp mutexes and condition
Eli> variables.

System-level condition variables are needed for lisp condition
variables, of course, but also for thread-join and thread-signal.  So
they are fairly fundamental.

>> Condition variables can be implemented in a number of ways.
>> libjava already has a Windows port you can lift from.

Eli> You mean libjava that is part of GCC?

Yeah.  This thread code is all vaguely Java-like, and libjava was ported
to Windows long ago.  I think most of the implementation is in
libjava/win32-threads.cc, but some bits may lie elsewhere, I forget the
details any more.

Tom

Re: the state of the concurrency branch

[Stefan Monnier]

> What about the keyboard fd? is it selected by all threads, or just by
> some?  If the latter, by which one(s)?

Ideally, every terminal should have its own thread, I think.  IIUC Tom
has tried something along these lines, but hasn't managed to make it
work yet.

> Also, what about the special inputs, such as file notifications etc.,
> which currently just stuff some events into the keyboard queue -- how,
> if at all, shall that change when more than one thread could be
> running and watching those events?

As noted elsewhere, these should not go through the keyboard queue (I
understand, they currently do, and there's no pending fix to move them
elsewhere, but I think it's important to keep it in mind when thinking
up a plan to fix those problems).

>    level?  Since several threads could potentially be running at the
>    same time, is there any meaning to talk about "current thread"
>    except in the context of some Lisp code?

The current code only allows a single thread running at a time.


        Stefan

Re: the state of the concurrency branch

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: Tom Tromey <tromey@redhat.com>,  lekktu@gmail.com,  emacs-devel@gnu.org
> Date: Tue, 27 Aug 2013 20:50:11 -0400
> 
> > What about the keyboard fd? is it selected by all threads, or just by
> > some?  If the latter, by which one(s)?
> 
> Ideally, every terminal should have its own thread, I think.

My question still stands if there's only one terminal in the session.

> The current code only allows a single thread running at a time.

Really?  How does it do that?

Anyway, if that's true, threads will be much less useful than they
could be (e.g., fetching a large newsgroup while editing would be
impossible, right?).  Also, the implementation of condition variables
could be much simpler and dumber than I imagined.

Re: the state of the concurrency branch

[Tom Tromey]

>> The current code only allows a single thread running at a time.

Eli> Really?  How does it do that?

Only one thread can run Lisp at a time.
Threads not running Lisp can do other things, though currently this amounts
to waiting for I/O or blocking on a mutex.
This is implemented using a global lock.

Eli> Anyway, if that's true, threads will be much less useful than they
Eli> could be (e.g., fetching a large newsgroup while editing would be
Eli> impossible, right?).  Also, the implementation of condition variables
Eli> could be much simpler and dumber than I imagined.

I am not sure whether it would work.
System level condition variables are used in thread-join.
This is a situation where the code is truly multi-threaded -- multiple
threads can be waiting to see another thread exit; they will
serialize only when re-entering Lisp.

Fetching while editing can work fine.

Tom

Re: the state of the concurrency branch

[Eli Zaretskii]

> Date: Wed, 28 Aug 2013 07:16:30 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: tromey@redhat.com, lekktu@gmail.com, emacs-devel@gnu.org
> 
> Anyway, if that's true, threads will be much less useful than they
> could be (e.g., fetching a large newsgroup while editing would be
> impossible, right?).                       ^^^^^^^^^^^^^

Should be "while running some CPU-intensive Lisp".

Re: the state of the concurrency branch

[Stefan Monnier]

>> Anyway, if that's true, threads will be much less useful than they
>> could be (e.g., fetching a large newsgroup while editing would be
>> impossible, right?).                       ^^^^^^^^^^^^^
> Should be "while running some CPU-intensive Lisp".

That indeed won't work.  True concurrency will take more work.


        Stefan

Re: the state of the concurrency branch

[Tom Tromey]

>> Should be "while running some CPU-intensive Lisp".

Stefan> That indeed won't work.  True concurrency will take more work.

Even that can work ok if the lisp code calls thread-yield or sit-for;
the latter is friendly to do anyhow to let processes make progress.

We also have an intermediate step available between what is on the
branch and true concurrency: we can make QUIT call thread-yield
periodically.  This is basically what Python does.

Tom

Re: the state of the concurrency branch

[Stefan Monnier]

> Even that can work ok if the lisp code calls thread-yield or sit-for;
> the latter is friendly to do anyhow to let processes make progress.

That's true.

> We also have an intermediate step available between what is on the
> branch and true concurrency: we can make QUIT call thread-yield
> periodically.  This is basically what Python does.

That might be a bit easier than true concurrency, but it will still take
a lot more work than the current concurrency work.


        Stefan

Re: the state of the concurrency branch

[Eli Zaretskii]

> Date: Tue, 27 Aug 2013 19:08:21 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: lekktu@gmail.com, monnier@iro.umontreal.ca, emacs-devel@gnu.org
> 
>  . Will the handling of SIGCHLD be thread-specific or global?  IOW, if
>    a thread fires up a subprocess, which exits while another thread is
>    running, which thread(s) will get the signal?  If the signal
>    arrives at some other thread, how will that thread know to handle
>    it, if it doesn't watch the corresponding fd's?

Please answer this question, if you can: I need the answer to analyze
what, if any, changes are needed in how Emacs on Windows handles
process demise and emulates SIGCHLD.  Currently, we watch all of the
subprocesses inside the 'pselect' call; the question is: should we
only watch those of them that are relevant to the thread that calls
'pselect'.

TIA

Re: the state of the concurrency branch

[Tom Tromey]

>> . Will the handling of SIGCHLD be thread-specific or global?  IOW, if
>> a thread fires up a subprocess, which exits while another thread is
>> running, which thread(s) will get the signal?  If the signal
>> arrives at some other thread, how will that thread know to handle
>> it, if it doesn't watch the corresponding fd's?

Eli> Please answer this question, if you can: I need the answer to analyze
Eli> what, if any, changes are needed in how Emacs on Windows handles
Eli> process demise and emulates SIGCHLD.  Currently, we watch all of the
Eli> subprocesses inside the 'pselect' call; the question is: should we
Eli> only watch those of them that are relevant to the thread that calls
Eli> 'pselect'.

I have to look into it and I haven't found the time.
I will try to get to it soon.
It's possible that it is just an oversight on my part; however I would
say that the goal is that if a process is locked to a particular thread
then that thread should also invoke the process sentinel; beyond that I
am less clear on the issue.

Tom

Re: the state of the concurrency branch

[Eli Zaretskii]

> From: Tom Tromey <tromey@redhat.com>
> Cc: lekktu@gmail.com, monnier@iro.umontreal.ca, emacs-devel@gnu.org
> Date: Wed, 28 Aug 2013 21:54:53 -0600
> 
> >> . Will the handling of SIGCHLD be thread-specific or global?  IOW, if
> >> a thread fires up a subprocess, which exits while another thread is
> >> running, which thread(s) will get the signal?  If the signal
> >> arrives at some other thread, how will that thread know to handle
> >> it, if it doesn't watch the corresponding fd's?
> 
> Eli> Please answer this question, if you can: I need the answer to analyze
> Eli> what, if any, changes are needed in how Emacs on Windows handles
> Eli> process demise and emulates SIGCHLD.  Currently, we watch all of the
> Eli> subprocesses inside the 'pselect' call; the question is: should we
> Eli> only watch those of them that are relevant to the thread that calls
> Eli> 'pselect'.
> 
> I have to look into it and I haven't found the time.
> I will try to get to it soon.

Thanks.  It's actually no more urgent than the other decisions to be
made before the concurrency branch is merged.

Re: the state of the concurrency branch

[Eli Zaretskii]

One more comments about the concurrency branch:

I think the error-handling facilities of the code should be upgraded,
to be able to cope with problems in the underlying low-level APIs.
For example, what happens if there's some error inside sys_cond_init?
Currently, there's no way for it to communicate anything to its
callers so that those could handle the problem gracefully.  Are
implementations supposed to call emacs_abort or signal an error from
such a low-level code?  If not, we need to at least make sys_*
function return some value.

Re: the state of the concurrency branch

[Eli Zaretskii]

The MS-Windows build of the concurrency branch should now build and
support threads now.  It should also build and work with pthreads, if
they are detected at configure time, although I didn't test that.

The resulting binary seems to pass the tests in
test/automated/threads.el.  There's some weird problem with the
"simple test of threads and let bindings" test: it succeeds the first
time, but if I run it twice, the second time crashes.  The crash is
not easy to interpret (the call stack is smashed), but it looks like
some error inside the call to longjmp by the main thread, while the
other thread is waiting for the global lock after it completed the
thread-yield call.  Any ideas?

Re: the state of the concurrency branch

[Eli Zaretskii]

> Date: Sat, 31 Aug 2013 14:51:31 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: lekktu@gmail.com, monnier@iro.umontreal.ca, emacs-devel@gnu.org
> 
> The resulting binary seems to pass the tests in
> test/automated/threads.el.  There's some weird problem with the
> "simple test of threads and let bindings" test: it succeeds the first
> time, but if I run it twice, the second time crashes.  The crash is
> not easy to interpret (the call stack is smashed), but it looks like
> some error inside the call to longjmp by the main thread, while the
> other thread is waiting for the global lock after it completed the
> thread-yield call.  Any ideas?

More info: Here's the backtrace from the last call to longjmp that
seems to be triggering a fatal runtime exception:

  Breakpoint 3, 0x77c36d74 in msvcrt!longjmp ()
     from C:\WINDOWS\system32\msvcrt.dll
  #0  0x77c36d74 in msvcrt!longjmp () from C:\WINDOWS\system32\msvcrt.dll
  #1  0x01174a1e in unwind_to_catch (catch=0x4e7feb8, value=60183150)
      at eval.c:1188
  #2  0x011757e2 in Fsignal (error_symbol=55041314, data=60183142)
      at eval.c:1607
  #3  0x01175869 in xsignal (error_symbol=55041314, data=60183142)
      at eval.c:1628
  #4  0x011758cf in xsignal2 (error_symbol=55041314, arg1=55041794,
      arg2=370042113) at eval.c:1649
  #5  0x01159a87 in wrong_type_argument (predicate=82312904, value=60157272)
      at data.c:189
  #6  0x0115b7de in find_symbol_value (symbol=370042113) at data.c:1147
  #7  0x01179dab in unbind_for_thread_switch () at eval.c:3495
  #8  0x011eb163 in post_acquire_global_lock (self=0x1501cd0 <primary_thread>)
      at thread.c:65
  #9  0x011eb252 in acquire_global_lock (self=0x1501cd0 <primary_thread>)
      at thread.c:90
  gdb: unknown target exception 0xc0000027 at 0x77c35464

  Program received signal ?, Unknown signal.
  0x77c35464 in msvcrt!_global_unwind2 () from C:\WINDOWS\system32\msvcrt.dll

It seems like the primary thread is signaling a wrong-type-argument
error, when it tries to unbind thread-local variables before switching
to a different thread.  Did Fsignal try to use a wrong 'struct
handler', one that belongs to a different thread?

Setting a breakpoint in wrong_type_argument, I see this:

  Breakpoint 3, wrong_type_argument (predicate=55041794, value=8580880)
      at data.c:189
  189       xsignal2 (Qwrong_type_argument, predicate, value);
  (gdb) bt
  #0  wrong_type_argument (predicate=55041794, value=8580880) at data.c:189
  #1  0x0115b7de in find_symbol_value (symbol=8580880) at data.c:1147
  #2  0x01179dab in unbind_for_thread_switch () at eval.c:3495
  #3  0x011eb163 in post_acquire_global_lock (self=0x1501cd0 <primary_thread>)
      at thread.c:65
  #4  0x011eb252 in acquire_global_lock (self=0x1501cd0 <primary_thread>)
      at thread.c:90
  #5  0x011ebd53 in yield_callback (ignore=0x0) at thread.c:601
  #6  0x01155e4d in flush_stack_call_func (func=0x11ebd30 <yield_callback>,
      arg=0x0) at alloc.c:4777
  #7  0x011ebd6f in Fthread_yield () at thread.c:608
  #8  0x011770ac in eval_sub (form=56246638) at eval.c:2240
  #9  0x01172da3 in Fprogn (body=56246646) at eval.c:480
  #10 0x01174591 in Fwhile (args=56246630) at eval.c:1010
  #11 0x01176db9 in eval_sub (form=56246606) at eval.c:2191
  #12 0x01172da3 in Fprogn (body=56246654) at eval.c:480
  #13 0x01176db9 in eval_sub (form=56246526) at eval.c:2191
  #14 0x011768d3 in Feval (form=56246526, lexical=54986778) at eval.c:2062
  #15 0x011784b1 in Ffuncall (nargs=3, args=0x82f448) at eval.c:2876
  #16 0x011bd36c in exec_byte_code (bytestr=20123049, vector=20123069,
      maxdepth=16, args_template=54986778, nargs=0, args=0x0) at bytecode.c:902
  #17 0x01179016 in funcall_lambda (fun=20123021, nargs=1, arg_vector=0x82f618)
      at eval.c:3107
  #18 0x011786c3 in Ffuncall (nargs=2, args=0x82f614) at eval.c:2922
  #19 0x011bd36c in exec_byte_code (bytestr=20123489, vector=20123509,
      maxdepth=12, args_template=54986778, nargs=0, args=0x0) at bytecode.c:902
  #20 0x01179016 in funcall_lambda (fun=20123461, nargs=1, arg_vector=0x82f824)
      at eval.c:3107
  #21 0x011786c3 in Ffuncall (nargs=2, args=0x82f820) at eval.c:2922
  #22 0x01171e00 in Fcall_interactively (function=56891970,
      record_flag=54986778, keys=55033861) at callint.c:836
  #23 0x011784e7 in Ffuncall (nargs=4, args=0x82fa0c) at eval.c:2880
  #24 0x011bd36c in exec_byte_code (bytestr=19816129, vector=19816149,
      maxdepth=52, args_template=4100, nargs=1, args=0x82fbf0) at bytecode.c:902
  #25 0x01178c6b in funcall_lambda (fun=19816109, nargs=1, arg_vector=0x82fbec)
      at eval.c:3041
  #26 0x011786c3 in Ffuncall (nargs=2, args=0x82fbe8) at eval.c:2922
  #27 0x01177f76 in call1 (fn=55032674, arg1=56891970) at eval.c:2672
  #28 0x010e2f06 in command_loop_1 () at keyboard.c:1560
  #29 0x011750c4 in internal_condition_case (bfun=0x10e26a5 <command_loop_1>,
      handlers=55041242, hfun=0x10e1f31 <cmd_error>) at eval.c:1359
  #30 0x010e2374 in command_loop_2 (ignore=54986778) at keyboard.c:1161
  #31 0x01174938 in internal_catch (tag=55031122,
      func=0x10e2351 <command_loop_2>, arg=54986778) at eval.c:1133
  #32 0x010e232c in command_loop () at keyboard.c:1140
  #33 0x010e1ae3 in recursive_edit_1 () at keyboard.c:779
  #34 0x010e1c97 in Frecursive_edit () at keyboard.c:843
  #35 0x010dfee0 in main (argc=2, argv=0xa42880) at emacs.c:1564

  Lisp Backtrace:
  "thread-yield" (0x4e7fb08)
  "let" (0x4e7fd58)
  "threads-test-thread2" (0x3516f5c)
  (gdb) up 2
  #2  0x01179dab in unbind_for_thread_switch () at eval.c:3495
  3495              bind->let.saved_value = find_symbol_value (specpdl_symbol (bin
  d));
  (gdb) p bind
  $2 = (union specbinding *) 0x34dac64
  (gdb) p bind->let
  $3 = {
    kind = 72,  <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
    symbol = 8580880,
    old_value = 54986778,
    where = 1,
    saved_value = -1
  }

Isn't 72 a value that should never happen in bind->kind?  Any idea how
this could happen?

Re: the state of the concurrency branch

[Eli Zaretskii]

> Date: Sat, 31 Aug 2013 16:42:46 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: lekktu@gmail.com, monnier@iro.umontreal.ca, emacs-devel@gnu.org
> 
>   Breakpoint 3, 0x77c36d74 in msvcrt!longjmp ()
>      from C:\WINDOWS\system32\msvcrt.dll
>   #0  0x77c36d74 in msvcrt!longjmp () from C:\WINDOWS\system32\msvcrt.dll
>   #1  0x01174a1e in unwind_to_catch (catch=0x4e7feb8, value=60183150)
>       at eval.c:1188
>   #2  0x011757e2 in Fsignal (error_symbol=55041314, data=60183142)
>       at eval.c:1607
>   #3  0x01175869 in xsignal (error_symbol=55041314, data=60183142)
>       at eval.c:1628
>   #4  0x011758cf in xsignal2 (error_symbol=55041314, arg1=55041794,
>       arg2=370042113) at eval.c:1649
>   #5  0x01159a87 in wrong_type_argument (predicate=82312904, value=60157272)
>       at data.c:189
>   #6  0x0115b7de in find_symbol_value (symbol=370042113) at data.c:1147
>   #7  0x01179dab in unbind_for_thread_switch () at eval.c:3495
>   #8  0x011eb163 in post_acquire_global_lock (self=0x1501cd0 <primary_thread>)
>       at thread.c:65
>   #9  0x011eb252 in acquire_global_lock (self=0x1501cd0 <primary_thread>)
>       at thread.c:90
>   gdb: unknown target exception 0xc0000027 at 0x77c35464
> 
>   Program received signal ?, Unknown signal.
>   0x77c35464 in msvcrt!_global_unwind2 () from C:\WINDOWS\system32\msvcrt.dll
> 
> It seems like the primary thread is signaling a wrong-type-argument
> error, when it tries to unbind thread-local variables before switching
> to a different thread.  Did Fsignal try to use a wrong 'struct
> handler', one that belongs to a different thread?

Answering my own question: yes, Fsignal tried to throw to a handler
that belonged to a different thread.  This is because
unbind_for_thread_switch is called in the context of a new thread, but
current_thread is still set to the old one.

I think I fixed this in revision 109719 on the concurrency branch.

> Setting a breakpoint in wrong_type_argument, I see this:
> 
>   Breakpoint 3, wrong_type_argument (predicate=55041794, value=8580880)
>       at data.c:189
>   189       xsignal2 (Qwrong_type_argument, predicate, value);
>   (gdb) bt
>   #0  wrong_type_argument (predicate=55041794, value=8580880) at data.c:189
>   #1  0x0115b7de in find_symbol_value (symbol=8580880) at data.c:1147
>   #2  0x01179dab in unbind_for_thread_switch () at eval.c:3495
>   #3  0x011eb163 in post_acquire_global_lock (self=0x1501cd0 <primary_thread>)
>       at thread.c:65
>   #4  0x011eb252 in acquire_global_lock (self=0x1501cd0 <primary_thread>)
>       at thread.c:90
>   #5  0x011ebd53 in yield_callback (ignore=0x0) at thread.c:601
>   #6  0x01155e4d in flush_stack_call_func (func=0x11ebd30 <yield_callback>,
>       arg=0x0) at alloc.c:4777
>   #7  0x011ebd6f in Fthread_yield () at thread.c:608
>   #8  0x011770ac in eval_sub (form=56246638) at eval.c:2240
>   #9  0x01172da3 in Fprogn (body=56246646) at eval.c:480
>   #10 0x01174591 in Fwhile (args=56246630) at eval.c:1010
>   #11 0x01176db9 in eval_sub (form=56246606) at eval.c:2191
>   #12 0x01172da3 in Fprogn (body=56246654) at eval.c:480
>   #13 0x01176db9 in eval_sub (form=56246526) at eval.c:2191
>   #14 0x011768d3 in Feval (form=56246526, lexical=54986778) at eval.c:2062
>   #15 0x011784b1 in Ffuncall (nargs=3, args=0x82f448) at eval.c:2876
>   #16 0x011bd36c in exec_byte_code (bytestr=20123049, vector=20123069,
>       maxdepth=16, args_template=54986778, nargs=0, args=0x0) at bytecode.c:902
>   #17 0x01179016 in funcall_lambda (fun=20123021, nargs=1, arg_vector=0x82f618)
>       at eval.c:3107
>   #18 0x011786c3 in Ffuncall (nargs=2, args=0x82f614) at eval.c:2922
>   #19 0x011bd36c in exec_byte_code (bytestr=20123489, vector=20123509,
>       maxdepth=12, args_template=54986778, nargs=0, args=0x0) at bytecode.c:902
>   #20 0x01179016 in funcall_lambda (fun=20123461, nargs=1, arg_vector=0x82f824)
>       at eval.c:3107
>   #21 0x011786c3 in Ffuncall (nargs=2, args=0x82f820) at eval.c:2922
>   #22 0x01171e00 in Fcall_interactively (function=56891970,
>       record_flag=54986778, keys=55033861) at callint.c:836
>   #23 0x011784e7 in Ffuncall (nargs=4, args=0x82fa0c) at eval.c:2880
>   #24 0x011bd36c in exec_byte_code (bytestr=19816129, vector=19816149,
>       maxdepth=52, args_template=4100, nargs=1, args=0x82fbf0) at bytecode.c:902
>   #25 0x01178c6b in funcall_lambda (fun=19816109, nargs=1, arg_vector=0x82fbec)
>       at eval.c:3041
>   #26 0x011786c3 in Ffuncall (nargs=2, args=0x82fbe8) at eval.c:2922
>   #27 0x01177f76 in call1 (fn=55032674, arg1=56891970) at eval.c:2672
>   #28 0x010e2f06 in command_loop_1 () at keyboard.c:1560
>   #29 0x011750c4 in internal_condition_case (bfun=0x10e26a5 <command_loop_1>,
>       handlers=55041242, hfun=0x10e1f31 <cmd_error>) at eval.c:1359
>   #30 0x010e2374 in command_loop_2 (ignore=54986778) at keyboard.c:1161
>   #31 0x01174938 in internal_catch (tag=55031122,
>       func=0x10e2351 <command_loop_2>, arg=54986778) at eval.c:1133
>   #32 0x010e232c in command_loop () at keyboard.c:1140
>   #33 0x010e1ae3 in recursive_edit_1 () at keyboard.c:779
>   #34 0x010e1c97 in Frecursive_edit () at keyboard.c:843
>   #35 0x010dfee0 in main (argc=2, argv=0xa42880) at emacs.c:1564
> 
>   Lisp Backtrace:
>   "thread-yield" (0x4e7fb08)
>   "let" (0x4e7fd58)
>   "threads-test-thread2" (0x3516f5c)
>   (gdb) up 2
>   #2  0x01179dab in unbind_for_thread_switch () at eval.c:3495
>   3495              bind->let.saved_value = find_symbol_value (specpdl_symbol (bin
>   d));
>   (gdb) p bind
>   $2 = (union specbinding *) 0x34dac64
>   (gdb) p bind->let
>   $3 = {
>     kind = 72,  <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
>     symbol = 8580880,
>     old_value = 54986778,
>     where = 1,
>     saved_value = -1
>   }
> 
> Isn't 72 a value that should never happen in bind->kind?  Any idea how
> this could happen?

This part still happens sometimes, but at least now Emacs signals an
error and doesn't crash.

Re: the state of the concurrency branch

[Eli Zaretskii]

> Date: Sun, 01 Sep 2013 18:49:19 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: lekktu@gmail.com, monnier@iro.umontreal.ca, emacs-devel@gnu.org
> 
> >   Breakpoint 3, wrong_type_argument (predicate=55041794, value=8580880)
> >       at data.c:189
> >   189       xsignal2 (Qwrong_type_argument, predicate, value);
> >   (gdb) bt
> >   #0  wrong_type_argument (predicate=55041794, value=8580880) at data.c:189
> >   #1  0x0115b7de in find_symbol_value (symbol=8580880) at data.c:1147
> >   #2  0x01179dab in unbind_for_thread_switch () at eval.c:3495
> >   #3  0x011eb163 in post_acquire_global_lock (self=0x1501cd0 <primary_thread>)
> >       at thread.c:65
> >   #4  0x011eb252 in acquire_global_lock (self=0x1501cd0 <primary_thread>)
> >       at thread.c:90
> >   #5  0x011ebd53 in yield_callback (ignore=0x0) at thread.c:601
> >   #6  0x01155e4d in flush_stack_call_func (func=0x11ebd30 <yield_callback>,
> >       arg=0x0) at alloc.c:4777
> >   #7  0x011ebd6f in Fthread_yield () at thread.c:608
> >   #8  0x011770ac in eval_sub (form=56246638) at eval.c:2240
> >   #9  0x01172da3 in Fprogn (body=56246646) at eval.c:480
> >   #10 0x01174591 in Fwhile (args=56246630) at eval.c:1010
> >   #11 0x01176db9 in eval_sub (form=56246606) at eval.c:2191
> >   #12 0x01172da3 in Fprogn (body=56246654) at eval.c:480
> >   #13 0x01176db9 in eval_sub (form=56246526) at eval.c:2191
> >   #14 0x011768d3 in Feval (form=56246526, lexical=54986778) at eval.c:2062
> >   #15 0x011784b1 in Ffuncall (nargs=3, args=0x82f448) at eval.c:2876
> >   #16 0x011bd36c in exec_byte_code (bytestr=20123049, vector=20123069,
> >       maxdepth=16, args_template=54986778, nargs=0, args=0x0) at bytecode.c:902
> >   #17 0x01179016 in funcall_lambda (fun=20123021, nargs=1, arg_vector=0x82f618)
> >       at eval.c:3107
> >   #18 0x011786c3 in Ffuncall (nargs=2, args=0x82f614) at eval.c:2922
> >   #19 0x011bd36c in exec_byte_code (bytestr=20123489, vector=20123509,
> >       maxdepth=12, args_template=54986778, nargs=0, args=0x0) at bytecode.c:902
> >   #20 0x01179016 in funcall_lambda (fun=20123461, nargs=1, arg_vector=0x82f824)
> >       at eval.c:3107
> >   #21 0x011786c3 in Ffuncall (nargs=2, args=0x82f820) at eval.c:2922
> >   #22 0x01171e00 in Fcall_interactively (function=56891970,
> >       record_flag=54986778, keys=55033861) at callint.c:836
> >   #23 0x011784e7 in Ffuncall (nargs=4, args=0x82fa0c) at eval.c:2880
> >   #24 0x011bd36c in exec_byte_code (bytestr=19816129, vector=19816149,
> >       maxdepth=52, args_template=4100, nargs=1, args=0x82fbf0) at bytecode.c:902
> >   #25 0x01178c6b in funcall_lambda (fun=19816109, nargs=1, arg_vector=0x82fbec)
> >       at eval.c:3041
> >   #26 0x011786c3 in Ffuncall (nargs=2, args=0x82fbe8) at eval.c:2922
> >   #27 0x01177f76 in call1 (fn=55032674, arg1=56891970) at eval.c:2672
> >   #28 0x010e2f06 in command_loop_1 () at keyboard.c:1560
> >   #29 0x011750c4 in internal_condition_case (bfun=0x10e26a5 <command_loop_1>,
> >       handlers=55041242, hfun=0x10e1f31 <cmd_error>) at eval.c:1359
> >   #30 0x010e2374 in command_loop_2 (ignore=54986778) at keyboard.c:1161
> >   #31 0x01174938 in internal_catch (tag=55031122,
> >       func=0x10e2351 <command_loop_2>, arg=54986778) at eval.c:1133
> >   #32 0x010e232c in command_loop () at keyboard.c:1140
> >   #33 0x010e1ae3 in recursive_edit_1 () at keyboard.c:779
> >   #34 0x010e1c97 in Frecursive_edit () at keyboard.c:843
> >   #35 0x010dfee0 in main (argc=2, argv=0xa42880) at emacs.c:1564
> > 
> >   Lisp Backtrace:
> >   "thread-yield" (0x4e7fb08)
> >   "let" (0x4e7fd58)
> >   "threads-test-thread2" (0x3516f5c)
> >   (gdb) up 2
> >   #2  0x01179dab in unbind_for_thread_switch () at eval.c:3495
> >   3495              bind->let.saved_value = find_symbol_value (specpdl_symbol (bin
> >   d));
> >   (gdb) p bind
> >   $2 = (union specbinding *) 0x34dac64
> >   (gdb) p bind->let
> >   $3 = {
> >     kind = 72,  <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
> >     symbol = 8580880,
> >     old_value = 54986778,
> >     where = 1,
> >     saved_value = -1
> >   }
> > 
> > Isn't 72 a value that should never happen in bind->kind?  Any idea how
> > this could happen?
> 
> This part still happens sometimes, but at least now Emacs signals an
> error and doesn't crash.

Here's a recipe for reproducing this:

 emacs -Q

Insert this:

   (defvar threads-test-binding nil)

   (defun threads-test-thread2 ()
     (let ((threads-test-binding 23))
       (thread-yield))
     (setq threads-test-global 23))

   (progn
     (setq threads-test-global nil)
     (make-thread #'threads-test-thread2)
     (while (not threads-test-global)
       (thread-yield))
     (and (not threads-test-binding)
	  threads-test-global))

Mark the defvar and the defun, type "M-x eval-region RET".  Go to the
last closing parent of progn, type "C-x C-e", get the expected result
23.  All's well till now.

Go to the right paren of the defvar, type "C-x C-e" there.  Now go
again to the rightmost paren of progn, type "C-x C-e", and get the
wrong-type-argument error.

Re: the state of the concurrency branch

[Stefan Monnier]

> The basic issue is that only one thread can select on a given fd at a
> time.

Why?

Stefan> --- src/eval.c	2013-08-20 03:53:07 +0000
Stefan> +++ src/eval.c	2013-08-26 20:54:04 +0000
Stefan> @@ -1119,6 +1119,7 @@
Stefan> c.next = catchlist;
Stefan> c.tag = tag;
Stefan> c.val = Qnil;
Stefan> +  /* FIXME: Why "f_"?  */
Stefan> c.f_handlerlist = handlerlist;
> The field needs some different name, because "handlerlist" is now a
> #define in thread.h.

Oh, I see, than makes sense.  In this case, I think it will be better
to replace uses of the `handlerlist' variable (maybe replace them with
TVAR(handlerlist)) rather than use a "handlerlist" macro so as to avoid
such problems.

The behavior of CPP "symbol macros" is pretty ugly, so we should try and
only use them for symbols that follow some naming convention (typically
being all-caps).

> Yeah.  The ordering is funky due to the #define hack.
> This could be done a different way; but the define approach at least
> makes merges simple; a big consideration for me given my extremely
> limited free time... a more invasive change would make merges very hard.

Makes a lot of sense.  But we should change it (shortly!) before merging
it into trunk.

> Somehow I wasn't aware that Emacs used bool.
> Is that new(-ish)?

Yes, it's newish.

Stefan> +/* FIXME: Why "m_"?  */
> I don't recall why "m" in particular, but it does need some prefix due
> to the #define approach.

Same as above, I'd rather use another macro that doesn't require such
renaming (and instead requires replacing some var-references by
macro-calls).

Stefan> +  /* FIXME: Why give it a name?  */
Stefan> /* The name of the mutex, or nil.  */
Stefan> Lisp_Object name;
> It's a debugging feature.  The name is optional for those who don't need it.

I see.  Not sure I like it, but maybe it's handy.


        Stefan

Re: the state of the concurrency branch

[Tom Tromey]

>> The basic issue is that only one thread can select on a given fd at a
>> time.

Stefan> Why?

To me it seemed like the simplest approach, not requiring changes
anywhere else in Emacs: if select says data is available, the thread can
proceed to read it and handle it.  Maybe another choice is to make the
reading code robust to failures here; and also audit to make sure there
aren't any possible races (readers have to really read all the data when
given a chance or else you could have different threads reading
different bits from the fd, possibly processing them in the wrong
order).

This approach was also convenient since it enabled thread-locking of
processes, something I think is desirable anyway.

Tom

Re: the state of the concurrency branch

[Tom Tromey]

Stefan>  DEFUN ("mutexp", Fmutexp, Smutexp, 1, 1, 0,
Stefan>         doc: /* Return t if OBJECT is a mutex.  */)
Stefan>    (Lisp_Object object)
Stefan>  {
Stefan> -  if (MUTEXP (object))
Stefan> -    return Qt;
Stefan> -  else
Stefan> -    return Qnil;
Stefan> +  return (MUTEXP (object) ? Qt : Qnil);
Stefan>  }
 
Here all the other predicate functions follow the style:

    if (MUTEXP (object))
      return Qt;
    return Qnil;

So I have adopted that instead.

Tom

Re: the state of the concurrency branch

[Eli Zaretskii]

> Date: Mon, 26 Aug 2013 21:51:41 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: tromey@redhat.com, monnier@iro.umontreal.ca, emacs-devel@gnu.org
> 
> I fixed the compilation, but the stuff in process.c needs work: if you
> invoke "M-x grep", Emacs aborts after Grep exits, here:
> 
>   void
>   delete_read_fd (int fd)
>   {
>     eassert (fd < MAXDESC);
>     eassert (fd <= max_desc); <<<<<<<<<<<<<<<<<
> 
> because max_desc is zero.

Here's some more info about this problem.  The assertion is violated
by the following series of calls:

  delete_read_fd (5);
  recompute_max_desc ();
  delete_read_fd (5);

The old code was doing FD_CLR instead of delete_read_fd, so it was
clearing twice the same bit, which doesn't hurt anything.  The new
code triggers the assertion, because clearing the flag bits in the
first delete_read_fd call causes recompute_max_desc to set max_desc to
zero, and the second call to delete_read_fd aborts.

If I remove the offending assertion, as in the patch below, the
problem goes away.

Why do we need these assertions?

=== modified file 'src/process.c'
--- src/process.c	2013-08-26 18:42:11 +0000
+++ src/process.c	2013-08-28 15:57:15 +0000
@@ -497,7 +497,6 @@ void
 delete_read_fd (int fd)
 {
   eassert (fd < MAXDESC);
-  eassert (fd <= max_desc);
   delete_keyboard_wait_descriptor (fd);
 
   if (fd_callback_info[fd].flags == 0)
@@ -559,7 +558,6 @@ delete_write_fd (int fd)
   int lim = max_desc;
 
   eassert (fd < MAXDESC);
-  eassert (fd <= max_desc);
 
 #ifdef NON_BLOCKING_CONNECT
   if ((fd_callback_info[fd].flags & NON_BLOCKING_CONNECT_FD) != 0)
@@ -6942,7 +6940,6 @@ delete_keyboard_wait_descriptor (int des
   int lim = max_desc;
 
   eassert (desc >= 0 && desc < MAXDESC);
-  eassert (desc <= max_desc);
 
   fd_callback_info[desc].flags &= ~(FOR_READ | KEYBOARD_FD | PROCESS_FD);
 

Re: the state of the concurrency branch

[Barry OReilly]

[-- Attachment #1: Type: text/plain, Size: 1754 bytes --]

> Here's a recipe for reproducing this:
>
>  emacs -Q
>
> Insert this:
>
>    (defvar threads-test-binding nil)
>
>    (defun threads-test-thread2 ()
>      (let ((threads-test-binding 23))
>        (thread-yield))
>      (setq threads-test-global 23))
>
>    (progn
>      (setq threads-test-global nil)
>      (make-thread #'threads-test-thread2)
>      (while (not threads-test-global)
>        (thread-yield))
>      (and (not threads-test-binding)
>           threads-test-global))
>
> Mark the defvar and the defun, type "M-x eval-region RET".  Go to the
> last closing parent of progn, type "C-x C-e", get the expected result
> 23.  All's well till now.
>
> Go to the right paren of the defvar, type "C-x C-e" there.  Now go
> again to the rightmost paren of progn, type "C-x C-e", and get the
> wrong-type-argument error.

If you evaluate the progn enough, it's possible to induce a core dump.

In unbind_for_thread_switch:

  void
  unbind_for_thread_switch (struct thread_state *thr)
  {
    union specbinding *bind;

    for (bind = thr->m_specpdl_ptr; bind != thr->m_specpdl; --bind)
      {
        if (bind->kind >= SPECPDL_LET)
          {
           bind->let.saved_value = find_symbol_value (specpdl_symbol
(bind));
           do_one_unbind (bind, 0);
          }
      }
  }

I find that bind->kind has values like 22 and 105, which are out of
bounds for the enum. Garbage values are one possibility.

When I looked at how other parts of the code iterate over the specpdl,
I find that the iterator decrements from specpdl_ptr first.
Furthermore, the documentation for specpdl_ptr says:

  /* Pointer to first unused element in specpdl.  */

  union specbinding *specpdl_ptr;

So is the solution to decrement bind once before iterating?

[-- Attachment #2: Type: text/html, Size: 2144 bytes --]

Re: the state of the concurrency branch

[Barry OReilly]

[-- Attachment #1: Type: text/plain, Size: 1017 bytes --]

Mimicing the style found elsewhere:

diff --git a/src/eval.c b/src/eval.c
index b8a6159..fc16c15 100644
--- a/src/eval.c
+++ b/src/eval.c
@@ -3488,9 +3488,9 @@ unbind_for_thread_switch (struct thread_state *thr)
 {
   union specbinding *bind;

-  for (bind = thr->m_specpdl_ptr; bind != thr->m_specpdl; --bind)
+  for (bind = thr->m_specpdl_ptr; bind > thr->m_specpdl;)
     {
-      if (bind->kind >= SPECPDL_LET)
+      if ((--bind)->kind >= SPECPDL_LET)
        {
          bind->let.saved_value = find_symbol_value (specpdl_symbol (bind));
          do_one_unbind (bind, 0);

With this I can eval the progn many times. I'm not sure why it's 23
the first time, then "23 (#o27, #x17, ?\C-w)" all subsequent times:

  23
  23 (#o27, #x17, ?\C-w) [3 times]

A couple other comments on the concurrency branch:

  • thread-yield should call timer_check. See discussion at:
    http://debbugs.gnu.org/cgi/bugreport.cgi?bug=15045#176

  • condition-variablep should be condition-variable-p

[-- Attachment #2: Type: text/html, Size: 1232 bytes --]

Re: the state of the concurrency branch

[Tom Tromey]

Barry> A couple other comments on the concurrency branch:

Barry> • thread-yield should call timer_check. See discussion at:
Barry> http://debbugs.gnu.org/cgi/bugreport.cgi?bug=15045#176

Barry> • condition-variablep should be condition-variable-p

Thanks for looking at this.
I haven't had time to hack on the branch lately but I hope to get back
to it soon.

Tom

Re: the state of the concurrency branch

[Tom Tromey]

Barry> - for (bind = thr->m_specpdl_ptr; bind != thr->m_specpdl; --bind)
Barry> + for (bind = thr->m_specpdl_ptr; bind > thr->m_specpdl;)
Barry> {
Barry> - if (bind->kind >= SPECPDL_LET)
Barry> + if ((--bind)->kind >= SPECPDL_LET)

I looked at this a little tonight and I think it is correct.
Can you check it in?
If not let me know and I will do it.

Barry> • thread-yield should call timer_check. See discussion at:
Barry> http://debbugs.gnu.org/cgi/bugreport.cgi?bug=15045#176

It seems reasonable to me.  However I wonder whether timers ought to be
thread-locked the way that processes are.

Barry> • condition-variablep should be condition-variable-p

Thanks, I've pushed a patch for this.

Tom

Re: the state of the concurrency branch

[Stefan Monnier]

> It seems reasonable to me.  However I wonder whether timers ought to be
> thread-locked the way that processes are.

Ideally, neither should be thread-locked.  So I'd rather not lock
it/them unless it's needed for backward compatibility.


        Stefan

Re: the state of the concurrency branch

[Tom Tromey]

>>>>> "Stefan" == Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> It seems reasonable to me.  However I wonder whether timers ought to be
>> thread-locked the way that processes are.

Stefan> Ideally, neither should be thread-locked.  So I'd rather not lock
Stefan> it/them unless it's needed for backward compatibility.

I think the bad case is a situation where some code dynamically binds
some variable and then sleeps, knowing that the timer will fire and see
the binding.

If there are multiple threads, the sleep may switch threads and cause
the timer to run in a different dynamic environment.

Whether this warrants thread-locking, I couldn't say.
I don't know whether this sort of thing is common for timers.

Tom

Re: the state of the concurrency branch

[Richard Stallman]

        [ To any NSA and FBI agents reading my email: please consider
        [ whether defending the US Constitution against all enemies,
        [ foreign or domestic, requires you to follow Snowden's example.

    If there are multiple threads, the sleep may switch threads and cause
    the timer to run in a different dynamic environment.

This suggests that a timer should be associated with a thread,
and should run its callback in that thread.

-- 
Dr Richard Stallman
President, Free Software Foundation
51 Franklin St
Boston MA 02110
USA
www.fsf.org  www.gnu.org
Skype: No way! That's nonfree (freedom-denying) software.
  Use Ekiga or an ordinary phone call.

Re: the state of the concurrency branch

[Barry OReilly]

[-- Attachment #1: Type: text/plain, Size: 627 bytes --]

> Can you check it in?

Done.

> If there are multiple threads, the sleep may switch threads and
> cause the timer to run in a different dynamic environment.

It's a good point, though I think such code sounds dodgy.

Theoretically, it's already true on trunk that timers can run with
different dynamic bindings each time it runs. sit-for is called from
various places, timers can run within timers, and all can dynamically
bind variables.

If timers changed to each run in their own thread, dynamic bindings
would be easier to reason about. I don't know if that should happen
before or after merging to trunk (or not at all).

[-- Attachment #2: Type: text/html, Size: 732 bytes --]

Re: the state of the concurrency branch

[Stefan Monnier]

> Theoretically, it's already true on trunk that timers can run with
> different dynamic bindings each time it runs. sit-for is called from
> various places, timers can run within timers, and all can dynamically
> bind variables.

In theory you could imagine a scheme where timers inherit the dynamic
bindings that are in place when the timer is setup (or when the process
is started).
It wouldn't provide the exact same behavior we have currently, butit
might be made close enough to give the right behavior in practice in
most cases.


        Stefan

Re: the state of the concurrency branch

[Barry OReilly]

[-- Attachment #1: Type: text/plain, Size: 940 bytes --]

> In theory you could imagine a scheme where timers inherit the
> dynamic bindings that are in place when the timer is setup (or when
> the process is started).
>
> It wouldn't provide the exact same behavior we have currently, butit
> might be made close enough to give the right behavior in practice in
> most cases.

Sounds kind of like Clojure's bound-fn.

  http://clojuredocs.org/clojure_core/clojure.core/bound-fn

Those docs don't say whether the non binding of a global is restored.
Looks like not:

  (def ^:dynamic *var01* "one")
  (def ^:dynamic *var02* "two")
  (let [bnd-f
        (binding [*var02* "ghi"]
          (bound-fn [] (list  *var01* *var02*)))]
    (binding [*var01* "abc" *var02* "def"]
      (bnd-f)))
  ;; Evaluates to ("abc" "ghi") as opposed to ("one" "ghi")

Something like bound-lambda for Elisp might be neat, then the scheme
you describe would be like

  (run-at-time '(0 1 0 0) nil (bound-lambda () ...))

[-- Attachment #2: Type: text/html, Size: 1258 bytes --]

Re: the state of the concurrency branch

[Tom Tromey]

>>>>> "Stefan" == Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> Theoretically, it's already true on trunk that timers can run with
>> different dynamic bindings each time it runs. sit-for is called from
>> various places, timers can run within timers, and all can dynamically
>> bind variables.

Stefan> In theory you could imagine a scheme where timers inherit the dynamic
Stefan> bindings that are in place when the timer is setup (or when the process
Stefan> is started).
Stefan> It wouldn't provide the exact same behavior we have currently, butit
Stefan> might be made close enough to give the right behavior in practice in
Stefan> most cases.

I had actually implemented this for new threads, but you had me take it
out again :)

I think this would be too magical to do just for timers.
For one thing, I think a typical reason to do this kind of funny
business is to communicate from the timer (or process filter or
whatever) back to the caller.  So the scheme would have to be more
complicated.

Tom

Re: the state of the concurrency branch

[Barry OReilly]

[-- Attachment #1: Type: text/plain, Size: 224 bytes --]

> I think a typical reason to do this kind of funny business is to
> communicate from the timer (or process filter or whatever) back to
> the caller.

For this use case, wouldn't a feature for thread futures be appropriate?

[-- Attachment #2: Type: text/html, Size: 281 bytes --]

Re: the state of the concurrency branch

[Stefan Monnier]

>> I think a typical reason to do this kind of funny business is to
>> communicate from the timer (or process filter or whatever) back to
>> the caller.
> For this use case, wouldn't a feature for thread futures be appropriate?

For process filters/sentinels, you can almost always replace the
dynamically scoped variable with a process property.

We could add timer properties for that same purpose.  Tho, there are
already many other ways to get the same result without relying on
dynamic bindings.


        Stefan

Re: the state of the concurrency branch

[Stefan Monnier]

Stefan> In theory you could imagine a scheme where timers inherit the dynamic
Stefan> bindings that are in place when the timer is setup (or when the process
Stefan> is started).
Stefan> It wouldn't provide the exact same behavior we have currently, butit
Stefan> might be made close enough to give the right behavior in practice in
Stefan> most cases.
> I had actually implemented this for new threads, but you had me take it
> out again :)

I know ;-) (tho it was slightly different, IIRC, because your code
copied the value, whereas for timers we'd need to really share the
bindings, so that assignments in the timer code are reflected back in
the other thread).

> I think this would be too magical to do just for timers.

Agreed.


        Stefan

Re: the state of the concurrency branch

[Barry OReilly]

[-- Attachment #1: Type: text/plain, Size: 279 bytes --]

> With this I can eval the progn many times. I'm not sure why it's 23
> the first time, then "23 (#o27, #x17, ?\C-w)" all subsequent times:
>
>   23
>   23 (#o27, #x17, ?\C-w) [3 times]

This occurs on trunk, so not a concern of the concurrency branch. I
submitted a bug report.

[-- Attachment #2: Type: text/html, Size: 366 bytes --]