Re: [PATCH 1/3] new: Do not defer maildir flag synchronization during the first run

From: Austin Clements <amdragon@mit.edu>
To: Michal Sojka <sojkam1@fel.cvut.cz>
Cc: notmuch@notmuchmail.org
Subject: Re: [PATCH 1/3] new: Do not defer maildir flag synchronization during the first run
Date: Tue, 25 Jan 2011 17:42:30 -0500	[thread overview]
Message-ID: <AANLkTinsC+89yx7jGUaB8PLiftLjyTr7gosUDgAC_g0S@mail.gmail.com> (raw)
In-Reply-To: <1295603977-14326-3-git-send-email-sojkam1@fel.cvut.cz>

[-- Attachment #1: Type: text/plain, Size: 5258 bytes --]

Wouldn't this be simpler and more general?

--- a/notmuch-new.c
+++ b/notmuch-new.c
@@ -419,12 +419,11 @@ add_files_recursive (notmuch_database_t *notmuch,
        case NOTMUCH_STATUS_SUCCESS:
            state->added_messages++;
            for (tag=state->new_tags; *tag != NULL; tag++)
                notmuch_message_add_tag (message, *tag);
            /* Defer sync of maildir flags until after old filenames
             * are removed in the case of a rename. */
            if (state->synchronize_flags == TRUE)
-               _filename_list_add (state->message_ids_to_sync,
-                                   notmuch_message_get_message_id
(message));
+               notmuch_message_maildir_flags_to_tags (message);
            break;
        /* Non-fatal issues (go on to next file) */
        case NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID:

The idea is that, if notmuch_database_add_message
returns NOTMUCH_STATUS_SUCCESS, then we know this is a new message (and not
a rename or anything complicated) and thus might as well perform the flag
synchronization immediately.  If it
returns NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID, then it could be a rename (or
something more complicated), and so we defer the flag synchronization like
usual.  This works for any new messages, regardless of whether this is the
initial import or not.

I believe my reasoning is correct.  At least, it passes the maildir sync
test cases, so if it isn't correct, then we need more maildir sync tests.

On Fri, Jan 21, 2011 at 4:59 AM, Michal Sojka <sojkam1@fel.cvut.cz> wrote:

> When notmuch new is run for the first time, it is not necessary to defer
> maildir flags synchronization to later because we already know that no
> files will be removed.
>
> Performing the maildinr flag synchronization immediately after the
> message is added to the database has the advantage that the message is
> likely hot in the disk cache so the synchronization is faster.
> Additionally, we also save one database query for each message, which
> must be performed when the operation is deferred.
>
> Without this patchi, the first notmuch new of 200k messages (3 GB) took
> 1h and 46m out of which 20m was maildir flags synchronization. With this
> patch, the whole operation took only 1h and 36m.
> ---
>  notmuch-new.c |   36 ++++++++++++++++++++++++++----------
>  1 files changed, 26 insertions(+), 10 deletions(-)
>
> diff --git a/notmuch-new.c b/notmuch-new.c
> index cdf8513..a2af045 100644
> --- a/notmuch-new.c
> +++ b/notmuch-new.c
> @@ -420,19 +420,35 @@ add_files_recursive (notmuch_database_t *notmuch,
>            state->added_messages++;
>            for (tag=state->new_tags; *tag != NULL; tag++)
>                notmuch_message_add_tag (message, *tag);
> -           /* Defer sync of maildir flags until after old filenames
> -            * are removed in the case of a rename. */
> -           if (state->synchronize_flags == TRUE)
> -               _filename_list_add (state->message_ids_to_sync,
> -                                   notmuch_message_get_message_id
> (message));
> +           if (state->synchronize_flags == TRUE) {
> +               if (!state->total_files) {
> +                   /* Defer sync of maildir flags until after old
> filenames
> +                    * are removed in the case of a rename. */
> +                   _filename_list_add (state->message_ids_to_sync,
> +                                       notmuch_message_get_message_id
> (message));
> +               } else {
> +                   /* During the first notmuch new we synchronize
> +                    * flags immediately, while the message is hot in
> +                    * disk cache. */
> +                   notmuch_message_maildir_flags_to_tags (message);
> +               }
> +           }
>            break;
>        /* Non-fatal issues (go on to next file) */
>        case NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID:
> -           /* Defer sync of maildir flags until after old filenames
> -            * are removed in the case of a rename. */
> -           if (state->synchronize_flags == TRUE)
> -               _filename_list_add (state->message_ids_to_sync,
> -                                   notmuch_message_get_message_id
> (message));
> +           if (state->synchronize_flags == TRUE) {
> +               if (!state->total_files) {
> +                   /* Defer sync of maildir flags until after old
> filenames
> +                    * are removed in the case of a rename. */
> +                   _filename_list_add (state->message_ids_to_sync,
> +                                       notmuch_message_get_message_id
> (message));
> +               } else {
> +                   /* During the first notmuch new we synchronize
> +                    * flags immediately, while the message is hot in
> +                    * disk cache. */
> +                   notmuch_message_maildir_flags_to_tags (message);
> +               }
> +           }
>            break;
>        case NOTMUCH_STATUS_FILE_NOT_EMAIL:
>            fprintf (stderr, "Note: Ignoring non-mail file: %s\n",
> --
> 1.7.2.3
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch
>

[-- Attachment #2: Type: text/html, Size: 6260 bytes --]
