From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!not-for-mail From: Paul Eggert Newsgroups: gmane.emacs.devel 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 Message-ID: <51EF80E6.9020300@cs.ucla.edu> References: <51EE4645.4000904@cs.ucla.edu> NNTP-Posting-Host: plane.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit X-Trace: ger.gmane.org 1374650607 1623 80.91.229.3 (24 Jul 2013 07:23:27 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Wed, 24 Jul 2013 07:23:27 +0000 (UTC) Cc: emacs-devel@gnu.org To: Stefan Monnier Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Wed Jul 24 09:23:28 2013 Return-path: Envelope-to: ged-emacs-devel@m.gmane.org Original-Received: from lists.gnu.org ([208.118.235.17]) by plane.gmane.org with esmtp (Exim 4.69) (envelope-from ) id 1V1tPz-0002Mg-TD for ged-emacs-devel@m.gmane.org; Wed, 24 Jul 2013 09:23:28 +0200 Original-Received: from localhost ([::1]:49220 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V1tPz-0000Qz-GB for ged-emacs-devel@m.gmane.org; Wed, 24 Jul 2013 03:23:27 -0400 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:52543) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V1tPw-0000Qb-4w for emacs-devel@gnu.org; Wed, 24 Jul 2013 03:23:25 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1V1tPu-0003H7-VY for emacs-devel@gnu.org; Wed, 24 Jul 2013 03:23:24 -0400 Original-Received: from smtp.cs.ucla.edu ([131.179.128.62]:59524) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V1tPu-0003Gs-LT for emacs-devel@gnu.org; Wed, 24 Jul 2013 03:23:22 -0400 Original-Received: from localhost (localhost.localdomain [127.0.0.1]) by smtp.cs.ucla.edu (Postfix) with ESMTP id EE2B5A60002; Wed, 24 Jul 2013 00:23:21 -0700 (PDT) X-Virus-Scanned: amavisd-new at smtp.cs.ucla.edu Original-Received: from smtp.cs.ucla.edu ([127.0.0.1]) by localhost (smtp.cs.ucla.edu [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id o2zZbhU4ukTt; Wed, 24 Jul 2013 00:23:21 -0700 (PDT) Original-Received: from [192.168.0.3] (host86-166-241-185.range86-166.btcentralplus.com [86.166.241.185]) by smtp.cs.ucla.edu (Postfix) with ESMTPSA id E754B39E8105; Wed, 24 Jul 2013 00:23:20 -0700 (PDT) User-Agent: Mozilla/5.0 (X11; Linux i686; rv:17.0) Gecko/20130623 Thunderbird/17.0.7 In-Reply-To: X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.6.x X-Received-From: 131.179.128.62 X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: "Emacs development discussions." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Original-Sender: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Xref: news.gmane.org gmane.emacs.devel:162103 Archived-At: 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.