unofficial mirror of emacs-devel@gnu.org 
 help / color / mirror / code / Atom feed
From: Paul Eggert <eggert@cs.ucla.edu>
To: Stefan Monnier <monnier@iro.umontreal.ca>
Cc: emacs-devel@gnu.org
Subject: Re: [Emacs-diffs] trunk r113437: New unwind-protect flavors to better type-check C callbacks.
Date: Wed, 24 Jul 2013 08:23:18 +0100	[thread overview]
Message-ID: <51EF80E6.9020300@cs.ucla.edu> (raw)
In-Reply-To: <jwvtxjlto3t.fsf-monnier+emacs@gnu.org>

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.




  reply	other threads:[~2013-07-24  7:23 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <E1UzCuc-0001NQ-T1@vcs.savannah.gnu.org>
2013-07-22  4:13 ` [Emacs-diffs] trunk r113437: New unwind-protect flavors to better type-check C callbacks Stefan Monnier
2013-07-23  9:00   ` Paul Eggert
2013-07-23 13:21     ` Stefan Monnier
2013-07-24  7:23       ` Paul Eggert [this message]
2013-07-24 14:08         ` Stefan Monnier

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: https://www.gnu.org/software/emacs/

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=51EF80E6.9020300@cs.ucla.edu \
    --to=eggert@cs.ucla.edu \
    --cc=emacs-devel@gnu.org \
    --cc=monnier@iro.umontreal.ca \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/emacs.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).