Re: [PATCH v4 2/5] unpack-trees: add performance tracing

From: Ben Peart <peartben@gmail.com>
Date: Aug 13, 2018, 18:44 UTC
Message-ID: <34bbad06-4b19-6558-3004-58b0498a5166@gmail.com>
In-Reply-To: <20180812081551.27927-3-pclouds@gmail.com>

On 8/12/2018 4:15 AM, Nguyễn Thái Ngọc Duy wrote:

Show 34 quoted lines

> We're going to optimize unpack_trees() a bit in the following
> patches. Let's add some tracing to measure how long it takes before
> and after. This is the baseline ("git checkout -" on webkit.git, 275k
> files on worktree)
> 
>      performance: 0.056651714 s:  read cache .git/index
>      performance: 0.183101080 s:  preload index
>      performance: 0.008584433 s:  refresh index
>      performance: 0.633767589 s:   traverse_trees
>      performance: 0.340265448 s:   check_updates
>      performance: 0.381884638 s:   cache_tree_update
>      performance: 1.401562947 s:  unpack_trees
>      performance: 0.338687914 s:  write index, changed mask = 2e
>      performance: 0.411927922 s:    traverse_trees
>      performance: 0.000023335 s:    check_updates
>      performance: 0.423697246 s:   unpack_trees
>      performance: 0.423708360 s:  diff-index
>      performance: 2.559524127 s: git command: git checkout -
> 
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>   cache-tree.c   | 2 ++
>   unpack-trees.c | 9 ++++++++-
>   2 files changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/cache-tree.c b/cache-tree.c
> index 6b46711996..105f13806f 100644
> --- a/cache-tree.c
> +++ b/cache-tree.c
> @@ -433,7 +433,9 @@ int cache_tree_update(struct index_state *istate, int flags)
>   
>   	if (i)
>   		return i;
> +	trace_performance_enter();

This one is a little odd to me. I think the either the trace_performance_enter() call should move up to include the verify_cache() call or the enter/leave should move into the update_one() call as that is all it is measuring/reporting on.

Show 41 quoted lines

>   	i = update_one(it, cache, entries, "", 0, &skip, flags);
> +	trace_performance_leave("cache_tree_update");
>   	if (i < 0)
>   		return i;
>   	istate->cache_changed |= CACHE_TREE_CHANGED;
> diff --git a/unpack-trees.c b/unpack-trees.c
> index cd0680f11e..b237eaa0f2 100644
> --- a/unpack-trees.c
> +++ b/unpack-trees.c
> @@ -354,6 +354,7 @@ static int check_updates(struct unpack_trees_options *o)
>   	struct checkout state = CHECKOUT_INIT;
>   	int i;
>   
> +	trace_performance_enter();
>   	state.force = 1;
>   	state.quiet = 1;
>   	state.refresh_cache = 1;
> @@ -423,6 +424,7 @@ static int check_updates(struct unpack_trees_options *o)
>   	errs |= finish_delayed_checkout(&state);
>   	if (o->update)
>   		git_attr_set_direction(GIT_ATTR_CHECKIN, NULL);
> +	trace_performance_leave("check_updates");
>   	return errs != 0;
>   }
>   
> @@ -1279,6 +1281,7 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
>   	if (len > MAX_UNPACK_TREES)
>   		die("unpack_trees takes at most %d trees", MAX_UNPACK_TREES);
>   
> +	trace_performance_enter();
>   	memset(&el, 0, sizeof(el));
>   	if (!core_apply_sparse_checkout || !o->update)
>   		o->skip_sparse_checkout = 1;
> @@ -1351,7 +1354,10 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
>   			}
>   		}
>   
> -		if (traverse_trees(len, t, &info) < 0)
> +		trace_performance_enter();
> +		ret = traverse_trees(len, t, &info);
> +		trace_performance_leave("traverse_trees");

Why not move this enter/leave pair into the traverse_trees() function itself?

Show 13 quoted lines

> +		if (ret < 0)
>   			goto return_failed;
>   	}
>   
> @@ -1443,6 +1449,7 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
>   	o->src_index = NULL;
>   
>   done:
> +	trace_performance_leave("unpack_trees");
>   	clear_exclude_list(&el);
>   	return ret;
>   
>