Re: [Emacs-diffs] trunk r113437: New unwind-protect flavors to better type-check C callbacks.

Re: [Emacs-diffs] trunk r113437: New unwind-protect flavors to better type-check C callbacks.

[Stefan Monnier]

>   New unwind-protect flavors to better type-check C callbacks.

I must say I much preferred having a single record_unwind.  No strong
technical arguments for it (other than the fact that the added
SPECPDL_UNWIND_* cases in the switch slow down `unbind').

Changing the function to return void instead of Lisp_Object is a good
change, OTOH.


        Stefan

Re: [Emacs-diffs] trunk r113437: New unwind-protect flavors to better type-check C callbacks.

[Paul Eggert]

On 07/22/2013 05:13 AM, Stefan Monnier wrote:
> I much preferred having a single record_unwind.

The old way was simpler, but it had a problem:
record_unbind_protect (foo, make_save_value (...))  didn't
work when make_save_value exhausted memory and threw an
error.  To avoid the problem, there needs to be a way to
invoke record_unwind_protect without the possibility of
throwing an error before the protection is in place.

record_unwind_protect_ptr does that, since the caller
already can have a local variable in the C stack, and pass
its address.  This works because every function that calls
record_unwind_protect_ptr also calls unbind_to to undo it,
before returning.

It wouldn't be feasible to convert all uses of
record_unwind_protect to record_unwind_protect_ptr, because
not every function that calls record_unbind_protect also
calls unbind_to.

Removing record_unwind_protect_int would not be trivial,
since one can't simply replace it with record_unwind_protect
+ make_integer (as not all 'int' values fit into Lisp
integers), and one can't simply replace it by passing an int
* to record_unwind_protect_ptr (as not all functions that
invoke record_unwind_protect_int also call unbind_to).

We could trivially remove record_unwind_protect_void by
using one of the other methods and passing a dummy argument.
This would slow down execution very slightly, though.  Plus,
an advantage of having record_unwind_protect_void and
record_unwind_protect_int is it catches more type errors at
the C level.

> No strong technical arguments for it (other than the fact that the
> added SPECPDL_UNWIND_* cases in the switch slow down `unbind').

It shouldn't slow down 'unbind', at least on my x86-64
platform, since the switch is implemented as an indirect
jump, which means that adding cases doesn't add any
instructions for the existing cases.  The new code should
even speed up 'unbind' a tad, at least for the void case,
since it can avoid passing an argument that the unwinder
doesn't look at.

Re: [Emacs-diffs] trunk r113437: New unwind-protect flavors to better type-check C callbacks.

[Stefan Monnier]

> To avoid the problem, there needs to be a way to
> invoke record_unwind_protect without the possibility of
> throwing an error before the protection is in place.

Why?

> It wouldn't be feasible to convert all uses of
> record_unwind_protect to record_unwind_protect_ptr, because
> not every function that calls record_unbind_protect also
> calls unbind_to.

Oh, man, that's even worse than I thought.  This is really bad, it means
that it suffers from the same kind of limitations as alloca, i.e. it is
tricky to use and coders need to be extra aware of it (e.g. common code
refactoring can be unsafe).

> Removing record_unwind_protect_int would not be trivial,
> since one can't simply replace it with record_unwind_protect
> + make_integer (as not all 'int' values fit into Lisp

We've lived with it for what... almost 30 years?

> This would slow down execution very slightly, though.

As mentioned, the extra cases in unbind_to also cause a minor slowdown,
so I don't think this is a significant argument.

>> No strong technical arguments for it (other than the fact that the
>> added SPECPDL_UNWIND_* cases in the switch slow down `unbind').
> It shouldn't slow down 'unbind', at least on my x86-64
> platform, since the switch is implemented as an indirect
> jump, which means that adding cases doesn't add any
> instructions for the existing cases.

The problem is not the number of instructions but the time taken by each
instruction because correctly predicting indirect jumps is very
expensive and adding cases to the switch will increase the number
of mispredictions.


        Stefan

Re: [Emacs-diffs] trunk r113437: New unwind-protect flavors to better type-check C callbacks.

[Paul Eggert]

On 07/23/2013 02:21 PM, Stefan Monnier wrote:
>> To avoid the problem, there needs to be a way to
>> invoke record_unwind_protect without the possibility of
>> throwing an error before the protection is in place.
>
> Why?

For example, here's a call to unwind_protect in Emacs,
before the recent unwind-protect changes.  It was in dired.c:

  DIR *d = open_directory (SSDATA (dirfilename), &fd);
  if (d == NULL)
    report_file_error ("Opening directory", directory);
  record_unwind_protect (directory_files_internal_unwind,
             make_save_pointer (d));

Suppose make_save_pointer exhausts memory.  Then it signals an error,
record_unwind_protect is not called, and there's a storage leak -- the
DIR * object newly allocated by open_directory is never freed, and the
corresponding Unix file descriptor leaks as well.

With the change, that last call becomes:

  record_unwind_protect_ptr (directory_files_internal_unwind, d);

This cannot possibly signal an error before D is safely ensconced in
the specpdl stack, so D and its file descriptor can't leak.

> it means that it suffers from the same kind of limitations as
> alloca, i.e. it is tricky to use and coders need to be extra aware
> of it (e.g. common code refactoring can be unsafe).

Yes, but the old code had the same problem: make_save_pointer (P)
created an object that could not be safely used if P's storage was
freed at the C level.  This is the usual pattern where
record_unwind_protect_ptr is now used, so we're no worse off than
before here.  Admittedly there are a couple of exceptions, where
the code now uses a local C object where it formerly created a
Lisp object -- if necessary we can go back to the
old way, but the old way will be slower, and it'll be more
complicated than it used to be (to avoid the leaks mentioned above),
so I'd rather not.

> We've lived with it for what... almost 30 years?

That potential integer overflow was a low-priority bug to fix, yes.
But it's fixed now, and we needn't reintroduce it.

> correctly predicting indirect jumps is very expensive and adding
> cases to the switch will increase the number of mispredictions.

Ah, now I see what you're driving at, and you're right.  Still, I
expect that the difference in performance is so small that it's not
significant -- the switch already had 5 cases and we're adding just 3,
and with two-level branch predictors almost universal nowadays we
should see measurements before worrying about it too much.

Certainly any such performance degradation should be swamped by the
other performance improvements inherent to the change, as there's no
longer a need to invoke make_save_pointer, so there's no need to
create and then garbage-collect an object for each of these
record_unwind_protect calls: that's such a win that it should be well
worth possibly losing one branch-predict miss.

Re: [Emacs-diffs] trunk r113437: New unwind-protect flavors to better type-check C callbacks.

[Stefan Monnier]

> Yes, but the old code had the same problem: make_save_pointer (P)
> created an object that could not be safely used if P's storage was
> freed at the C level.

Oh, that's right.

> Certainly any such performance degradation should be swamped by the
> other performance improvements inherent to the change, as there's no
> longer a need to invoke make_save_pointer, so there's no need to
> create and then garbage-collect an object for each of these
> record_unwind_protect calls: that's such a win that it should be well
> worth possibly losing one branch-predict miss.

I'm not sure about "well worth" because AFAIK these make_save_pointer
calls were only in non-performance-sensitive areas, whereas unbind_to is
used all the time.  But I agree that the overall impact is probably
small enough.


        Stefan