| Message ID | 5d1c9c8a356b9003be21059c7ed6232732fd26c0.1609356414.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Cleanups around index operations | expand |
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?) > istate->cache_changed |= CACHE_TREE_CHANGED; > } > > -- > gitgitgadget >
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
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); prime_cache_tree_rec(r, istate->cache_tree, tree); + trace2_region_leave("cache-tree", "prime_cache_tree", the_repository); istate->cache_changed |= CACHE_TREE_CHANGED; }