7:58 a.m.
You who are about to crash (Norbert & Jerry, this means you!),
I salute you:
Please apply the enclosed patch (against current CVS). After that
salutation, it should be clear why this is not in CVS. :-)
I found a possible crash left unguarded in Mike's earlier patch. I
have my suspicions about other opcodes, but let's see if this helps.
Build is OK, I'm sending this post from the patched XEmacs, but since
with Mike's patch to GCPRO_STACK Fset I only crash a little more often
once a day, I can't say this is "tested" yet.
Bvarbind definitely calls Fset through specbind_magic(). I have my
suspicions about Bset_buffer, and maybe some other B.*set.* opcodes.
Mike, please look at those opcodes: Maybe your oracle is faster than
me trying to trace function calls....
Of course it passes 100% of the tests. :-)
>>>> "sjt" == Stephen J Turnbull
<stephen(a)xemacs.org> writes:
sjt> Per this discussion I've reverted the GCPRO_STACK patch temporarily.
Index: src/ChangeLog
===================================================================
RCS file: /pack/xemacscvs/XEmacs/xemacs/src/ChangeLog,v
retrieving revision 1.649
diff -u -U0 -r1.649 ChangeLog
--- src/ChangeLog 1 Nov 2003 14:54:53 -0000 1.649
+++ src/ChangeLog 4 Nov 2003 06:48:54 -0000
＠＠ -0,0 +1,13 ＠＠
+2003-11-04 Stephen J. Turnbull <stephen(a)xemacs.org>
+
+ * bytecode.c (execute_optimized_program): GCPRO_STACK Bvarbind, too.
+
+ Restore 2003-08-12 Mike Sperber <mike(a)xemacs.org>:
+
+ * bytecode.c (GCPRO_STACK): Added.
+ (execute_optimized_program): Use GCPRO_STACK, fixing a space leak:
+ Formerly, the byte-code engine would always hold on to the entire
+ stack memory area, including the stuff above the top. Now, we
+ adjust the GCPRO record via GCPRO_STACK just before a GC may
+ occur.
+
Index: src/bytecode.c
===================================================================
RCS file: /pack/xemacscvs/XEmacs/xemacs/src/bytecode.c,v
retrieving revision 1.37
diff -u -u -r1.37 bytecode.c
--- src/bytecode.c 21 Oct 2003 08:21:02 -0000 1.37
+++ src/bytecode.c 4 Nov 2003 06:49:02 -0000
＠＠ -447,6 +447,8 ＠＠
but don't pop it. */
#define TOP (*stack_ptr)
+#define GCPRO_STACK (gcpro1.nvars = stack_ptr - stack_beg)
+
/* The actual interpreter for byte code.
This function has been seriously optimized for performance.
Don't change the constructs unless you are willing to do
＠＠ -460,8 +462,8 ＠＠
{
/* This function can GC */
REGISTER const Opbyte *program_ptr = (Opbyte *) program;
- REGISTER Lisp_Object *stack_ptr
- = alloca_array (Lisp_Object, stack_depth + 1);
+ Lisp_Object *stack_beg = alloca_array (Lisp_Object, stack_depth + 1);
+ REGISTER Lisp_Object *stack_ptr = stack_beg;
int speccount = specpdl_depth ();
struct gcpro gcpro1;
＠＠ -471,23 +473,11 ＠＠
#endif
#ifdef ERROR_CHECK_BYTE_CODE
- Lisp_Object *stack_beg = stack_ptr;
Lisp_Object *stack_end = stack_beg + stack_depth;
#endif
- /* Initialize all the objects on the stack to Qnil,
- so we can GCPRO the whole stack.
- The first element of the stack is actually a dummy. */
- {
- int i;
- Lisp_Object *p;
- for (i = stack_depth, p = stack_ptr; i--;)
- *++p = Qnil;
- }
-
GCPRO1 (stack_ptr[1]);
- gcpro1.nvars = stack_depth;
-
+
while (1)
{
REGISTER Opcode opcode = (Opcode) READ_UINT_1;
＠＠ -512,7 +502,11 ＠＠
if (opcode >= Bconstant)
PUSH (constants_data[opcode - Bconstant]);
else
- stack_ptr = execute_rare_opcode (stack_ptr, program_ptr, opcode);
+ {
+ /* We're not sure what these do, so better safe than sorry. */
+ GCPRO_STACK;
+ stack_ptr = execute_rare_opcode (stack_ptr, program_ptr, opcode);
+ }
break;
case Bvarref:
＠＠ -549,8 +543,12 ＠＠
Lisp_Object new_value = POP;
if (!SYMBOL_VALUE_MAGIC_P (old_value) || UNBOUNDP (old_value))
symbol_ptr->value = new_value;
- else
+ else {
+ /* Fset may call magic handlers */
+ GCPRO_STACK;
Fset (symbol, new_value);
+ }
+
break;
}
＠＠ -583,7 +581,11 ＠＠
#endif
}
else
- specbind_magic (symbol, new_value);
+ {
+ /* does an Fset, may call magic handlers */
+ GCPRO_STACK;
+ specbind_magic (symbol, new_value);
+ }
break;
}
＠＠ -597,6 +599,7 ＠＠
case Bcall+7:
n = (opcode < Bcall+6 ? opcode - Bcall :
opcode == Bcall+6 ? READ_UINT_1 : READ_UINT_2);
+ GCPRO_STACK;
DISCARD (n);
#ifdef BYTE_CODE_METER
if (byte_metering_on && SYMBOLP (TOP))
＠＠ -974,6 +977,8 ＠＠
case Bset:
{
Lisp_Object arg = POP;
+ /* Fset may call magic handlers */
+ GCPRO_STACK;
TOP = Fset (TOP, arg);
break;
}
--
Institute of Policy and Planning Sciences http://turnbull.sk.tsukuba.ac.jp
University of Tsukuba Tennodai 1-1-1 Tsukuba 305-8573 JAPAN
Ask not how you can "do" free software business;
ask what your business can "do for" free software.
8:48 a.m.
"Stephen J. Turnbull" <stephen(a)xemacs.org> writes:
You who are about to crash (Norbert & Jerry, this means you!),
I salute you:
Please apply the enclosed patch (against current CVS). After that
salutation, it should be clear why this is not in CVS. :-)
:-)
I've applied it, testing right now. I haven't seen crashes lately,
although I have been using Gnus, the ultimate regression checker :-)
Do you want a build report?
norbert.
9:10 a.m.
>>>> "viteno" == Norbert Koch
<viteno(a)xemacs.org> writes:
viteno> Do you want a build report?
You mean you're NOT a 'bot? I was SURE you weren't real. :-)
Build reports are always nice. I've gotten lazy, myself, so I can't
really use words like "should". ;-)
--
Institute of Policy and Planning Sciences http://turnbull.sk.tsukuba.ac.jp
University of Tsukuba Tennodai 1-1-1 Tsukuba 305-8573 JAPAN
Ask not how you can "do" free software business;
ask what your business can "do for" free software.
10:46 a.m.
"Stephen J. Turnbull" <stephen(a)xemacs.org> writes:
Bvarbind definitely calls Fset through specbind_magic(). I have my
suspicions about Bset_buffer, and maybe some other B.*set.* opcodes.
Doesn't set-buffer change the values of all buffer-local variables?
I'm not sure if buffer-local vars can be dontuse-ed, but if they can,
Bset_buffer can GC.
What a cool expression, dontuse. We should use it everywhere. For
example, dontuse-lazy-shot-mode, dontuse-balloon-x, dontuse-mule, the
dontuse-iso-2022-jp coding system, etc.
Given how hard GC is to trace, maybe we really should just update
nvars whenever the stack changes. I wonder what the impact on
performance would be.
3:46 p.m.
>>>> "Hrvoje" == Hrvoje Niksic
<hniksic(a)xemacs.org> writes:
Hrvoje> "Stephen J. Turnbull" <stephen(a)xemacs.org> writes:
> Bvarbind definitely calls Fset through specbind_magic(). I have
my
> suspicions about Bset_buffer, and maybe some other B.*set.* opcodes.
Yes, great catch!
Hrvoje> Doesn't set-buffer change the values of all buffer-local
Hrvoje> variables? I'm not sure if buffer-local vars can be
Hrvoje> dontuse-ed, but if they can, Bset_buffer can GC.
It can theoretically call GC, but not in that place. (Look at
get_buffer.) I guess the equal methods might GC, or that's what the
code assumes, anyway. So I guess we stick a GCPRO_STACK there, too.
Do you want me to take this again from now on, Stephen? (Again, many,
many thanks.)
(Hrvoje is of course, in a roundabout way, alluding to the completely
fucked interaction between buffer-local variables, dynamic binding,
and assignment. But last I tried, FSFmacs was even worse than we.)
Hrvoje> Given how hard GC is to trace, maybe we really should just update
Hrvoje> nvars whenever the stack changes. I wonder what the impact on
Hrvoje> performance would be.
It's not quite so easy. Look at Bcall where it's necessary to do the
update *before* the stack pointer changes.
The general implementation strategy for the byte-code is kind of
brain-dead, anyway. It sure isn't suitable for anything but tectonic
performance, that's for sure.
--
Cheers =8-} Mike
Friede, Völkerverständigung und überhaupt blabla
7:18 a.m.
OK, both Norbert and I have observed crashes with my patch.
I tried GCPRO_STACK before Bset_buffer. That did not prevent another
crash.
The equal methods might as well be userland, there are so many of
them.... I didn't try to do that, though, because I don't know where
equal might be called (I first caught it in "Bmember", is there a
"Bassoc"? and then "Bequal" codes themselves, urg).
>>>> "ms" == Michael Sperber
<sperber(a)informatik.uni-tuebingen.de> writes:
Hrvoje> Doesn't set-buffer change the values of all buffer-local
Hrvoje> variables? I'm not sure if buffer-local vars can be
Hrvoje> dontuse-ed, but if they can, Bset_buffer can GC.
ms> It can theoretically call GC, but not in that place. (Look at
ms> get_buffer.) I guess the equal methods might GC, or that's
ms> what the code assumes, anyway. So I guess we stick a
ms> GCPRO_STACK there, too.
ms> Do you want me to take this again from now on, Stephen?
ms> (Again, many, many thanks.)
Please. I've spent some time tracing some of the calls and concluded
that I simply don't have the background to do this effectively. But
if you have a question, I'll be happy to try to describe what I've
looked at so far (FWIW).
My current experiment is to keep the GCPRO(stack[1]), and put a
GCPRO_STACK in front of the while (there's more bytecode) loop. But I
don't even know if that would do the right thing at all (and haven't
even run make check to be sure weak tests pass :-/).
That should give you an idea just how little I grok this. :-/
ms> The general implementation strategy for the byte-code is kind
ms> of brain-dead, anyway. It sure isn't suitable for anything
ms> but tectonic performance, that's for sure.
Uh-oh. Not the byte-interpreter, too!
--
Institute of Policy and Planning Sciences http://turnbull.sk.tsukuba.ac.jp
University of Tsukuba Tennodai 1-1-1 Tsukuba 305-8573 JAPAN
Ask not how you can "do" free software business;
ask what your business can "do for" free software.
7:57 p.m.
Stephen, you're a hero. I have only very limited machine and Internet
access right now, so it'll take me until the weekend until I can try
this out. Meanwhile, many thanks for looking into this!
--
Cheers =8-} Mike
Friede, Völkerverständigung und überhaupt blabla
7683
days inactive
7689
days old
6 comments
4 participants
participants (4)
-
Hrvoje Niksic -
Michael Sperber -
Norbert Koch -
Stephen J. Turnbull