Re: [PATCH] index: add trace2 region for clear skip worktree

[Anh Le <anh@canva.com>]

Thank you Junio and Jeff for the feedback! I hadn't considered that
`ensure_full_index()` would mean the loop can only at most restart
once. I'll action the feedbacks:
- count both loop iterations separately with int instead of intmax_t
- remove the restart counter
- don't log metrics if it's 0

Regards,
Anh


Regards,
Anh


On Thu, Oct 27, 2022 at 5:29 AM Jeff Hostetler <git@jeffhostetler.com> wrote:
>
>
>
> On 10/26/22 12:01 PM, Junio C Hamano wrote:
> > Jeff Hostetler <git@jeffhostetler.com> writes:
> >
> >> In the worst case, we walk the entire index and lstat() for a
> >> significant number of skipped-and-not-present files, then near
> >> the end of the loop, we find a skipped-but-present directory
> >> and have to restart the loop.  The second pass will still run
> >> the full loop again.  Will the second pass actually see any
> >> skipped cache-entries?  Will it re-lstat() them?  Could the
> >> `goto restart` just be a `break` or `return`?
> >>
> >> I haven't had time to look under the hood here, but I was
> >> hoping that these two counters would help the series author
> >> collect telemetry over many runs and gain more insight into
> >> the perf problem.
> >
> > Without being able to answer these questions, would we be able to
> > interpret the numbers reported from these counters?
> >
> >> Continuing the example from above, if we've already paid the
> >> costs to lstat() the 95% sparse files AND THEN near the bottom
> >> of the loop we have to do a restart, then we should expect
> >> this loop to be doubly slow.  It was my hope that this combination
> >> of counters would help us understand the variations in perf.
> >
> > Yes, I understand that double-counting may give some clue to detect
> > that, but it just looked roundabout way to do that.  Perhaps
> > counting the path inspected during the first iteration and the path
> > inspected during the second iteration, separately, without the "how
> > many times did we repeat?", would give you a better picture?  "After
> > inspecting 2400 paths, we need to go back and then ended up scanning
> > 3000 paths in the flattened index in the second round" would be
> > easier to interpret than "We needed flattening, and scanned 5400
> > paths in total in the two iterations".
>
> Good point and I was wondering about whether we might see "2 x 95%"
> values for path_count in really slow cases.  And yes, it would be
> better to have `int path_count[2]` and tally each loop pass
> independently.
>
> I guess I was looking for a first-order "where is the pain?" knowing
> that we could always dig deeper later.  That is, is the lstat's or
> is it the ensure-full and index rewrite?  Or both?
>
> We have other places that do lstat() over the cache-entries and have
> added code to spread the loop across n threads.  I doubt that is needed
> here and I didn't want to lead with it.
>
>
> >
> >> WRT the `intmax_t` vs just `int`: either is fine.
> >
> > I thought "int" was supposed to be natural machine word, while
> > incrementing "intmax_t" is allowed to be much slower than "int".
> > Do we expect more than 2 billion paths?
> >
>
> You're right.  An `int` would be fine here.
>
> Thanks,
> Jeff

-- 
**
** <https://www.canva.com/>Empowering the world to design
We're hiring, 
apply here <https://www.canva.com/careers/>! Check out the latest news and 
learnings from our team on the Canva Newsroom 
<https://www.canva.com/newsroom/news/>.
 <https://twitter.com/canva> 
<https://facebook.com/canva> <https://au.linkedin.com/company/canva> 
<https://twitter.com/canva>  <https://facebook.com/canva>  
<https://www.linkedin.com/company/canva> 
 <https://instagram.com/canva>