From: Junio C Hamano <gitster@pobox.com>
Date: Thu, 07 Nov 2019 05:32:53 GMT
Subject: Re: [PATCH v3 1/1] fetch: add trace2 instrumentation
Message-ID: <xmqq5zjws12y.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <364c526a5d674e3f137578b24143678256601d08.1573069152.git.gitgitgadget@gmail.com>

"Erik Chen via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Erik Chen <erikchen@chromium.org>
>
> Add trace2 regions to fetch-pack.c to better track time spent in the various
> phases of a fetch:
>
>     * matching common remote and local refs
>     * marking local refs as complete (part of the matching process)
>
> Both of these stages can be slow for repositories with many refs.
>
> Signed-off-by: Erik Chen <erikchen@chromium.org>
> ---
>  fetch-pack.c | 8 ++++++++
>  1 file changed, 8 insertions(+)

OK.

> diff --git a/fetch-pack.c b/fetch-pack.c
> index 0130b44112..5e3eee0477 100644
> --- a/fetch-pack.c
> +++ b/fetch-pack.c
> @@ -669,6 +669,8 @@ static void mark_complete_and_common_ref(struct fetch_negotiator *negotiator,
>  
>  	save_commit_buffer = 0;
>  
> +	trace2_region_enter("fetch-pack", "mark_complete_and_common_ref", NULL);
> +
>  	for (ref = *refs; ref; ref = ref->next) {
>  		struct object *o;
>  
> @@ -690,6 +692,10 @@ static void mark_complete_and_common_ref(struct fetch_negotiator *negotiator,
>  		}
>  	}
>  
> +	/* This block marks all local refs as COMPLETE, and then recursively marks all
> +	 * parents of those refs as COMPLETE.
> +	 */

        /*
         * We write our multi-line comments like this, with the
         * slash-asterisk at the beginning and the asterisk-slash
         * at the end on its own line.  Learn such local conventions
         * from the existing surrounding code and imitate, which
         * would reduce stylistic errors.
         */

Will fix-up while queuing (no need to reroll only to fix this).

> +	trace2_region_enter("fetch-pack", "mark_complete_local_refs", NULL);
>  	if (!args->deepen) {
>  		for_each_ref(mark_complete_oid, NULL);
>  		for_each_cached_alternate(NULL, mark_alternate_complete);
> @@ -697,6 +703,7 @@ static void mark_complete_and_common_ref(struct fetch_negotiator *negotiator,
>  		if (cutoff)
>  			mark_recent_complete_commits(args, cutoff);
>  	}
> +	trace2_region_leave("fetch-pack", "mark_complete_local_refs", NULL);
>  
>  	/*
>  	 * Mark all complete remote refs as common refs.
> @@ -716,6 +723,7 @@ static void mark_complete_and_common_ref(struct fetch_negotiator *negotiator,
>  	}
>  
>  	save_commit_buffer = old_save_commit_buffer;
> +	trace2_region_leave("fetch-pack", "mark_complete_and_common_ref", NULL);


So this introduces a single region around the entire function body
of mark_complete_and_common_ref(), within which only one subpart is
also enclosed in a nested region.  Is that because the parts inside
the outer region before and after the inner region are known to
consume negligible time?  IOW I would understand

        F () {
                <region 1 begin>
                    <region 1.1 begin>
                        code
                    <region 1.1 end>
                    <region 1.2 begin>
                        code
                    <region 1.2 end>
                    <region 1.3 begin>
                        code
                    <region 1.3 end>
                <region 1 end>
        }

or

        F () {
                        trivial code
                <region 1 begin>
                        heavy code
                <region 1 end>
                        trivial code
        }

but this appears to do


        F () {
                <region 1 begin>
                        code
                    <region 1.1 begin>
                        code
                    <region 1.1 end>
                        code
                <region 1 end>
        }

which is somewhat puzzling.

Thanks.