From: Jani Nikula <jani@nikula.org>
To: david@tethera.net, notmuch@notmuchmail.org
Subject: Re: [Patch v2 17/17] tag-util: optimization of tag application
In-Reply-To: <1353792017-31459-18-git-send-email-david@tethera.net>
References: <1353792017-31459-1-git-send-email-david@tethera.net>
	<1353792017-31459-18-git-send-email-david@tethera.net>
User-Agent: Notmuch/0.14+124~g3b17402 (http://notmuchmail.org) Emacs/23.4.1
	(i686-pc-linux-gnu)
Date: Sun, 02 Dec 2012 16:42:12 +0200
Message-ID: <87txs4cy7v.fsf@nikula.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: David Bremner <bremner@debian.org>
Precedence: list

On Sat, 24 Nov 2012, david@tethera.net wrote:
> From: David Bremner <bremner@debian.org>
>
> The idea is not to bother with restore operations if they don't change
> the set of tags. This is actually a relatively common case.
>
> In order to avoid fancy datastructures, this method is quadratic in
> the number of tags; at least on my mail database this doesn't seem to
> be a big problem.

It's bound to be slower than the original optimization using strcmp, but
that was based on a number of assumptions we can no longer make. In any
case, not doing message sync is the key optimization.

One side effect of any optimization (also the existing one) is not doing
maildir sync from tags to flags when there are no tag changes. This just
emphasizes that one should never do dump or restore with the mail store
and database out of sync. I don't think we emphasize the importance of
running notmuch new before dump and restore enough.

A few more comments below.

BR,
Jani.

> ---
>  notmuch-tag.c |    2 +-
>  tag-util.c    |   59 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 60 insertions(+), 1 deletion(-)
>
> diff --git a/notmuch-tag.c b/notmuch-tag.c
> index 8a8af0b..e4fca67 100644
> --- a/notmuch-tag.c
> +++ b/notmuch-tag.c
> @@ -140,7 +140,7 @@ tag_query (void *ctx, notmuch_database_t *notmuch, const char *query_string,
>  	 notmuch_messages_valid (messages) && ! interrupted;
>  	 notmuch_messages_move_to_next (messages)) {
>  	message = notmuch_messages_get (messages);
> -	tag_op_list_apply (message, tag_ops, flags);
> +	tag_op_list_apply (message, tag_ops, flags | TAG_FLAG_PRE_OPTIMIZED);
>  	notmuch_message_destroy (message);
>      }
>  
> diff --git a/tag-util.c b/tag-util.c
> index 287cc67..2bb8355 100644
> --- a/tag-util.c
> +++ b/tag-util.c
> @@ -111,6 +111,62 @@ message_error (notmuch_message_t *message,
>      fprintf (stderr, "Status: %s\n", notmuch_status_to_string (status));
>  }
>  
> +static int
> +makes_changes (notmuch_message_t *message,
> +	       tag_op_list_t *list,
> +	       tag_op_flag_t flags)
> +{
> +
> +    int i;
> +
> +    notmuch_tags_t *tags;
> +    notmuch_bool_t changes = FALSE;
> +
> +    /* First, do we delete an existing tag? */
> +    changes = FALSE;
> +    for (tags = notmuch_message_get_tags (message);
> +	 ! changes && notmuch_tags_valid (tags);
> +	 notmuch_tags_move_to_next (tags)) {
> +	const char *cur_tag = notmuch_tags_get (tags);
> +	int last_op =  (flags & TAG_FLAG_REMOVE_ALL) ? -1 : 0;
> +
> +	for (i = 0; i < list->count; i++) {
> +	    if (strcmp (cur_tag, list->ops[i].tag) == 0) {
> +		last_op = list->ops[i].remove ? -1 : 1;
> +	    }
> +	}

If you looped from end to beginning, you could break on first match.

> +
> +	changes = (last_op == -1);
> +    }
> +    notmuch_tags_destroy (tags);
> +
> +    if (changes)
> +	return TRUE;
> +
> +    /* Now check for adding new tags */
> +    for (i = 0; i < list->count; i++) {
> +	notmuch_bool_t exists = FALSE;
> +

I think you can do:

	if (list->ops[i].remove)
	    continue;

here and remove the check on .remove below.

> +	for (tags = notmuch_message_get_tags (message);
> +	     notmuch_tags_valid (tags);
> +	     notmuch_tags_move_to_next (tags)) {
> +	    const char *cur_tag = notmuch_tags_get (tags);
> +	    if (strcmp (cur_tag, list->ops[i].tag) == 0) {
> +		exists = TRUE;
> +		break;
> +	    }
> +	}
> +	notmuch_tags_destroy (tags);
> +
> +	/* the following test is conservative, it's ok to think we
> +	 * make changes when we don't */

I think it's "too" conservative only when the input has e.g. "+foo
-foo", but I wouldn't worry about optimizing that.

> +	if ( ! exists && ! list->ops[i].remove )
> +	    return TRUE;
> +    }
> +    return FALSE;
> +
> +}
> +
>  notmuch_status_t
>  tag_op_list_apply (notmuch_message_t *message,
>  		   tag_op_list_t *list,
> @@ -121,6 +177,9 @@ tag_op_list_apply (notmuch_message_t *message,
>      notmuch_status_t status = 0;
>      tag_operation_t *tag_ops = list->ops;
>  
> +    if (! (flags & TAG_FLAG_PRE_OPTIMIZED) && ! makes_changes (message, list, flags))
> +	return NOTMUCH_STATUS_SUCCESS;
> +
>      status = notmuch_message_freeze (message);
>      if (status) {
>  	message_error (message, status, "freezing message");
> -- 
> 1.7.10.4
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch