On 10/18/2018 2:36 AM, Jeff King wrote:
On Thu, Oct 18, 2018 at 12:40:48PM +0900, Junio C Hamano wrote:
Jeff King <peff@xxxxxxxx> 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."
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
