Re: [notmuch] [PATCH] notmuch-new: Eliminate tallocs whilst construct filenames.

diff --git a/3a/ca3526d7c088d5d3a166b62e4dc08870a69b36 b/3a/ca3526d7c088d5d3a166b62e4dc08870a69b36
new file mode 100644 (file)
index 0000000..a4e6920
--- /dev/null
+++ b/3a/ca3526d7c088d5d3a166b62e4dc08870a69b36
@@ -0,0 +1,116 @@
+Return-Path: <chris@chris-wilson.co.uk>\r
+X-Original-To: notmuch@notmuchmail.org\r
+Delivered-To: notmuch@notmuchmail.org\r
+Received: from localhost (localhost [127.0.0.1])\r
+       by olra.theworths.org (Postfix) with ESMTP id 760BF431FBC\r
+       for <notmuch@notmuchmail.org>; Fri, 27 Nov 2009 06:17:07 -0800 (PST)\r
+X-Virus-Scanned: Debian amavisd-new at olra.theworths.org\r
+Received: from olra.theworths.org ([127.0.0.1])\r
+       by localhost (olra.theworths.org [127.0.0.1]) (amavisd-new, port 10024)\r
+       with ESMTP id 9B0eyLvFz9lx for <notmuch@notmuchmail.org>;\r
+       Fri, 27 Nov 2009 06:17:05 -0800 (PST)\r
+Received: from orsmga101.jf.intel.com (mga06.intel.com [134.134.136.21])\r
+       by olra.theworths.org (Postfix) with ESMTP id CB719431FAE\r
+       for <notmuch@notmuchmail.org>; Fri, 27 Nov 2009 06:17:05 -0800 (PST)\r
+Received: from orsmga002.jf.intel.com ([10.7.209.21])\r
+       by orsmga101.jf.intel.com with ESMTP; 27 Nov 2009 06:16:38 -0800\r
+X-ExtLoop1: 1\r
+X-IronPort-AV: E=Sophos;i="4.47,301,1257148800"; d="scan'208";a="470989888"\r
+Received: from unknown (HELO cwilso3-mobl.ger.corp.intel.com)\r
+ ([10.255.16.178])     by orsmga002.jf.intel.com with SMTP; 27 Nov 2009 06:34:02\r
+ -0800\r
+Received: by cwilso3-mobl.ger.corp.intel.com (sSMTP sendmail emulation);\r
+       Fri, 27 Nov 2009 14:17:02 +0000\r
+Content-Type: text/plain; charset=UTF-8\r
+From: Chris Wilson <chris@chris-wilson.co.uk>\r
+To: Carl Worth <cworth@cworth.org>\r
+In-reply-to: <87einkqeyt.fsf@yoom.home.cworth.org>\r
+References: <1258851430-28732-1-git-send-email-chris@chris-wilson.co.uk>\r
+       <87einkqeyt.fsf@yoom.home.cworth.org>\r
+Date: Fri, 27 Nov 2009 14:17:02 +0000\r
+Message-Id: <1259329997-sup-2634@broadwater.alporthouse.com>\r
+User-Agent: Sup/git\r
+Content-Transfer-Encoding: 8bit\r
+Cc: notmuch <notmuch@notmuchmail.org>\r
+Subject: Re: [notmuch] [PATCH] notmuch-new: Eliminate tallocs whilst\r
+       construct filenames.\r
+X-BeenThere: notmuch@notmuchmail.org\r
+X-Mailman-Version: 2.1.12\r
+Precedence: list\r
+List-Id: "Use and development of the notmuch mail system."\r
+       <notmuch.notmuchmail.org>\r
+List-Unsubscribe: <http://notmuchmail.org/mailman/options/notmuch>,\r
+       <mailto:notmuch-request@notmuchmail.org?subject=unsubscribe>\r
+List-Archive: <http://notmuchmail.org/pipermail/notmuch>\r
+List-Post: <mailto:notmuch@notmuchmail.org>\r
+List-Help: <mailto:notmuch-request@notmuchmail.org?subject=help>\r
+List-Subscribe: <http://notmuchmail.org/mailman/listinfo/notmuch>,\r
+       <mailto:notmuch-request@notmuchmail.org?subject=subscribe>\r
+X-List-Received-Date: Fri, 27 Nov 2009 14:17:07 -0000\r
+\r
+Excerpts from Carl Worth's message of Fri Nov 27 13:23:06 +0000 2009:\r
+> On Sun, 22 Nov 2009 00:57:10 +0000, Chris Wilson <chris@chris-wilson.co.uk> wrote:\r
+> > The majority of filenames will fit within PATH_MAX [4096] (because\r
+> > that's a hard limit imposed by the filesystems) so we can avoid an\r
+> > allocation per lookup and thereby eliminate a large proportion of the\r
+> > overhead of scanning a maildir.\r
+> \r
+> Hi Chris,\r
+> \r
+> I *know* I composed a reply to this message earlier, but apparently\r
+> you're right that it never went out. (*sigh*---if only I had a reliable\r
+> mail client[*]).\r
+\r
+I hear there's one called sup... ;-)\r
+\r
+> Anyway, on to the promised review of the patch:\r
+> \r
+> The basic idea of this I really like, of course. Thanks for helping to\r
+> improve the efficiency of notmuch. But this part I don't:\r
+\r
+It's a bit outdated now, the impact of the asprintf overhead is lost\r
+with the introduction of the scandir. I'm sure the profiles will\r
+indicate something else to improve beyond xapian...\r
+\r
+> One might argue that the error-cleanup goto is a common and\r
+> well-understood idiom, so that it's not bad to have. The problem I have\r
+> with it is how much work it is to verify. If I'm reading one line of\r
+> code in the middle of a function that's testing for an error case and\r
+> handling it with goto, then I need to:\r
+> \r
+>     1. Verify this condition, and that a return value variable gets\r
+>        set.\r
+> \r
+>     2. Check down at the end of the function to ensure the correct\r
+>        objects are freed and the correct return value is returned.\r
+> \r
+>     3. Check back up at the beginning of the function to ensure the\r
+>        relevant objects are initialized to NULL.\r
+> \r
+> And beyond verification, one has to code in these three places\r
+> simultaneously as well.\r
+> \r
+> Meanwhile, by taking advantage of talloc like I did in the original\r
+> version of this code, an error return becomes a much more local decision\r
+> and is much simpler to code.\r
+\r
+The issue I see with the "error, continue" pattern is that we are in\r
+danger of not reporting the first error but the last one. The common\r
+practice is abort on error and cleanup, and this single instance is\r
+inconsistent with the rest of the error handling everywhere else in\r
+notmuch-new.c. The argument to counter your 3 points is the unified\r
+unwind approach where there is just a single exit path that handles\r
+both error and normal returns. (You always have to set the appropriate\r
+error value whether you continue or abort.)\r
+\r
+The advantage of talloc is that it provides a convenient allocation\r
+context that not only groups object by their associated lifetimes, but\r
+provides a single point of access for reaping allocations on unwind. I\r
+don't see how talloc affects the decision process on how to actually\r
+handle errors, but it does make it easier to cleanup afterwards.\r
+\r
+Is notmuch ready for fault-injection yet? Maybe once you have a nasty\r
+testsuite...\r
+-ickle\r
+-- \r
+Chris Wilson, Intel Open Source Technology Centre\r