On Wed, 17 Nov 2010 14:28:26 -0500, Austin Clements <amdragon@MIT.EDU> wrote:
> This reduces thread search's 1+2t Xapian queries (where t is the
> number of matched threads) to 1+t queries and constructs exactly one
> notmuch_message_t for each message instead of 2 to 3.

Fantastic stuff, Austin!

I've merged this now, (sorry it took me a while to get to it).

One of the reasons I didn't merge it immediately is that I wanted to
ensure that I understood the original author-ordering bug. Basically,
I'm inherently uncomfortable with a performance optimization that fixes
a bug as a side effect, (unless we understand that very well).

So what I pushed actually adds the bug fix first, so that the
performance optimization makes no change at all to the test suite. That
feels better to me, (even though it simply demonstrated conclusively
that the bug was in a piece of code that was eliminated by the
optimization).

Anyway, in a quick reading of the code, the only little thing I saw was:

> +    size_t count = (bound + sizeof (doc_ids->bitmap[0]) - 1) /
> +	sizeof (doc_ids->bitmap[0]);

Which would look better to my eyes with a 1 factored out of the
division:

	size_t count = 1 + (bound - 1) / sizeof (doc_ids->bitmap[0]);

And the repeated use of "sizeof (doc_ids->bitmap[0])" could maybe do
with a macro for better legibility. Though it would be an evil macro if
it didn't accept an argument, and it wouldn't be much shorter if it
did. So maybe it's fine as-is.

Thanks for the optimization. Now all we need is a little notmuch
benchmark so that I can be sure not to regress any performance work with
my sloppy coding!

-Carl

-- 
carl.d.worth@intel.com