From 70962fabf9c57cda5af26c28894fc9371fd085f2 Mon Sep 17 00:00:00 2001 From: Carl Worth Date: Mon, 23 Nov 2009 21:47:24 -0800 Subject: [PATCH] lib/messages.c: Make message searches stream as well. Xapian provides an interator-based interface to all search results. So it was natural to make notmuch_messages_t be iterator-based as well. Which we did originally. But we ran into a problem when we added two APIs, (_get_replies and _get_toplevel_messages), that want to return a messages iterator that's *not* based on a Xapian search result. My original compromise was to use notmuch_message_list_t as the basis for all returned messages iterators in the public interface. This had the problem of introducing extra latency at the beginning of a search for messages, (the call would block while iterating over all results from Xapian, converting to a message list). In this commit, we remove that initial conversion and instead provide two alternate implementations of notmuch_messages_t (one on top of a Xapian iterator and one on top of a message list). With this change, I tested a "notmuch search" returning *many* results as previously taking about 7 seconds before results started appearing, and now taking only 2 seconds. --- lib/messages.c | 37 +++++++++++---- lib/notmuch-private.h | 20 ++++++++ lib/query.cc | 104 ++++++++++++++++++++++++++++++++---------- 3 files changed, 130 insertions(+), 31 deletions(-) diff --git a/lib/messages.c b/lib/messages.c index 2f7c283e..54c0ab07 100644 --- a/lib/messages.c +++ b/lib/messages.c @@ -20,13 +20,6 @@ #include "notmuch-private.h" -#include /* GList */ - - -struct _notmuch_messages { - notmuch_message_node_t *iterator; -}; - /* Create a new notmuch_message_list_t object, with 'ctx' as its * talloc owner. * @@ -85,20 +78,45 @@ _notmuch_messages_create (notmuch_message_list_t *list) if (unlikely (messages == NULL)) return NULL; + messages->is_of_list_type = TRUE; messages->iterator = list->head; return messages; } +/* We're using the "is_of_type_list" to conditionally defer to the + * notmuch_mset_messages_t implementation of notmuch_messages_t in + * query.cc. It's ugly that that's over in query.cc, and it's ugly + * that we're not using a union here. Both of those uglies are due to + * C++: + * + * 1. I didn't want to force a C++ header file onto + * notmuch-private.h and suddenly subject all our code to a + * C++ compiler and its rules. + * + * 2. C++ won't allow me to put C++ objects, (with non-trivial + * constructors) into a union anyway. Even though I'd + * carefully control object construction with placement new + * anyway. *sigh* + */ notmuch_bool_t notmuch_messages_has_more (notmuch_messages_t *messages) { - return (messages != NULL && messages->iterator != NULL); + if (messages == NULL) + return FALSE; + + if (! messages->is_of_list_type) + return _notmuch_mset_messages_has_more (messages); + + return (messages->iterator != NULL); } notmuch_message_t * notmuch_messages_get (notmuch_messages_t *messages) { + if (! messages->is_of_list_type) + return _notmuch_mset_messages_get (messages); + if (messages->iterator == NULL) return NULL; @@ -108,6 +126,9 @@ notmuch_messages_get (notmuch_messages_t *messages) void notmuch_messages_advance (notmuch_messages_t *messages) { + if (! messages->is_of_list_type) + return _notmuch_mset_messages_advance (messages); + if (messages->iterator == NULL) return; diff --git a/lib/notmuch-private.h b/lib/notmuch-private.h index 2b4bf319..d3f9a4c4 100644 --- a/lib/notmuch-private.h +++ b/lib/notmuch-private.h @@ -290,6 +290,15 @@ typedef struct _notmuch_message_list { notmuch_message_node_t **tail; } notmuch_message_list_t; +/* There's a rumor that there's an alternate struct _notmuch_messages + * somewhere with some nasty C++ objects in it. We'll try to maintain + * ignorance of that here. (See notmuch_mset_messages_t in query.cc) + */ +struct _notmuch_messages { + notmuch_bool_t is_of_list_type; + notmuch_message_node_t *iterator; +}; + notmuch_message_list_t * _notmuch_message_list_create (const void *ctx); @@ -304,6 +313,17 @@ _notmuch_message_list_add_message (notmuch_message_list_t *list, notmuch_messages_t * _notmuch_messages_create (notmuch_message_list_t *list); +/* query.cc */ + +notmuch_bool_t +_notmuch_mset_messages_has_more (notmuch_messages_t *messages); + +notmuch_message_t * +_notmuch_mset_messages_get (notmuch_messages_t *messages); + +void +_notmuch_mset_messages_advance (notmuch_messages_t *messages); + /* message.cc */ void diff --git a/lib/query.cc b/lib/query.cc index 7d191e52..a571a618 100644 --- a/lib/query.cc +++ b/lib/query.cc @@ -31,11 +31,12 @@ struct _notmuch_query { notmuch_sort_t sort; }; -struct _notmuch_messages { +typedef struct _notmuch_mset_messages { + notmuch_messages_t base; notmuch_database_t *notmuch; Xapian::MSetIterator iterator; Xapian::MSetIterator iterator_end; -}; +} notmuch_mset_messages_t; struct _notmuch_threads { notmuch_query_t *query; @@ -75,19 +76,42 @@ notmuch_query_set_sort (notmuch_query_t *query, notmuch_sort_t sort) query->sort = sort; } +/* We end up having to call the destructors explicitly because we had + * to use "placement new" in order to initialize C++ objects within a + * block that we allocated with talloc. So C++ is making talloc + * slightly less simple to use, (we wouldn't need + * talloc_set_destructor at all otherwise). + */ +static int +_notmuch_messages_destructor (notmuch_mset_messages_t *messages) +{ + messages->iterator.~MSetIterator (); + messages->iterator_end.~MSetIterator (); + + return 0; +} + notmuch_messages_t * notmuch_query_search_messages (notmuch_query_t *query) { notmuch_database_t *notmuch = query->notmuch; const char *query_string = query->query_string; - notmuch_message_list_t *message_list; - Xapian::MSetIterator i; + notmuch_mset_messages_t *messages; - message_list = _notmuch_message_list_create (query); - if (unlikely (message_list == NULL)) + messages = talloc (query, notmuch_mset_messages_t); + if (unlikely (messages == NULL)) return NULL; try { + + messages->base.is_of_list_type = FALSE; + messages->base.iterator = NULL; + messages->notmuch = notmuch; + new (&messages->iterator) Xapian::MSetIterator (); + new (&messages->iterator_end) Xapian::MSetIterator (); + + talloc_set_destructor (messages, _notmuch_messages_destructor); + Xapian::Enquire enquire (*notmuch->xapian_db); Xapian::Query mail_query (talloc_asprintf (query, "%s%s", _find_prefix ("type"), @@ -130,22 +154,8 @@ notmuch_query_search_messages (notmuch_query_t *query) mset = enquire.get_mset (0, notmuch->xapian_db->get_doccount ()); - for (i = mset.begin (); i != mset.end (); i++) { - notmuch_message_t *message; - notmuch_private_status_t status; - - message = _notmuch_message_create (message_list, notmuch, - *i, &status); - if (message == NULL) - { - if (status == NOTMUCH_PRIVATE_STATUS_NO_DOCUMENT_FOUND) - INTERNAL_ERROR ("A message iterator contains a " - "non-existent document ID.\n"); - break; - } - - _notmuch_message_list_add_message (message_list, message); - } + messages->iterator = mset.begin (); + messages->iterator_end = mset.end (); } catch (const Xapian::Error &error) { fprintf (stderr, "A Xapian exception occurred performing query: %s\n", @@ -154,7 +164,55 @@ notmuch_query_search_messages (notmuch_query_t *query) notmuch->exception_reported = TRUE; } - return _notmuch_messages_create (message_list); + return &messages->base; +} + +notmuch_bool_t +_notmuch_mset_messages_has_more (notmuch_messages_t *messages) +{ + notmuch_mset_messages_t *mset_messages; + + mset_messages = (notmuch_mset_messages_t *) messages; + + return (mset_messages->iterator != mset_messages->iterator_end); +} + +notmuch_message_t * +_notmuch_mset_messages_get (notmuch_messages_t *messages) +{ + notmuch_message_t *message; + Xapian::docid doc_id; + notmuch_private_status_t status; + notmuch_mset_messages_t *mset_messages; + + mset_messages = (notmuch_mset_messages_t *) messages; + + if (! _notmuch_mset_messages_has_more (&mset_messages->base)) + return NULL; + + doc_id = *mset_messages->iterator; + + message = _notmuch_message_create (mset_messages, + mset_messages->notmuch, doc_id, + &status); + + if (message == NULL && + status == NOTMUCH_PRIVATE_STATUS_NO_DOCUMENT_FOUND) + { + INTERNAL_ERROR ("a messages iterator contains a non-existent document ID.\n"); + } + + return message; +} + +void +_notmuch_mset_messages_advance (notmuch_messages_t *messages) +{ + notmuch_mset_messages_t *mset_messages; + + mset_messages = (notmuch_mset_messages_t *) messages; + + mset_messages->iterator++; } /* Glib objects force use to use a talloc destructor as well, (but not