[PATCH 0/3] Speedups and enhancements of notmuch new

[PATCH 0/3] Speedups and enhancements of notmuch new

[Michal Sojka]

Hi all,

after yesterday discussion on IRC I looked at some ways how to speedup
the initial run of notmuch new. It turned out that with a few pretty
simple changes I'm able to reduce the time by about 30% (from 1h 46m
to 1h 14m on ext4, 200k messages, 3GB).

-Michal

Michal Sojka (3):
  new: Do not defer maildir flag synchronization during the first run
  new: Add all initial tags at once
  new: Enhance progress reporting

 notmuch-new.c |  194 +++++++++++++++++++++++++++++++++++++--------------------
 1 files changed, 126 insertions(+), 68 deletions(-)

-- 
1.7.2.3

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

[Michal Sojka]

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

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

[Michal Sojka]

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

[PATCH 2/3] new: Add all initial tags at once

[Michal Sojka]

If there are several tags applied to the new messages, it is beneficial
to store them to the database at one, because it saves some time,
especially when the notmuch new is run for the first time.

This patch decreased the time for initial import from 1h 35m to 1h 14m.
---
 notmuch-new.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/notmuch-new.c b/notmuch-new.c
index a2af045..d71e497 100644
--- a/notmuch-new.c
+++ b/notmuch-new.c
@@ -418,6 +418,7 @@ add_files_recursive (notmuch_database_t *notmuch,
 	/* success */
 	case NOTMUCH_STATUS_SUCCESS:
 	    state->added_messages++;
+	    notmuch_message_freeze (message);
 	    for (tag=state->new_tags; *tag != NULL; tag++)
 	        notmuch_message_add_tag (message, *tag);
 	    if (state->synchronize_flags == TRUE) {
@@ -433,6 +434,7 @@ add_files_recursive (notmuch_database_t *notmuch,
 		    notmuch_message_maildir_flags_to_tags (message);
 		}
 	    }
+	    notmuch_message_thaw (message);
 	    break;
 	/* Non-fatal issues (go on to next file) */
 	case NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID:
-- 
1.7.2.3

[PATCH 3/3] new: Enhance progress reporting

[Michal Sojka]

notmuch new reports progress only during the "first" phase when the
files on disk are traversed and indexed. After this phase, other
operations like rename detection and maildir flags synchronization are
performed, but the user is not informed about them. Since these
operations can take significant time, we want to inform the user about
them.

This patch enhances the progress reporting facility that was already
present. The timer that triggers reporting is not stopped after the
first phase but continues to run until all operations are finished. The
rename detection and maildir flag synchronization are enhanced to report
their progress.
---
 notmuch-new.c |  156 ++++++++++++++++++++++++++++++++++++---------------------
 1 files changed, 98 insertions(+), 58 deletions(-)

diff --git a/notmuch-new.c b/notmuch-new.c
index d71e497..fa7a76d 100644
--- a/notmuch-new.c
+++ b/notmuch-new.c
@@ -28,6 +28,7 @@ typedef struct _filename_node {
 } _filename_node_t;
 
 typedef struct _filename_list {
+    unsigned count;
     _filename_node_t *head;
     _filename_node_t **tail;
 } _filename_list_t;
@@ -50,12 +51,12 @@ typedef struct {
     _filename_list_t *message_ids_to_sync;
 } add_files_state_t;
 
-static volatile sig_atomic_t do_add_files_print_progress = 0;
+static volatile sig_atomic_t do_print_progress = 0;
 
 static void
 handle_sigalrm (unused (int signal))
 {
-    do_add_files_print_progress = 1;
+    do_print_progress = 1;
 }
 
 static volatile sig_atomic_t interrupted;
@@ -81,6 +82,7 @@ _filename_list_create (const void *ctx)
 
     list->head = NULL;
     list->tail = &list->head;
+    list->count = 0;
 
     return list;
 }
