speed of git reset -- file

speed of git reset -- file

[Joey Hess]

[-- Attachment #1: Type: text/plain, Size: 304 bytes --]

I'd expect that resetting a single file would need to update the index,
reading some objects from .git to do it.

But according to strace, it also stats every file in the working tree.
I have lots of files, and so that is very slow. Is it really necessary?

git version 1.7.5.1

-- 
see shy jo

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: speed of git reset -- file

[Jeff King]

On Tue, May 31, 2011 at 03:00:15PM -0400, Joey Hess wrote:

> I'd expect that resetting a single file would need to update the index,
> reading some objects from .git to do it.
> 
> But according to strace, it also stats every file in the working tree.
> I have lots of files, and so that is very slow. Is it really necessary?

Conceptually, no, I don't think so. But remember that your "file" is not
really a file at all, but a pathspec that may match many entries. Also,
we try not to overwrite things that would not be changed. So that
complicates it a little bit.

You can see the implementation in builtin/reset.c:read_from_tree. We
actually diff the tree (e.g., HEAD) against the index, and update only
the differences. Unfortunately this seems to trash the index (see the
comment there), and we end up having to refresh it. I'm not sure how
avoidable that trashing is. I think we're getting deep into how
unpack_trees works, and it handles a lot more cases than just "unpack a
few entries". So I don't know how easy it would be to separate this
relatively simple case from more complex ones.

-Peff

Re: speed of git reset -- file

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Conceptually, no, I don't think so.

Hmm, don't we have to say like all other reset types which paths are
dirty, i.e.

  $ git reset HEAD -- diff.c
  Unstaged changes after reset:
  M       diff.c
  M       diff.h

which would mean we would need to refresh the index anyway?

Re: speed of git reset -- file

[Jeff King]

On Tue, May 31, 2011 at 02:54:05PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > Conceptually, no, I don't think so.
> 
> Hmm, don't we have to say like all other reset types which paths are
> dirty, i.e.
> 
>   $ git reset HEAD -- diff.c
>   Unstaged changes after reset:
>   M       diff.c
>   M       diff.h
> 
> which would mean we would need to refresh the index anyway?

True, though if you pass "-q", we could bypass that step.

-Peff

Re: speed of git reset -- file

[Matthieu Moy]

Junio C Hamano <gitster@pobox.com> writes:

> Jeff King <peff@peff.net> writes:
>
>> Conceptually, no, I don't think so.
>
> Hmm, don't we have to say like all other reset types which paths are
> dirty, i.e.
>
>   $ git reset HEAD -- diff.c
>   Unstaged changes after reset:
>   M       diff.c
>   M       diff.h
>
> which would mean we would need to refresh the index anyway?

This reminder is handy, but I can understand that people with really big
trees whish to stat all the files only as needed (and would need to run
"git status" by hand when needed). We can imagine a config variable like
core.IHaveAReallyBigTree or so disabled by default for such cases.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: speed of git reset -- file

[Nguyen Thai Ngoc Duy]

On Wed, Jun 1, 2011 at 5:13 AM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> This reminder is handy, but I can understand that people with really big
> trees whish to stat all the files only as needed (and would need to run
> "git status" by hand when needed). We can imagine a config variable like
> core.IHaveAReallyBigTree or so disabled by default for such cases.

git update-index --assume-unchanged and core.ignorestat?
-- 
Duy

Re: speed of git reset -- file

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> ... Unfortunately this seems to trash the index (see the
> comment there), and we end up having to refresh it.

I think you are reading it wrong. Yes, diff-index will contaminate the
in-core index when it tries to find out which paths are to be reset, the
"discard" refers to the fact that it would be useless to add_cache_entry()
or remove_file_from_cache() to the in-core index without reading the in-core
index afresh.

So we restart from a freshly read index (see read_cache() at the beginning
of update_index_from_diff() callback) with all the stat information just
as fresh as you used to have before you ran this "git reset", and then we
update the paths that the internal diff-index found in the in-core index.

"git reset" has always refreshed the index. If somebody _really_ wants to
introduce a slight inconsistency to "git reset" so that only in "per-path"
mode it doesn't refresh, the call to refresh_index() can easily be removed
from update_index_refresh(). There is no "we end up having to".

IOW, we refresh by choice, design and inertia ;-).

