[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: the state of the concurrency branch
From: |
Stefan Monnier |
Subject: |
Re: the state of the concurrency branch |
Date: |
Mon, 26 Aug 2013 17:29:03 -0400 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.3.50 (gnu/linux) |
> 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;
- the state of the concurrency branch, Tom Tromey, 2013/08/25
- Re: the state of the concurrency branch, Stefan Monnier, 2013/08/26
- Re: the state of the concurrency branch, Juanma Barranquero, 2013/08/26
- Re: the state of the concurrency branch, Tom Tromey, 2013/08/26
- Re: the state of the concurrency branch, Eli Zaretskii, 2013/08/26
- Re: the state of the concurrency branch,
Stefan Monnier <=
- Re: the state of the concurrency branch, Tom Tromey, 2013/08/26
- Re: the state of the concurrency branch, Eli Zaretskii, 2013/08/27
- Re: the state of the concurrency branch, Paul Eggert, 2013/08/27
- Re: the state of the concurrency branch, Tom Tromey, 2013/08/27
- Re: the state of the concurrency branch, Paul Eggert, 2013/08/27
- Re: the state of the concurrency branch, Tom Tromey, 2013/08/27
- Re: the state of the concurrency branch, Paul Eggert, 2013/08/27
- Re: the state of the concurrency branch, Eli Zaretskii, 2013/08/27
- Re: the state of the concurrency branch, Paul Eggert, 2013/08/27
- Re: the state of the concurrency branch, Eli Zaretskii, 2013/08/27