From: Mark Walters <markwalters1009@gmail.com>
To: david@tethera.net, notmuch@notmuchmail.org
Subject: Re: [Patch v3b 9/9] tag-util: optimization of tag application
In-Reply-To: <1354843607-17980-10-git-send-email-david@tethera.net>
References: <1354843607-17980-1-git-send-email-david@tethera.net>
	<1354843607-17980-10-git-send-email-david@tethera.net>
User-Agent: Notmuch/0.14+81~g9730584 (http://notmuchmail.org) Emacs/23.4.1
	(x86_64-pc-linux-gnu)
Date: Sat, 08 Dec 2012 11:22:53 +0000
Message-ID: <87zk1og54i.fsf@qmul.ac.uk>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: David Bremner <bremner@debian.org>
Precedence: list

On Fri, 07 Dec 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.
> ---
>  tag-util.c |   66 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 66 insertions(+)
>
> diff --git a/tag-util.c b/tag-util.c
> index 932ee7f..3d54e9e 100644
> --- a/tag-util.c
> +++ b/tag-util.c
> @@ -124,6 +124,69 @@ 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)
> +{
> +
> +    size_t 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;
> +
> +	/* slight contortions to count down with an unsigned index */
> +	for (i = list->count; i-- > 0; /*nothing*/) {
> +	    if (strcmp (cur_tag, list->ops[i].tag) == 0) {
> +		last_op = list->ops[i].remove ? -1 : 1;
> +		break;
> +	    }
> +	}

I agree that this is a little ugly but ok I think. Is it worth adding a
comment as to why you are counting backwards? eg " we count backwards to
check whether the last change for the tag foo is removal"

But basically LGTM

Mark


> +
> +	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;
> +
> +	if (list->ops[i].remove)
> +	    continue;
> +
> +	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,
> +	 * in the sense it ignores cases like +foo ... -foo
> +	 * but this is OK from a correctness point of view
> +	 */
> +	if (! exists)
> +	    return TRUE;
> +    }
> +    return FALSE;
> +
> +}
> +
>  notmuch_status_t
>  tag_op_list_apply (notmuch_message_t *message,
>  		   tag_op_list_t *list,
> @@ -133,6 +196,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