Re: [PATCH v3 1/1] fetch: add trace2 instrumentation

> "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.