please review new branch feature/byte-unwind-protect

please review new branch feature/byte-unwind-protect

[Tom Tromey]

Hi.

I recently wrote a couple of patches to add two new bytecodes to Emacs.
These make it possible to compile unwind-protect without the need to
introduce a closure for the unwind forms.

I've pushed these to feature/byte-unwind-protect.  They seem to be
working pretty well, so I would appreciate a review.  I'd like to merge
these to trunk.

The first patch changes the C code so that a CATCHER_ALL handler sees
both signals and throws.  In addition to being needed for the second
patch, this allowed some simplifications in the module code.  Note that
this changes the interface exposed by internal_catch_all, but as there
is only one caller, it is easy to verify that the change doesn't matter.

The second patch adds the new bytecodes and updates the byte compiler.
Two bytecodes are added.

The first, Bpushunwindprotect, pushes a CATCHER_ALL handler.  On error,
it then pushes the exception value and `t' and jumps to the unwind
forms.  When there is no error, the body form's value is left on the
stack and then an additional `nil' is pushed.

The second bytecode, Bendunwindprotect, looks at the top of the stack
and decides whether to carry on or to call `signal' or `throw'.

It's possible that this new bytecode could be used to remove the
unwind-protect restrictions in generator.el, though I didn't look deeply
into that.

thanks,
Tom

Re: please review new branch feature/byte-unwind-protect

[John Wiegley]

>>>>> "TT" == Tom Tromey <tom@tromey.com> writes:

TT> I recently wrote a couple of patches to add two new bytecodes to Emacs.
TT> These make it possible to compile unwind-protect without the need to
TT> introduce a closure for the unwind forms.

Hi Tom,

Thanks for these changes. I just want to ask: what motivates them?  Is it
efficiency? Does it enable something you can't achieve otherwise? If the
former, has it been measured? What is the corresponding cost of having this
change?

Thanks,
-- 
John Wiegley                  GPG fingerprint = 4710 CF98 AF9B 327B B80F
http://newartisans.com                          60E1 46C4 BD1A 7AC1 4BA2

Re: please review new branch feature/byte-unwind-protect

[Tom Tromey]

>>>>> "John" == John Wiegley <johnw@gnu.org> writes:

>>>>> "TT" == Tom Tromey <tom@tromey.com> writes:
TT> I recently wrote a couple of patches to add two new bytecodes to Emacs.
TT> These make it possible to compile unwind-protect without the need to
TT> introduce a closure for the unwind forms.

John> Hi Tom,

John> Thanks for these changes. I just want to ask: what motivates them?  Is it
John> efficiency? Does it enable something you can't achieve otherwise? If the
John> former, has it been measured? What is the corresponding cost of having this
John> change?

It's partly for efficiency -- I didn't benchmark it though.  I suppose I
can.  It's pretty normal to see unwind-protect in the expansion of
macros, though; there are plenty in subr.el.  This enables better code
generation in the jit I've written (I plan to post about that when it's
ready but I can do it sooner if you want).

Also this seems like an un-finished to-do item from the lexical binding
conversion.

I don't think there's a cost to this change.  It uses two byte codes,
but there are still some to spare, so I think it isn't a big deal.

Tom

Re: please review new branch feature/byte-unwind-protect

[Stefan Monnier]

> It's partly for efficiency -- I didn't benchmark it though.

I'd be interested to see some benchmarks, as well (e.g. comparing
old-unwind-protect vs new-unwind-protect vs prog1, where the prog1 case
is used as the "speed of light" reference).

> Also this seems like an un-finished to-do item from the lexical binding
> conversion.

Not exactly "unfinished", but yes, it's a good change.

> I don't think there's a cost to this change.

There's always costs:
- fewer bytecodes left.
- incompatility of .elc files.
- potentially slower execution (hopefully it's the reverse, but without
  measuring it, it's difficult to be sure).
- subtle changes (e.g. the debugger backtrace, or the behavior of
  called-interactively-p when called from within the 2nd part of an
  unwind-protect).
From where I stand I think they're worth the trouble, tho it depends on
the benchmarks (I don't have a good intuition for the cost of setjmp,
mostly, so while I think/hope that it will make things faster, I'm not
completely sure).

BTW, when I wrote those FIXME, the implementation strategy I was thinking
of was:
- use byte-codes with a similar semantics to the ones you introduce.
- implement the pushunwindprotect by pushing onto the specpdl stack
  an entry that contains a reference to the bytecode stack and
  bytecode's pc (note: no setjmp).
- when running the unwind_protect during a non-local exit (i.e. from
  unbind_to), run the byte-code interpreter using that saved stack and pc.


        Stefan

Re: please review new branch feature/byte-unwind-protect

[Rocky Bernstein]

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

If this does get added to emacs, would you please consider making a change
to https://github.com/rocky/elisp-bytecode ? Thanks.

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

Re: please review new branch feature/byte-unwind-protect

[Markus Triska]

Hi Tom,

Tom Tromey <tom@tromey.com> writes:

> I recently wrote a couple of patches to add two new bytecodes to Emacs.
> These make it possible to compile unwind-protect without the need to
> introduce a closure for the unwind forms.

Thank you very much for your work!

I have one feature request that is related to unwind-protect, filed as
issue #23963:

   https://debbugs.gnu.org/cgi/bugreport.cgi?bug=23963

This is only tangentially related to your immediate patch. However, as
you are now working in this area, I would like to use the opportunity to
ask you for your opinion on this construct. In my view, it would help to
reliably solve a long-standing issue with unwind-protect and possible
resource leaks. Would new instructions help with this construct?

Thank you and all the best!
Markus

Re: please review new branch feature/byte-unwind-protect

[Stefan Monnier]

> I have one feature request that is related to unwind-protect, filed as
> issue #23963:

While we're here I have another related feature request: make it
possible to provide a "rewind".  Currently the C code has a few
hard-coded rewinds (basically: those for let bindings and those for
save-excursion, IIRC), but doesn't offer any way to provide one's own.

The rewinds are used in the backtrace debugger when the user evaluates
an expression with `e` (which evaluates the expression in the specific
stack frame at point, so it first unwinds up to that stack frame so that
the evaluation has access to the same variables in the same buffer, and
afterwards rewinds back down to the stack frame where the error
occurred).


        Stefan

Re: please review new branch feature/byte-unwind-protect

[Philipp Stephani]

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

Tom Tromey <tom@tromey.com> schrieb am Di., 23. Jan. 2018 um 06:21 Uhr:

>
> The first patch changes the C code so that a CATCHER_ALL handler sees
> both signals and throws.  In addition to being needed for the second
> patch, this allowed some simplifications in the module code.  Note that
> this changes the interface exposed by internal_catch_all, but as there
> is only one caller, it is easy to verify that the change doesn't matter.
>

This patch is a pretty uncontroversial refactoring that simplifies the
codebase independently of the byte-code change, so I'd suggest you install
it as-is. Thanks.

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