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.