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

[Erik Chen <erikchen@chromium.org>]

On 11/18/19 5:55 PM, Junio C Hamano wrote:
> Derrick Stolee <stolee@gmail.com> writes:
>
>> On 11/7/2019 12:32 AM, Junio C Hamano wrote:
>>> 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 () {
>>> ...
>>>          }
>>>
>>> or
>>>
>>>          F () {
>>>                          trivial code
>>>                  <region 1 begin>
>>>                          heavy code
>>>                  <region 1 end>
>>>                          trivial code
>>>          }
>>>
>>> but this appears to do
>>> ...
>>> which is somewhat puzzling.
>> I notice that a v4 was sent that adds more sub-regions without actually
>> responding to this request. (It is worth also pointing out that you
>> ignored Junio's request you use the cover letter to explain your reasoning
>> for changes between versions.)
> Thanks for noticing.  I wasn't requesting any change in particular
> (at least not yet), but was inquiring the reasoning behind what was
> done.  From that point of view, the lack of answers was worse than
> yet another patch that does not explain why it was done that other
> way this time around.

Sorry, I've been replying on the GitGitGadget pull request thread:

e.g. https://github.com/gitgitgadget/git/pull/451#issuecomment-555044068


But none of those replies have been making their way to this mailing 
list. I'll attempt to send these replies to the mailing list now.


>
>> There is a real downside to nesting regions like this. Specifically, we
>> frequently limit the depth that we report nested regions to avoid
>> overwhelming the logs.
>>
>> In general, these sub-regions should be avoided when possible and instead
>> create regions around important sections, such as the second option Junio
>> lists above.
> Thanks for a clear direction as the area expert of trace2.
>
>