Re: [PATCH v1 1/2] reset: don't compute unstaged changes after reset when --quiet

From: Ben Peart <peartben@gmail.com>
Date: Oct 18, 2018, 18:15 UTC
Message-ID: <5b4d46c2-ac0b-8a44-5e99-b0926ea764d3@gmail.com>
In-Reply-To: <20181018063628.GA23537@sigill.intra.peff.net>

On 10/18/2018 2:36 AM, Jeff King wrote:

Show 21 quoted lines

> On Thu, Oct 18, 2018 at 12:40:48PM +0900, Junio C Hamano wrote:
> 
>> Jeff King <peff@peff.net> writes:
>>
>>> Whereas for the new config variable, you'd probably set it not because
>>> you want it quiet all the time, but because you want to get some time
>>> savings. So there it does make sense to me to explain.
>>>
>>> Other than that, this seems like an obvious and easy win. It does feel a
>>> little hacky (you're really losing something in the output, and ideally
>>> we'd just be able to give that answer quickly), but this may be OK as a
>>> hack in the interim.
>>
>> After "git reset --quiet -- this/area/" with this change, any
>> operation you'd do next that needs to learn if working tree files
>> are different from what is recorded in the index outside that area
>> will have to spend more cycles, because the refresh done by "reset"
>> is now limited to the area.  So if your final goal is "make 'reset'
>> as fast as possible", this is an obvious and easy win.  For other
>> goals, i.e. "make the overall experience of using Git feel faster",
>> it is not so obvious to me, though.

The final goal is to make git faster (especially on larger repos) and this proposal accomplishes that. Let's look at why that is.

By scoping down (or eliminating) what refresh_index() has to lstat() at the end of the reset command, clearly the reset command is faster. Yes, the index isn't as "fresh" because not everything was updated but that doesn't typically impact the performance of subsequent commands.

On the next command, git still has to lstat() every file because it isn't sure what changes could have happened in the file system. As a result, the overall impact is that we have had to lstat() every file one fewer times between the two commands. A net win overall.

In addition, the preload_index() code that does the lstat() command is highly optimized across multiple threads (and on Windows takes advantage of the fscache). This means that it can lstat() every file _much_ faster than the single threaded loop in refresh_index(). This also makes the overall performance of the pair of git commands faster as we got rid of the slow lstat() loop and kept the fast one.

Here are some numbers to demonstrate that. These are hot cache numbers as they are easier to generate. Cold cache numbers make the net perf win significantly better as the cost for the reset jumps from 2.43 seconds to 7.16 seconds.

0.32 git add asdf 0.31 git -c reset.quiet=true reset asdf 1.34 git status 1.97 Total

0.32 git add asdf 2.43 git -c reset.quiet=false reset asdf 1.32 git status 4.07 Total

Note the status command after the reset doesn't really change as it still must lstat() every file (the 0.02 difference is well within the variability of run to run differences).

FWIW, none of these numbers are using fsmonitor.

There was additional discussion about whether this should be tied to the "--quiet" option and how it should be documented.

One option would be to change the default behavior of reset so that it doesn't do the refresh_index() call at all. This speeds up reset by default so there are no user discoverability issues but changes the default behavior which is an issue.

Another option that was suggested was to add a separate flag that could be passed to reset so that the "quiet" and "fast" options don't get conflated. I don't care for that option because the two options (at this point and for the foreseeable future) would be identical in behavior from the end users perspective.

It was also suggested that there be a single "fast and quiet" option for all of git instead of separate options for each command. I worry about that because now we're forcing users to choose between the "fast and quiet" version of git and the "slow and noisy" version. How do we help them decide which they want? That seems difficult to explain so that they can make a rational choice and also hard to discover. I also have to wonder who would say "give me the slow and noisy version please." :)

I'd prefer we systematically move towards a model where the default values that are chosen for various settings throughout the code are all configurable via settings. All defaults by necessity make certain assumptions about user preference, data shape, machine performance, etc and if those assumptions don't match the user's environment then the hard coded defaults aren't appropriate. We do our best but its going to be hit or miss.

A consistent way to be able to change those defaults would be very useful in those circumstances. To be clear, I'm not proposing we do a wholesale update of our preferences model at this point in time - that seems like a significant undertaking and I don't want to tie this specific optimization to a potential change in how default settings work.

To move this forward, here is what I propose:

1) If the '--quiet' flag is passed, we silently take advantage of the 
fact we can avoid having to do an "extra" lstat() of every file and 
scope the refresh_index() call to those paths that we know have changed.

2) I can remove the note in the documentation of --quiet which I only 
added to facilitate discoverability.

3) I can also edit the documentation for reset.quietDefault (maybe I 
should rename that to "reset.quiet"?) so that it does not discuss the 
potential performance impact.

4) To improve the discoverability of the enhanced performance, I could 
add logic similar to what exists for "status --uno" and if 
refresh_index() takes > x seconds, prompt the user with something like:

"It took %.2f seconds to enumerate unstaged changes after reset. 'reset --quiet' may speed it up. Set the config setting reset.quiet to true to make this the default."

Show 17 quoted lines

>>
>> If we somehow know that it is much less important in your setup that
>> the cached stat bits in the index is kept up to date (e.g. perhaps
>> you are more heavily relying on fsmonitor and are happy with it),
>> then I suspect that we could even skip the refreshing altogether and
>> gain more performance, without sacrificing the "overall experience
>> of using Git" at all, which would be even better.
> 
> Yeah, I assumed that Ben was using fsmonitor. I agree if we can just use
> that to make this output faster, that would be the ideal. This is the
> "later the message would get faster to produce" I hinted at in my
> earlier message.
> 
> So I think we are in agreement. It just isn't clear to me how much work
> it would take to get to the "ideal". If it's long enough, then this kind
> of hackery may be useful in the meantime.
>

I actually started my effort to speed up reset by attempting to multi-thread refresh_index(). You can see a work in progress at:

https://github.com/benpeart/git/pull/new/refresh-index-multithread-gvfs

The patch doesn't always work as it is still not thread safe. When it works, it's great but I ran into to many difficulties trying to debug the remaining threading issues (even adding print statements would change the timing and the repro would disappear). It will take a lot of code review to discover and fix the remaining non-thread safe code paths.

In addition, the optimized code path that takes advantage of fsmonitor, 
uses multiple threads, fscache, etc _already exists_ in preload_index(). 
  Trying to recreate all those optimizations in refresh_index() is (as I 
discovered) a daunting task.

This patch was tiny/trivial in comparison and provided all the performance benefits so seems like a much better option at this point in time. For now, I suggest we just use that existing path as it provides the benefits without the significant additional work and complexity.

> -Peff
>