Re: speed of git reset -- file

[Jeff King]

On Tue, May 31, 2011 at 04:39:25PM -0700, Junio C Hamano wrote:

> "git reset" has always refreshed the index. If somebody _really_ wants to
> introduce a slight inconsistency to "git reset" so that only in "per-path"
> mode it doesn't refresh, the call to refresh_index() can easily be removed
> from update_index_refresh(). There is no "we end up having to".
> 
> IOW, we refresh by choice, design and inertia ;-).

Ah, yeah, I really wasn't thinking of the fact that refreshing is a
user-facing operation. These days most of the porcelain commands will do
it for you automatically, so it's easy to forget.

So implementing the "optimization" to drop the refresh here doesn't seem
worth it. It inroduces an awful inconsistency, and it probably isn't
saving much in practice. Lots of other commands will end up stat'ing
everything, anyway. Users with giant repos or slow stat calls are
probably better off using assume-unchanged, which would help this and
many other situations.

-Peff

Re: speed of git reset -- file

[Joey Hess]

[-- Attachment #1: Type: text/plain, Size: 1141 bytes --]

Jeff King wrote:
> So implementing the "optimization" to drop the refresh here doesn't seem
> worth it. It inroduces an awful inconsistency, and it probably isn't
> saving much in practice. Lots of other commands will end up stat'ing
> everything, anyway. Users with giant repos or slow stat calls are
> probably better off using assume-unchanged, which would help this and
> many other situations.

Sounded like git-reset -q file could be optimised to not reread the
index without any visible inconsistency?

My experience with semi-large trees[1] is that I have to remember to use
"git status ." in a subdir; that "git commit -a" is of course slow when
I need to use it; and that the index gets big and things that need to
update it can become somewhat slow especially on slow disks, but that
otherwise git scales fairly well and has good locality, and that it's
easy to reason about what operations are global and avoid them.
So this git-reset behavior was surprising.

assume-unchanged seems like it would add a lot of work when merging.

-- 
see shy jo

[1] Two or three times the number of files as linux-2.6.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: speed of git reset -- file

[Jeff King]

On Wed, Jun 01, 2011 at 04:16:29PM -0400, Joey Hess wrote:

> Jeff King wrote:
> > So implementing the "optimization" to drop the refresh here doesn't seem
> > worth it. It inroduces an awful inconsistency, and it probably isn't
> > saving much in practice. Lots of other commands will end up stat'ing
> > everything, anyway. Users with giant repos or slow stat calls are
> > probably better off using assume-unchanged, which would help this and
> > many other situations.
> 
> Sounded like git-reset -q file could be optimised to not reread the
> index without any visible inconsistency?

No, there would still be a visible inconsistency. For example:

  $ git reset -q -- file
  $ git diff-files

might leave stat-dirty entries in the index, but:

  $ git reset -- file
  $ git diff-files

would not.

In practice, it doesn't matter that much, because you would probably run
the porcelain "git diff" instead of the plumbing "git diff-files"
anyway. And "git diff" refreshes the index itself, so the behavior
change is not that important. But then, as you see, we end up refreshing
the index defensively a lot, anyway, so saving the one refresh from
reset may be lost in the noise (unless you are doing some tight loop of
resets).

So what you could do with assume-unchanged (or much better, the
autorefreshindex variable that Junio recommends), would be to shut off
_all_ of the unnecessary refreshes, not just this one.

> My experience with semi-large trees[1] is that I have to remember to use
> "git status ." in a subdir; that "git commit -a" is of course slow when
> I need to use it; and that the index gets big and things that need to
> update it can become somewhat slow especially on slow disks, but that

Generally I find that the stats are very fast because everything is in
cache, and the disk doesn't come into it at all. Are you on an OS
besides Linux, or on a machine with low memory?

> otherwise git scales fairly well and has good locality, and that it's
> easy to reason about what operations are global and avoid them.
> So this git-reset behavior was surprising.

Yeah, git is generally very good about touching only the pieces of data
you need to complete the current operation. But I think the decision was
made long ago that "update-index --refresh" was not too expensive to do
it in most porcelain-ish commands, especially since the results are much
better (e.g., more accurately indicating modified files after a reset).

> assume-unchanged seems like it would add a lot of work when merging.

Yeah, Junio's autorefresh suggestion is much better.

-Peff

Re: speed of git reset -- file

[Joey Hess]

[-- Attachment #1: Type: text/plain, Size: 911 bytes --]

Jeff King wrote:
> > My experience with semi-large trees[1] is that I have to remember to use
> > "git status ." in a subdir; that "git commit -a" is of course slow when
> > I need to use it; and that the index gets big and things that need to
> > update it can become somewhat slow especially on slow disks, but that
> 
> Generally I find that the stats are very fast because everything is in
> cache, and the disk doesn't come into it at all. Are you on an OS
> besides Linux, or on a machine with low memory?

I have Linux and a gigabyte of ram and a not particularly good SSD.
Here `git reset file` takes 30 seconds, `git status` 45 seconds.

I do not seem to be alone in finding git reset file to be slow, the
problem was reported to me by another user.

> Yeah, Junio's autorefresh suggestion is much better.

That did seem reasonable and something I'd probably use.

-- 
see shy jo

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: speed of git reset -- file

[Jeff King]

On Wed, Jun 01, 2011 at 06:05:02PM -0400, Joey Hess wrote:

> Jeff King wrote:
> > > My experience with semi-large trees[1] is that I have to remember to use
> > > "git status ." in a subdir; that "git commit -a" is of course slow when
> > > I need to use it; and that the index gets big and things that need to
> > > update it can become somewhat slow especially on slow disks, but that
> > 
> > Generally I find that the stats are very fast because everything is in
> > cache, and the disk doesn't come into it at all. Are you on an OS
> > besides Linux, or on a machine with low memory?
> 
> I have Linux and a gigabyte of ram and a not particularly good SSD.
> Here `git reset file` takes 30 seconds, `git status` 45 seconds.

OK, that's horrific. For me, the stat information for linux-2.6 all sits
in cache and it takes about 0.3 seconds to refresh the index. I have 8G
of ram and a nice SSD, though it doesn't actually hit the disk at all.

Is it really faulting the stat information from disk that takes so long?
Have you tried running "perf" on "git update-index --refresh"?

-Peff

Re: speed of git reset -- file

[Joey Hess]

[-- Attachment #1: Type: text/plain, Size: 1528 bytes --]

Jeff King wrote:
> OK, that's horrific. For me, the stat information for linux-2.6 all sits
> in cache and it takes about 0.3 seconds to refresh the index. I have 8G
> of ram and a nice SSD, though it doesn't actually hit the disk at all.
> 
> Is it really faulting the stat information from disk that takes so long?
> Have you tried running "perf" on "git update-index --refresh"?

Cold cache:

 Performance counter stats for 'git update-index --refresh':

       9725.574501 task-clock-msecs         #      0.337 CPUs 
            58,163 context-switches         #      0.006 M/sec
                94 CPU-migrations           #      0.000 M/sec
             8,637 page-faults              #      0.001 M/sec
    12,825,956,531 cycles                   #   1318.787 M/sec  (scaled from 33.43%)
     2,696,089,912 instructions             #      0.210 IPC    (scaled from 50.03%)
       457,705,110 branches                 #     47.062 M/sec  (scaled from 51.30%)
        40,033,685 branch-misses            #      8.747 %      (scaled from 50.19%)
       149,071,234 cache-references         #     15.328 M/sec  (scaled from 32.83%)
         9,662,661 cache-misses             #      0.994 M/sec  (scaled from 32.27%)

       28.846625099  seconds time elapsed

Warm cache is 0.88 seconds, but cold cache is typical; I use a cheap netbook
for a lot of different stuff.

Also, I can get much worse cold cache numbers on eg, a fairly average
VPS node, where it takes > 1 minute.

-- 
see shy jo

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: speed of git reset -- file

[Jeff King]

On Wed, Jun 01, 2011 at 07:31:57PM -0400, Joey Hess wrote:

> Cold cache:
> [...]
>        28.846625099  seconds time elapsed
> 
> Warm cache is 0.88 seconds, but cold cache is typical; I use a cheap netbook
> for a lot of different stuff.

Yeah, it is going to be painful on a cold cache. But I wonder whether
your workflow would really permit the "reset" thing to make a
difference. That is, are you doing "git reset -- file" from a cold
cache, and then doing _nothing_ else with git? Because while yes, it may
be annoying for the "reset" to take 30 seconds, it's warming the cache
so that the subsequent "diff" or "status" will take 29.1 seconds less.

Which isn't to say I'm not sympathetic to the performance problems of
large repos on a cold cache. But I'm not sure there's really a way
around that. You're going to want to see the stat information eventually
if you are doing anything meaningful with git, and once it's loaded, the
warm cache delay isn't too bad. Trying to avoid it seems like a losing
battle.

-Peff

Re: speed of git reset -- file

[Joey Hess]

[-- Attachment #1: Type: text/plain, Size: 1149 bytes --]

Jeff King wrote:
> Yeah, it is going to be painful on a cold cache. But I wonder whether
> your workflow would really permit the "reset" thing to make a
> difference. That is, are you doing "git reset -- file" from a cold
> cache, and then doing _nothing_ else with git? Because while yes, it may
> be annoying for the "reset" to take 30 seconds, it's warming the cache
> so that the subsequent "diff" or "status" will take 29.1 seconds less.
> 
> Which isn't to say I'm not sympathetic to the performance problems of
> large repos on a cold cache. But I'm not sure there's really a way
> around that. You're going to want to see the stat information eventually
> if you are doing anything meaningful with git, and once it's loaded, the
> warm cache delay isn't too bad. Trying to avoid it seems like a losing
> battle.

Could be true in general. While I've gotten the reset out of this
workflow (realized I could just `git checkout HEAD file` and that would
also clear staged changes), in this case it was actually *unlikely* that
the cache would be unused, as I was resetting to throw unwanted changes
away.

-- 
see shy jo

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: speed of git reset -- file

[Joey Hess]

[-- Attachment #1: Type: text/plain, Size: 1267 bytes --]

Joey Hess wrote:
> Jeff King wrote:
> > Yeah, it is going to be painful on a cold cache. But I wonder whether
> > your workflow would really permit the "reset" thing to make a
> > difference. That is, are you doing "git reset -- file" from a cold
> > cache, and then doing _nothing_ else with git? Because while yes, it may
> > be annoying for the "reset" to take 30 seconds, it's warming the cache
> > so that the subsequent "diff" or "status" will take 29.1 seconds less.
> > 
> > Which isn't to say I'm not sympathetic to the performance problems of
> > large repos on a cold cache. But I'm not sure there's really a way
> > around that. You're going to want to see the stat information eventually
> > if you are doing anything meaningful with git, and once it's loaded, the
> > warm cache delay isn't too bad. Trying to avoid it seems like a losing
> > battle.
> 
> Could be true in general. While I've gotten the reset out of this
> workflow (realized I could just `git checkout HEAD file` and that would
> also clear staged changes), in this case it was actually *unlikely* that
> the cache would be unused, as I was resetting to throw unwanted changes
> away.

Typo, meant to say unlikly that the cache would be used.

-- 
see shy jo

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: speed of git reset -- file

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> So implementing the "optimization" to drop the refresh here doesn't seem
> worth it. It inroduces an awful inconsistency, and it probably isn't
> saving much in practice...

You could make it less inconsistent by introducing core.autorefreshindex
for people who _do_ care about the latency. We already have a precedent in
diff.autorefreshindex and the new configuration would supercede it.

Usually we refresh the cached lstat information in the index at strategic
places so that later operations do not have to pay the penalty, but at the
same time, the higher level commands that want to make sure they are not
fooled by stale lstat differences do refresh the index themselves anyway,
so leaving cached stat information stale is not exactly the end of the
world.

All the high-level commands like "reset" that do _not_ absolutely need to
refresh the index for them to work, but do refresh the index to help later
invocation of other commands, could be taught to pay attention to the
core.autorefreshindex. After many operations the users usually do not see
changes from diff-files, people who set this to false will see phantom
changes due to stale cached lstat information, and they will see them
consistently. They know exactly when having up-to-date lstat information
in the index really matters, and they will run "update-index --refresh"
themselves when needed.

I however personally doubt it would be worth the effort. The _only_ person
who complained about this doesn't even use "update-index --refresh" but
uses "git status", which does more than refreshing the index, and seems to
think the extra latency coming from the overhead is acceptable.

So...