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