@@ -91,6 +93,8 @@ _filename_list_add (_filename_list_t *list,
 {
     _filename_node_t *node = talloc (list, _filename_node_t);
 
+    list->count++;
+
     node->filename = talloc_strdup (list, filename);
     node->next = NULL;
 
@@ -99,28 +103,28 @@ _filename_list_add (_filename_list_t *list,
 }
 
 static void
-add_files_print_progress (add_files_state_t *state)
+generic_print_progress (const char *action, const char *object,
+			struct timeval tv_start, unsigned processed, unsigned total)
 {
     struct timeval tv_now;
     double elapsed_overall, rate_overall;
 
     gettimeofday (&tv_now, NULL);
 
-    elapsed_overall = notmuch_time_elapsed (state->tv_start, tv_now);
-    rate_overall = (state->processed_files) / elapsed_overall;
+    elapsed_overall = notmuch_time_elapsed (tv_start, tv_now);
+    rate_overall = processed / elapsed_overall;
 
-    printf ("Processed %d", state->processed_files);
+    printf ("%s %d ", action, processed);
 
-    if (state->total_files) {
+    if (total) {
 	double time_remaining;
 
-	time_remaining = ((state->total_files - state->processed_files) /
-			  rate_overall);
-	printf (" of %d files (", state->total_files);
+	time_remaining = ((total - processed) / rate_overall);
+	printf ("of %d %s (", total, object);
 	notmuch_time_print_formatted_seconds (time_remaining);
-	printf (" remaining).      \r");
+	printf (" remaining).\033[K\r");
     } else {
-	printf (" files (%d files/sec.)    \r", (int) rate_overall);
+	printf ("%s (%d %s/sec.)\033[K\r", object, (int) rate_overall, object);
     }
 
     fflush (stdout);
@@ -479,9 +483,10 @@ add_files_recursive (notmuch_database_t *notmuch,
 	    message = NULL;
 	}
 
-	if (do_add_files_print_progress) {
-	    do_add_files_print_progress = 0;
-	    add_files_print_progress (state);
+	if (do_print_progress) {
+	    do_print_progress = 0;
+	    generic_print_progress ("Processed", "files", state->tv_start,
+				    state->processed_files, state->total_files);
 	}
 
 	talloc_free (next);
@@ -540,38 +545,56 @@ add_files_recursive (notmuch_database_t *notmuch,
     return ret;
 }
 
+static void
+setup_progress_printing_timer (void)
+{
+    struct sigaction action;
+    struct itimerval timerval;
+
+    /* Setup our handler for SIGALRM */
+    memset (&action, 0, sizeof (struct sigaction));
+    action.sa_handler = handle_sigalrm;
+    sigemptyset (&action.sa_mask);
+    action.sa_flags = SA_RESTART;
+    sigaction (SIGALRM, &action, NULL);
+
+    /* Then start a timer to send SIGALRM once per second. */
+    timerval.it_interval.tv_sec = 1;
+    timerval.it_interval.tv_usec = 0;
+    timerval.it_value.tv_sec = 1;
+    timerval.it_value.tv_usec = 0;
+    setitimer (ITIMER_REAL, &timerval, NULL);
+}
+
+static void
+stop_progress_printing_timer (void)
+{
+    struct sigaction action;
+    struct itimerval timerval;
+
+    /* Now stop the timer. */
+    timerval.it_interval.tv_sec = 0;
+    timerval.it_interval.tv_usec = 0;
+    timerval.it_value.tv_sec = 0;
+    timerval.it_value.tv_usec = 0;
+    setitimer (ITIMER_REAL, &timerval, NULL);
+
+    /* And disable the signal handler. */
+    action.sa_handler = SIG_IGN;
+    sigaction (SIGALRM, &action, NULL);
+}
+
+
 /* This is the top-level entry point for add_files. It does a couple
- * of error checks, sets up the progress-printing timer and then calls
- * into the recursive function. */
+ * of error checks and then calls into the recursive function. */
 static notmuch_status_t
 add_files (notmuch_database_t *notmuch,
 	   const char *path,
 	   add_files_state_t *state)
 {
     notmuch_status_t status;
-    struct sigaction action;
-    struct itimerval timerval;
-    notmuch_bool_t timer_is_active = FALSE;
     struct stat st;
 
-    if (state->output_is_a_tty && ! debugger_is_active () && ! state->verbose) {
-	/* Setup our handler for SIGALRM */
-	memset (&action, 0, sizeof (struct sigaction));
-	action.sa_handler = handle_sigalrm;
-	sigemptyset (&action.sa_mask);
-	action.sa_flags = SA_RESTART;
-	sigaction (SIGALRM, &action, NULL);
-
-	/* Then start a timer to send SIGALRM once per second. */
-	timerval.it_interval.tv_sec = 1;
-	timerval.it_interval.tv_usec = 0;
-	timerval.it_value.tv_sec = 1;
-	timerval.it_value.tv_usec = 0;
-	setitimer (ITIMER_REAL, &timerval, NULL);
-
-	timer_is_active = TRUE;
-    }
-
     if (stat (path, &st)) {
 	fprintf (stderr, "Error reading directory %s: %s\n",
 		 path, strerror (errno));
@@ -585,19 +608,6 @@ add_files (notmuch_database_t *notmuch,
 
     status = add_files_recursive (notmuch, path, state);
 
-    if (timer_is_active) {
-	/* Now stop the timer. */
-	timerval.it_interval.tv_sec = 0;
-	timerval.it_interval.tv_usec = 0;
-	timerval.it_value.tv_sec = 0;
-	timerval.it_value.tv_usec = 0;
-	setitimer (ITIMER_REAL, &timerval, NULL);
-
-	/* And disable the signal handler. */
-	action.sa_handler = SIG_IGN;
-	sigaction (SIGALRM, &action, NULL);
-    }
-
     return status;
 }
 
@@ -746,7 +756,7 @@ notmuch_new_command (void *ctx, int argc, char *argv[])
     notmuch_database_t *notmuch;
     add_files_state_t add_files_state;
     double elapsed;
-    struct timeval tv_now;
+    struct timeval tv_now, tv_start;
     int ret = 0;
     struct stat st;
     const char *db_path;
@@ -756,6 +766,7 @@ notmuch_new_command (void *ctx, int argc, char *argv[])
     int renamed_files, removed_files;
     notmuch_status_t status;
     int i;
+    notmuch_bool_t timer_is_active = FALSE;
 
     add_files_state.verbose = 0;
     add_files_state.output_is_a_tty = isatty (fileno (stdout));
@@ -768,7 +779,6 @@ notmuch_new_command (void *ctx, int argc, char *argv[])
 	    return 1;
 	}
     }
-
     config = notmuch_config_open (ctx, NULL, NULL);
     if (config == NULL)
 	return 1;
@@ -831,21 +841,41 @@ notmuch_new_command (void *ctx, int argc, char *argv[])
     add_files_state.removed_files = _filename_list_create (ctx);
     add_files_state.removed_directories = _filename_list_create (ctx);
 
+    if (! debugger_is_active () && add_files_state.output_is_a_tty
+	&& ! add_files_state.verbose) {
+	setup_progress_printing_timer ();
+	timer_is_active = TRUE;
+    }
+
     ret = add_files (notmuch, db_path, &add_files_state);
 
     removed_files = 0;
     renamed_files = 0;
+    gettimeofday (&tv_start, NULL);
     for (f = add_files_state.removed_files->head; f; f = f->next) {
 	status = notmuch_database_remove_message (notmuch, f->filename);
 	if (status == NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID)
 	    renamed_files++;
 	else
 	    removed_files++;
+	if (do_print_progress) {
+	    do_print_progress = 0;
+	    generic_print_progress ("Cleaned up", "messages",
+		tv_start, removed_files + renamed_files,
+		add_files_state.removed_files->count);
+	}
     }
 
-    for (f = add_files_state.removed_directories->head; f; f = f->next) {
+    gettimeofday (&tv_start, NULL);
+    for (f = add_files_state.removed_directories->head, i = 0; f; f = f->next, i++) {
 	_remove_directory (ctx, notmuch, f->filename,
 			   &renamed_files, &removed_files);
+	if (do_print_progress) {
+	    do_print_progress = 0;
+	    generic_print_progress ("Cleaned up", "directories",
+		tv_start, i,
+		add_files_state.removed_directories->count);
+	}
     }
 
     talloc_free (add_files_state.removed_files);
@@ -854,22 +884,32 @@ notmuch_new_command (void *ctx, int argc, char *argv[])
     /* Now that removals are done (hence the database is aware of all
      * renames), we can synchronize maildir_flags to tags for all
      * messages that had new filenames appear on this run. */
+    gettimeofday (&tv_start, NULL);
     if (add_files_state.synchronize_flags) {
 	_filename_node_t *node;
 	notmuch_message_t *message;
-	for (node = add_files_state.message_ids_to_sync->head;
+	for (node = add_files_state.message_ids_to_sync->head, i = 0;
 	     node;
-	     node = node->next)
+	     node = node->next, i++)
 	{
 	    message = notmuch_database_find_message (notmuch, node->filename);
 	    notmuch_message_maildir_flags_to_tags (message);
 	    notmuch_message_destroy (message);
+	    if (do_print_progress) {
+		do_print_progress = 0;
+		generic_print_progress (
+		    "Synchronized tags for", "messages",
+		    tv_start, i, add_files_state.message_ids_to_sync->count);
+	    }
 	}
     }
 
     talloc_free (add_files_state.message_ids_to_sync);
     add_files_state.message_ids_to_sync = NULL;
 
+    if (timer_is_active)
+	stop_progress_printing_timer ();
+
     gettimeofday (&tv_now, NULL);
     elapsed = notmuch_time_elapsed (add_files_state.tv_start,
 				    tv_now);
@@ -880,10 +920,10 @@ notmuch_new_command (void *ctx, int argc, char *argv[])
 		"file" : "total files");
 	notmuch_time_print_formatted_seconds (elapsed);
 	if (elapsed > 1) {
-	    printf (" (%d files/sec.).                 \n",
+	    printf (" (%d files/sec.).\033[K\n",
 		    (int) (add_files_state.processed_files / elapsed));
 	} else {
-	    printf (".                    \n");
+	    printf (".\033[K\n");
 	}
     }
 
-- 
1.7.2.3

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

[Austin Clements]

[-- 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 --]

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

[Carl Worth]

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

On Tue, 25 Jan 2011 17:42:30 -0500, Austin Clements <amdragon@mit.edu> wrote:
> Wouldn't this be simpler and more general?
...
>         case NOTMUCH_STATUS_SUCCESS:
..
>             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);

Yes, that is much simpler and should work equally well as the original
patch.

But there's perhaps a problem with both of these patches. Besides
rename, (which obviously can't happen with a new message), we also need
to take care when a message is added with multiple filenames (and with
different flags on the files).

We've got a plan for adding flags-to-tags mappings which only apply if
every file for the message has the corresponding flag. For example, this
is the semantic we want for the 'D' flag mapping to the "deleted" tag.

So we'll want to make sure these cases do the right thing. Consider two
new files with the same Message-Id both appearing in a run of "notmuch
new", one with the D flag and one without.

If the file with the D flag is seen first, and the maildir_flags_to_tags
processing happens without being deferred, then the "deleted" tag will
be applied to the message. This is different than would happen if both
messages were seen, but I think it's just fine. It's still in a state
that's consistent, nothing bad would happen if you interrupted this and
then acted on the "deleted" tag, and if you restarted "notmuch new" and
the second message were seen, then the tag would be correctly removed.

So, I think I've convinced myself that the change is actually OK. But
then I'm also wondering if perhaps we could do the processing undeferred
in all cases?

I haven't thought that through, but I'd be glad to hear your ideas.

-Carl

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

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

[Carl Worth]

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

On Wed, 26 Jan 2011 19:15:21 +1000, Carl Worth <cworth@cworth.org> wrote:
> Yes, that is much simpler and should work equally well as the original
> patch.
...
> So, I think I've convinced myself that the change is actually OK.

For those reasons, I'm pushing the patch now.

> But then I'm also wondering if perhaps we could do the processing undeferred
> in all cases?
> 
> I haven't thought that through, but I'd be glad to hear your ideas.

This is still an open question if anyone wants to pursue it, (it might
make it simpler to then fix the atomicity bugs with adding new messages
to the database, and only later adjusting the maildir filename).

On that topic, if we decide we do need to defer the tags/flags mapping,
then the real fix is to probably also defer the addition of the filename
to the message document in the database. If we change these things only
at the same time, then we should be able to avoid any problems with
things getting out of synchronization.

-Carl

-- 
carl.d.worth@intel.com

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH 2/3] new: Add all initial tags at once

[Carl Worth]

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

On Fri, 21 Jan 2011 10:59:36 +0100, Michal Sojka <sojkam1@fel.cvut.cz> wrote:
> If there are several tags applied to the new messages, it is beneficial
> to store them to the database at one, because it saves some time,
> especially when the notmuch new is run for the first time.
> 
> This patch decreased the time for initial import from 1h 35m to 1h
> 14m.

Thanks. This is pushed now.

-Carl

-- 
carl.d.worth@intel.com

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH 3/3] new: Enhance progress reporting

[Carl Worth]

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

On Fri, 21 Jan 2011 10:59:37 +0100, Michal Sojka <sojkam1@fel.cvut.cz> wrote:
> This patch enhances the progress reporting facility that was already
> present. The timer that triggers reporting is not stopped after the
> first phase but continues to run until all operations are finished. The
> rename detection and maildir flag synchronization are enhanced to report
> their progress.

Looks good to me. This is pushed.

-Carl

-- 
carl.d.worth@intel.com

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

[PATCH] new: Print progress estimates only when we have sufficient information

[Michal Sojka]

Without this patch, it might happen that the remaining time or processing
rate were calculated just after start where nothing was processed yet.
This resulted into division by a very small number (or zero) and the
printed information was of little value.

Instead of printing nonsenses we print only that the operation is in
progress. The estimates will be printed later, after there is enough data.
---
 notmuch-new.c |   18 +++++++++++-------
 1 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/notmuch-new.c b/notmuch-new.c
index fa7a76d..8f64b25 100644
--- a/notmuch-new.c
+++ b/notmuch-new.c
@@ -117,15 +117,19 @@ generic_print_progress (const char *action, const char *object,
     printf ("%s %d ", action, processed);
 
     if (total) {
-	double time_remaining;
-
-	time_remaining = ((total - processed) / rate_overall);
-	printf ("of %d %s (", total, object);
-	notmuch_time_print_formatted_seconds (time_remaining);
-	printf (" remaining).\033[K\r");
+	printf ("of %d %s", total, object);
+	if (processed > 0 && elapsed_overall > 0.5) {
+	    double time_remaining = ((total - processed) / rate_overall);
+	    printf (" (");
+	    notmuch_time_print_formatted_seconds (time_remaining);
+	    printf (" remaining)");
+	}
     } else {
-	printf ("%s (%d %s/sec.)\033[K\r", object, (int) rate_overall, object);
+	printf ("%s", object);
+	if (elapsed_overall > 0.5)
+	    printf (" (%d %s/sec.)", (int) rate_overall, object);
     }
+    printf (".\033[K\r");
 
     fflush (stdout);
 }
-- 
1.7.2.3

Re: [PATCH 3/3] new: Enhance progress reporting

[Michal Sojka]

On Wed, 26 Jan 2011, Carl Worth wrote:
> On Fri, 21 Jan 2011 10:59:37 +0100, Michal Sojka <sojkam1@fel.cvut.cz> wrote:
> > This patch enhances the progress reporting facility that was already
> > present. The timer that triggers reporting is not stopped after the
> > first phase but continues to run until all operations are finished. The
> > rename detection and maildir flag synchronization are enhanced to report
> > their progress.
> 
> Looks good to me. This is pushed.

I've just sent a small fix to this. Please apply it as well.

M.

Re: [PATCH] new: Print progress estimates only when we have sufficient information

[Carl Worth]

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

On Wed, 26 Jan 2011 14:06:57 +0100, Michal Sojka <sojkam1@fel.cvut.cz> wrote:
> Without this patch, it might happen that the remaining time or processing
> rate were calculated just after start where nothing was processed yet.

Thanks. Pushed now.

-Carl

-- 
carl.d.worth@intel.com

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

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

[Austin Clements]

Quoth Carl Worth on Jan 26 at  9:59 pm:
> On Wed, 26 Jan 2011 19:15:21 +1000, Carl Worth <cworth@cworth.org> wrote:
> > But then I'm also wondering if perhaps we could do the processing undeferred
> > in all cases?
> > 
> > I haven't thought that through, but I'd be glad to hear your ideas.
> 
> This is still an open question if anyone wants to pursue it, (it might
> make it simpler to then fix the atomicity bugs with adding new messages
> to the database, and only later adjusting the maildir filename).

I believe you're right that synchronization could always be done
eagerly.  In fact, I believe this is true even with the addition of
conjunctive and disjunctive flags.

When adding or removing messages, flag synchronization fully dictates
all tags in flag2tag (that is, the tags' current states don't matter).
It also always examines *all* file names backing the message.  This is
a stateless process.  With eager synchronization, the tags may go
through some transient states during notmuch new, but will always
settle down to the correct set as of the final add or remove for a
given message ID.

> On that topic, if we decide we do need to defer the tags/flags mapping,
> then the real fix is to probably also defer the addition of the filename
> to the message document in the database. If we change these things only
> at the same time, then we should be able to avoid any problems with
> things getting out of synchronization.

I'd been thinking about this as a way to break up notmuch new into
small, consistent transactions, but I don't think it's necessary since
flags can be synchronized eagerly.  In fact, with eager
synchronization and one other change, I believe notmuch new can be
completely correctly performed in lots of small transactions.

The one other change is that currently, if notmuch is interrupted
after updating a directory mtime but before processing file removals,
a subsequent notmuch new will miss those removals.  This could be
brushed under the rug, since notmuch will notice the removal after any
other change in that directory.  Or it could be handled correctly by
giving directories a "removal mtime" in addition to their current
"addition mtime" that is only updated after removals are handled.

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

[Michal Sojka]

On Wed, 26 Jan 2011, Austin Clements wrote:
> The one other change is that currently, if notmuch is interrupted
> after updating a directory mtime but before processing file removals,
> a subsequent notmuch new will miss those removals.  This could be
> brushed under the rug, since notmuch will notice the removal after any
> other change in that directory.  Or it could be handled correctly by
> giving directories a "removal mtime" in addition to their current
> "addition mtime" that is only updated after removals are handled.

Yes, that might be a problem if notmuch new is interrupted by something
like SIGKILL or power-off. There is protection against interruption by
Ctrl-C.

-Michal

Re: [PATCH 2/3] new: Add all initial tags at once

[Thomas Schwinge]

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

Hallo!

On Fri, 21 Jan 2011 10:59:36 +0100, Michal Sojka <sojkam1@fel.cvut.cz> wrote:
> --- a/notmuch-new.c
> +++ b/notmuch-new.c
> @@ -418,6 +418,7 @@ add_files_recursive (notmuch_database_t *notmuch,
>  	/* success */
>  	case NOTMUCH_STATUS_SUCCESS:
>  	    state->added_messages++;
> +	    notmuch_message_freeze (message);
>  	    for (tag=state->new_tags; *tag != NULL; tag++)
>  	        notmuch_message_add_tag (message, *tag);
>  	    if (state->synchronize_flags == TRUE) {
> @@ -433,6 +434,7 @@ add_files_recursive (notmuch_database_t *notmuch,
>  		    notmuch_message_maildir_flags_to_tags (message);
>  		}
>  	    }
> +	    notmuch_message_thaw (message);
>  	    break;
>  	/* Non-fatal issues (go on to next file) */
>  	case NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID:

I do support the patch's idea (which was recently committed; and what
follows in this message is not at all directed towards Michal, who wrote
this patch) -- but what about return values checking?  This is one aspect
of the notmuch C code (which I generally consider to be nice to read and
of high quality, as I said before already), that I consider totally
lacking -- there are literally hundreds of C functions calls where the
return values are just discarded.  This is bad.  For example (simulating
a full disk):

    $ notmuch dump > /dev/full
    $ echo $?
    0

The command returned ``success'' -- but no data has been saved.  This
could, in some extreme (but still likely) case mean: total tag data loss.
(This is likely due to printf / close or fprintf / fclose (yes, really,
(f)close! -- think of buffering) not getting their return values
checked.)  Here is what is expected:

    $ echo foo > /dev/full
    bash: echo: write error: No space left on device
    $ echo $?
    1

Then, in the few (!) lines quoted above, there are (exactly / only) calls
to notmuch_message_freeze, notmuch_message_add_tag,
notmuch_message_maildir_flags_to_tags, notmuch_message_thaw -- each of
which can fail, will return this via the notmuch_status_t return value
(whose possible values are documented, too), but none has their return
value checked.

Other languages have the concept of exceptions; C doesn't, so we're
supposed to put some ``ABORT_IF_NOT_NOTMUCH_STATUS_SUCCESS(ret)''
statements after each and every non-void (etc.) C function call.  Or make
the functions abort themselves (which is not a too good idea, as we
surely agree).  Or use a different programming language -- now, at the
present state, it wouldn't be too painful to switch, in my opinion.  (I
won't suggest any specific language, though.)  If staying with C (which I
don't object, either), then this needs a whole code audit, and a lot of
discipline in the future.

Comments?  (And I hope this doesn't sound too harsh :-) -- but it is a
serious programming issue.)


Grüße,
 Thomas

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH 2/3] new: Add all initial tags at once

[Carl Worth]

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

On Wed, 26 Jan 2011 17:52:53 +0100, Thomas Schwinge <thomas@schwinge.name> wrote:
> I do support the patch's idea (which was recently committed; and what
> follows in this message is not at all directed towards Michal, who wrote
> this patch) -- but what about return values checking?  This is one aspect
> of the notmuch C code (which I generally consider to be nice to read and
> of high quality, as I said before already), that I consider totally
> lacking -- there are literally hundreds of C functions calls where the
> return values are just discarded.  This is bad.  For example (simulating
> a full disk):
> 
>     $ notmuch dump > /dev/full
>     $ echo $?
>     0

All very well pointed out. This is clearly something we need to fix.

> Other languages have the concept of exceptions; C doesn't, so we're
> supposed to put some ``ABORT_IF_NOT_NOTMUCH_STATUS_SUCCESS(ret)''
> statements after each and every non-void (etc.) C function call.  Or make
> the functions abort themselves (which is not a too good idea, as we
> surely agree).  Or use a different programming language -- now, at the
> present state, it wouldn't be too painful to switch, in my opinion.  (I
> won't suggest any specific language, though.)

I wouldn't have any problem with anyone re-implementing notmuch in some
other language than C. But that's not something I would be likely to
work on myself, I don't think. 

>                                                If staying with C (which I
> don't object, either), then this needs a whole code audit, and a lot of
> discipline in the future.

Even a code audit and developer discipline won't be enough here. We'll
still miss things.

What we need is exhaustive testing. A great approach is to take calls
like malloc, open, read, write, etc. and at each site, fork() and fail
the call along one path, (which should then exit with a failure), and
then let the other path continue.

Just a few hours ago I attended an interesting talk by Rusty Russell in
which he talks about a CCAN module he has written called failtest which
provides an implementation of this kind of testing.

I'd love to see something like that integrated with notmuch.

> Comments?  (And I hope this doesn't sound too harsh :-) -- but it is a
> serious programming issue.)

Please don't apologize! It would be a shame if people didn't share
problems they notice in the code. Being able to hear those kinds of
things is one of the great benefits I get from publishing this code as
free software.

So, please, keep the suggestions coming!

-Carl

-- 
carl.d.worth@intel.com

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

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

[Carl Worth]

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

On Wed, 26 Jan 2011 10:07:28 -0500, Austin Clements <amdragon@MIT.EDU> wrote:
> Quoth Carl Worth on Jan 26 at  9:59 pm:
> 
> I believe you're right that synchronization could always be done
> eagerly.  In fact, I believe this is true even with the addition of
> conjunctive and disjunctive flags.

Excellent! Want to take a whack at implementing that? And seeing if you
can find any test cases that stress this more than existing test cases
might?

-Carl

-- 
carl.d.worth@intel.com

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

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

[Austin Clements]

Sure. I've been wanting to take a crack at notmuch new's atomicity for a while. Though you'll have to get through some of my outstanding patches. I can only keep so many branches in my head. ]:--8)

rlb, you expressed an interest in solving this problem, too. Did you make any headway?

"Carl Worth" <cworth@cworth.org> wrote:

>On Wed, 26 Jan 2011 10:07:28 -0500, Austin Clements <amdragon@MIT.EDU>
>wrote:
>> Quoth Carl Worth on Jan 26 at  9:59 pm:
>> 
>> I believe you're right that synchronization could always be done
>> eagerly.  In fact, I believe this is true even with the addition of
>> conjunctive and disjunctive flags.
>
>Excellent! Want to take a whack at implementing that? And seeing if you
>can find any test cases that stress this more than existing test cases
>might?
>
>-Carl
>
>-- 
>carl.d.worth@intel.com

-- 
Sent from my Android. Please excuse my brevity.

Re: [PATCH 2/3] new: Add all initial tags at once

[Carl Worth]

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

On Thu, 27 Jan 2011 15:03:49 +1000, Carl Worth <cworth@cworth.org> wrote:
> Just a few hours ago I attended an interesting talk by Rusty Russell in
> which he talks about a CCAN module he has written called failtest which
> provides an implementation of this kind of testing.

I meant to include some links with the above. CCAN is here:

	http://ccan.ozlabs.org/

And the failtest module is here:

	http://ccan.ozlabs.org/info/failtest.html

I talked to Rusty about adding copyright attribution and a one-line
pointer to the LICENSE file to this. Once that's in place, we could
incorporate failtest.c into notmuch if it would be useful.

-Carl

-- 
carl.d.worth@intel.com

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH 2/3] new: Add all initial tags at once

[Michal Sojka]

On Thu, 27 Jan 2011, Carl Worth wrote:
> On Thu, 27 Jan 2011 15:03:49 +1000, Carl Worth <cworth@cworth.org> wrote:
> > Just a few hours ago I attended an interesting talk by Rusty Russell in
> > which he talks about a CCAN module he has written called failtest which
> > provides an implementation of this kind of testing.
> 
> I meant to include some links with the above. CCAN is here:
> 
> 	http://ccan.ozlabs.org/
> 
> And the failtest module is here:
> 
> 	http://ccan.ozlabs.org/info/failtest.html

Yes, this looks interesting. We may also want to use GCC function
attribute "__attribute__ ((warn_unused_result))" to get warnings when
the returned value is not checked.

-Michal

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

[Rob Browning]

Austin Clements <amdragon@MIT.EDU> writes:

> Sure. I've been wanting to take a crack at notmuch new's atomicity for
> a while. Though you'll have to get through some of my outstanding
> patches. I can only keep so many branches in my head. ]:--8)
>
> rlb, you expressed an interest in solving this problem, too. Did you
> make any headway?

No, I haven't done anything there yet.

Thanks
-- 
Rob Browning
rlb @defaultvalue.org and @debian.org
GPG as of 2002-11-03 14DD 432F AE39 534D B592 F9A0 25C8 D377 8C7E 73A4