Content-Type: text/plain; charset=UTF-8
From: Chris Wilson <chris@chris-wilson.co.uk>
To: Carl Worth <cworth@cworth.org>
In-reply-to: <87einkqeyt.fsf@yoom.home.cworth.org>
References: <1258851430-28732-1-git-send-email-chris@chris-wilson.co.uk>
	<87einkqeyt.fsf@yoom.home.cworth.org>
Date: Fri, 27 Nov 2009 14:17:02 +0000
Message-Id: <1259329997-sup-2634@broadwater.alporthouse.com>
User-Agent: Sup/git
Content-Transfer-Encoding: 8bit
Cc: notmuch <notmuch@notmuchmail.org>
Subject: Re: [notmuch] [PATCH] notmuch-new: Eliminate tallocs whilst
	construct filenames.
Precedence: list

Excerpts from Carl Worth's message of Fri Nov 27 13:23:06 +0000 2009:
> On Sun, 22 Nov 2009 00:57:10 +0000, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > The majority of filenames will fit within PATH_MAX [4096] (because
> > that's a hard limit imposed by the filesystems) so we can avoid an
> > allocation per lookup and thereby eliminate a large proportion of the
> > overhead of scanning a maildir.
> 
> Hi Chris,
> 
> I *know* I composed a reply to this message earlier, but apparently
> you're right that it never went out. (*sigh*---if only I had a reliable
> mail client[*]).

I hear there's one called sup... ;-)

> Anyway, on to the promised review of the patch:
> 
> The basic idea of this I really like, of course. Thanks for helping to
> improve the efficiency of notmuch. But this part I don't:

It's a bit outdated now, the impact of the asprintf overhead is lost
with the introduction of the scandir. I'm sure the profiles will
indicate something else to improve beyond xapian...

> One might argue that the error-cleanup goto is a common and
> well-understood idiom, so that it's not bad to have. The problem I have
> with it is how much work it is to verify. If I'm reading one line of
> code in the middle of a function that's testing for an error case and
> handling it with goto, then I need to:
> 
>     1. Verify this condition, and that a return value variable gets
>        set.
> 
>     2. Check down at the end of the function to ensure the correct
>        objects are freed and the correct return value is returned.
> 
>     3. Check back up at the beginning of the function to ensure the
>        relevant objects are initialized to NULL.
> 
> And beyond verification, one has to code in these three places
> simultaneously as well.
> 
> Meanwhile, by taking advantage of talloc like I did in the original
> version of this code, an error return becomes a much more local decision
> and is much simpler to code.

The issue I see with the "error, continue" pattern is that we are in
danger of not reporting the first error but the last one. The common
practice is abort on error and cleanup, and this single instance is
inconsistent with the rest of the error handling everywhere else in
notmuch-new.c. The argument to counter your 3 points is the unified
unwind approach where there is just a single exit path that handles
both error and normal returns. (You always have to set the appropriate
error value whether you continue or abort.)

The advantage of talloc is that it provides a convenient allocation
context that not only groups object by their associated lifetimes, but
provides a single point of access for reaping allocations on unwind. I
don't see how talloc affects the decision process on how to actually
handle errors, but it does make it easier to cleanup afterwards.

Is notmuch ready for fault-injection yet? Maybe once you have a nasty
testsuite...
-ickle
-- 
Chris Wilson, Intel Open Source Technology Centre