speed of git reset -- file

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

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

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

Jeff King <peff@peff.net> writes:

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?

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

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

-Peff

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

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.

On Wed, Jun 1, 2011 at 5:13 AM, Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> wrote:

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

Jeff King <peff@peff.net> writes:

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 ;-).

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

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

Jeff King wrote:

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.

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

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.

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?

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).

Yeah, Junio's autorefresh suggestion is much better.

-Peff

Jeff King wrote:

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.

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

On Wed, Jun 01, 2011 at 06:05:02PM -0400, Joey Hess 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"?

-Peff

Jeff King wrote:

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.

On Wed, Jun 01, 2011 at 07:31:57PM -0400, Joey Hess 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.

-Peff

Jeff King wrote:

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.

Joey Hess wrote:

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

Jeff King <peff@peff.net> writes:

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...