Re: [PATCH 5/8] cache-tree: trace regions for prime_cache_tree

[Derrick Stolee <stolee@gmail.com>]

On 12/30/2020 2:48 PM, Elijah Newren wrote:
> On Wed, Dec 30, 2020 at 11:26 AM Derrick Stolee via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>>
>> From: Derrick Stolee <dstolee@microsoft.com>
>>
>> Commands such as "git reset --hard" rebuild the in-memory representation
>> of the cached tree index extension by parsing tree objects starting at a
>> known root tree. The performance of this operation can vary widely
>> depending on the width and depth of the repository's working directory
>> structure. Measure the time in this operation using trace2 regions in
>> prime_cache_tree().
>>
>> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
>> ---
>>  cache-tree.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/cache-tree.c b/cache-tree.c
>> index 45fb57b17f3..f135bb77af5 100644
>> --- a/cache-tree.c
>> +++ b/cache-tree.c
>> @@ -746,7 +746,10 @@ void prime_cache_tree(struct repository *r,
>>  {
>>         cache_tree_free(&istate->cache_tree);
>>         istate->cache_tree = cache_tree();
>> +
>> +       trace2_region_enter("cache-tree", "prime_cache_tree", the_repository);
> 
> Shouldn't this be at the start of the function, a few lines up?
> 
>>         prime_cache_tree_rec(r, istate->cache_tree, tree);
>> +       trace2_region_leave("cache-tree", "prime_cache_tree", the_repository);
> 
> ...and this be one more line down?  (or the string "prime_cache_tree"
> have a "_rec" added to it?)

I guess my focus was on creating changes around the "bulk"
of the operation, keeping the region enter/leave pair close
together. But perhaps enclosing the full block will be
better for full coverage in case this method became more
complicated (but not within prime_cache_tree_rec()).

Thanks,
-Stolee