Page MenuHomePhabricator

Accelerate working tree operations in git
ClosedPublic

Authored by alexmv on Dec 23 2017, 1:13 AM.

Details

Summary

Currently, arc on git uses the following commands to examine the
state of the working tree and history; example times for a no-op diff in a
165k-file working tree are also shown:

1)  git diff --no-ext-diff --no-textconv --submodule=short --raw 'fb062d4ecce5d9c1786b7bfc8a0dedf6b11fdd96' --
  = 1,722,514 us

2a) git diff --no-ext-diff --no-textconv --submodule=short --raw 'HEAD' --
  = 1,715,507 us

2b) git ls-files --others --exclude-standard
  = 2,359,202 us

3)  git diff-files --name-only
  = 1,333,274 us

Steps (2a) and (2b) are run concurrently; this results in a total elapsed
wallclock time of approximately 5.4 seconds. This is inefficient -- all four of
the above steps must both load the index and examine the working copy, which may
be slow operations when large repositories are used. Additionally, none of the
effort of those stat calls on the working tree, or load time of the index, is
shared across the processes.

Step (1) is called from getCommitRangeStatus, which was split out in D4095; it
is currently never called on its own, only ever from getWorkingCopyStatus,
where it it combined with getUncommittedStatus. The current behavior of the
method is to return the set of changes either in local commits or
uncommitted in the working tree, which duplicates work that
getUncommittedStatus is intended to do. Changing the behavior of this method
(in Git, and other VCSes) to only examine _committed_ status seems both inline
with the name of the method and the original description of it in D4095 -- and
also serves to make it much faster, as it is an operation that need not inspect
the working tree at all.

Steps (2a), (2b), and (3) attempt to gather the state of the working copy, and
as such are all I/O bound but must examine nearly identical data. For git
2.11.0 and higher, we can instead rely on the machine-parseable `git status
--porcelain=2` format, which provides the information from all of these commands
at once. It also allows additional performance improvements, as git status
has been the focus of several optimizations in the latest versions of git (the
untracked cache and fsmonitor services, for instance), which are not available
in the lower-level diff, ls-files, and diff-files commands.

This has the added benefit of fixing a bug noticed in T9455, in that uncommitted
or unstaged changes in modules can now be detected, regardless of if they also
have changed their base commit. It further resolves a bug where .gitmodules
appeared to have unstaged changes, when in reality the unstaged changes were in
submodules elsewhere in the tree.

For backwards compatibility with versions of git < 2.11.0, the old code is left
in place. It is possible that the simpler output from v1 of `git status
--porcelain` would also suffice for some of the above benefits, but the payoff
of parsing yet another format is deemed insufficient; users wishing improved
performance should simply upgrade git.

Alltogether, these result in the following, for a no-op diff in a
165k-working-file tree:

1) git diff --no-ext-diff --no-textconv --submodule=short --raw 'fb062d4ecce5d9c1786b7bfc8a0dedf6b11fdd96' HEAD --
 = 9,227 us
2) git status --porcelain=2 -z
 = 739,964 us

...for a total of 749ms, an improvement of 4.7s.

Depends on D18841.

Test Plan

Existing tests.

Diff Detail

Repository
rARC Arcanist
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

alexmv created this revision.Dec 23 2017, 1:13 AM
alexmv requested review of this revision.Dec 23 2017, 1:14 AM
epriestley accepted this revision.Dec 23 2017, 4:31 PM
epriestley added inline comments.
src/repository/api/ArcanistGitAPI.php
55–61

At some point, we should move this to use PhutilGitBinaryAnalyzer (which has test coverage and is also used by Phabricator now), but practically speaking it has identical behavior (and, currently, no caching) so I think this is entirely reasonable.

src/repository/api/ArcanistMercurialAPI.php
376

Does --rev -1 have identical behavior to the (more conventional?) --rev tip? If so, can we use --rev tip for readability/consistency?

src/repository/api/__tests__/ArcanistRepositoryAPIStateTestCase.php
59–67

I don't love this but alternatives seem like a lot of work for not a lot of benefit.

This revision is now accepted and ready to land.Dec 23 2017, 4:31 PM
alexmv added inline comments.Dec 24 2017, 4:06 AM
src/repository/api/ArcanistMercurialAPI.php
376

Ah, yes, per the docs. My mercurial-fu is not great -- I'll switch to tip.

src/repository/api/__tests__/ArcanistRepositoryAPIStateTestCase.php
59–67

Yeah. I wrote this chunk with regret. But it's better than having tests that fail, or confusing semi-duplicate tests, or requiring git 2.11.0 for arc.

alexmv updated this revision to Diff 45208.Dec 24 2017, 4:12 AM
  • Switch to --rev tip
alexmv closed this revision.Dec 24 2017, 4:12 AM
This revision was automatically updated to reflect the committed changes.