Skip to content
This repository has been archived by the owner on Nov 9, 2017. It is now read-only.

Improve git-status performance #94

Closed
wants to merge 12 commits into from
Closed

Conversation

kblees
Copy link

@kblees kblees commented Oct 4, 2013

This is v4 of my fscache branch. The fscache code can now be turned on/off via config option (default off), so it is now pretty safe to merge.

See previous discussions at:

Changes since kb/fscache-v3 (#47):

  • the new hashmap implementation (1st patch in v3) has been sent upstream
  • fscache code (2nd patch in v3) has been split into several patches
  • fscache can be turned on/off at runtime via 'git config core.fscache'
  • if turned off, the original lstat and dirent implementations are used

Git for Windows installer with this PR applied:
https://docs.google.com/uc?id=0BxXUoUg2r8ftZ21mR2RyOG9NLWc&export=download

Here is git status performance on the WebKit repo (~200k files), best of 5 runs (i.e. hot cache):

     |  core.  |  --un   |                 |
 UAC | preload | tracked |   core.fscache  |
 [1] |  index  | -files  |  off   |   on   | gain
-----+---------+---------+--------+--------+------
 yes |   off   |   all   | 10.327 |  1.841 |  5.6
 yes |   off   |    no   |  8.939 |  1.404 |  6.4
 yes |    on   |   all   |  3.853 |  0.998 |  3.9
 yes |    on   |    no   |  2.449 |  0.671 |  3.6
  no |   off   |   all   |  4.102 |  1.778 |  2.3
  no |   off   |    no   |  2.699 |  1.404 |  1.9
  no |    on   |   all   |  2.636 |  0.999 |  2.6
  no |    on   |    no   |  1.232 |  0.671 |  1.8

[1] The UAC penalty hits on Vista/7 if the repository is located on the system drive and the luafv.sys driver is running.

The commit list looks a bit intimidating. However, only the last six patches are relevant for fscache (the msvc fixes have been merged to junio/next, hashmap is currently beeing discussed upstream, and the performance-tracing stuff is optional and the same as pull request #46).

Cheers,
Karsten

Set NO_GETTEXT in config.mak.uname to get rid of libintl.h dependency.

Signed-off-by: Karsten Blees <blees@dcon.de>
Skip errno.h definitions if they are already defined.

Signed-off-by: Karsten Blees <blees@dcon.de>
In msvc.h, there's a couple of stat related functions defined diffently
from mingw.h. When we remove these definitions, the only problem we get is
"warning C4005: '_stati64' : macro redefinition" for this line in mingw.h:

#define _stati64(x,y) mingw_stat(x,y)

The reason is that as of MSVCR80.dll (distributed with MSVC 2005), the
original _stati64 family of functions was renamed to _stat32i64, and the
former function names became macros (pointing to the appropriate function
based on the definition of _USE_32BIT_TIME_T).

Defining _stati64 works on MinGW because MinGW by default compiles against
the MSVCRT.DLL that is part of Windows (i.e. _stati64 is a function rather
than a macro).

Note: MinGW *can* compile for newer MSVC runtime versions, and MSVC
apparently can also compile for the Windows MSVCRT.DLL via the DDK (see
http://www.syndicateofideas.com/posts/fighting-the-msvcrt-dll-hell ).

Remove the stat definitions from msvc.h, as they are not compiler related.

In mingw.h, determine the runtime version in use from the definitions of
_stati64 and _USE_32BIT_TIME_T, and define stat() accordingly.

This also fixes that stat() in MSVC builds still resolves to mingw_lstat()
instead of mingw_stat().

Signed-off-by: Karsten Blees <blees@dcon.de>
The existing hashtable implementation (in hash.[ch]) uses open addressing
(i.e. resolve hash collisions by distributing entries across the table).
Thus, removal is difficult to implement with less than O(n) complexity.
Resolving collisions of entries with identical hashes (e.g. via chaining)
is left to the client code.

Add a hashtable implementation that supports O(1) removal and is slightly
easier to use due to builtin entry chaining.

Supports all basic operations init, free, get, add, remove and iteration.

Also includes ready-to-use hash functions based on the public domain FNV-1
algorithm (http://www.isthe.com/chongo/tech/comp/fnv).

The per-entry data structure (hashmap_entry) is piggybacked in front of
the client's data structure to save memory. See test-hashmap.c for usage
examples.

The hashtable is resized by a factor of four when 80% full. With these
settings, average memory consumption is about 2/3 of hash.[ch], and
insertion is about twice as fast due to less frequent resizing.

Lookups are also slightly faster, because entries are strictly confined to
their bucket (i.e. no data of other buckets needs to be traversed).

Signed-off-by: Karsten Blees <blees@dcon.de>
@buildhive
Copy link

MSysGit - the development behind Git for Windows » git #133 FAILURE
Looks like there's a problem with this pull request
(what's this?)

@buildhive
Copy link

MSysGit - the development behind Git for Windows » git #134 FAILURE
Looks like there's a problem with this pull request
(what's this?)

Signed-off-by: Karsten Blees <blees@dcon.de>
Move opendir down in preparation for the next patch.

Signed-off-by: Karsten Blees <blees@dcon.de>
Emulating the POSIX dirent API on Windows via FindFirstFile/FindNextFile is
pretty staightforward, however, most of the information provided in the
WIN32_FIND_DATA structure is thrown away in the process. A more
sophisticated implementation may cache this data, e.g. for later reuse in
calls to lstat.

Make the dirent implementation pluggable so that it can be switched at
runtime, e.g. based on a config option.

Define a base DIR structure with pointers to readdir/closedir that match
the opendir implementation (i.e. similar to vtable pointers in OOP).
Define readdir/closedir so that they call the function pointers in the DIR
structure. This allows to choose the opendir implementation on a
call-by-call basis.

Move the fixed sized dirent.d_name buffer to the dirent-specific DIR
structure, as d_name may be implementation specific (e.g. a caching
implementation may just set d_name to point into the cache instead of
copying the entire file name string).

Signed-off-by: Karsten Blees <blees@dcon.de>
Emulating the POSIX lstat API on Windows via GetFileAttributes[Ex] is quite
slow. Windows operating system APIs seem to be much better at scanning the
status of entire directories than checking single files. A caching
implementation may improve performance by bulk-reading entire directories
or reusing data obtained via opendir / readdir.

Make the lstat implementation pluggable so that it can be switched at
runtime, e.g. based on a config option.

Signed-off-by: Karsten Blees <blees@dcon.de>
Add a macro to mark code sections that only read from the file system,
along with a config option and documentation.

This facilitates implementation of relatively simple file system level
caches without the need to synchronize with the file system.

Enable read-only sections for 'git status' and preload_index.

Signed-off-by: Karsten Blees <blees@dcon.de>
Checking the work tree status is quite slow on Windows, due to slow lstat
emulation (git calls lstat once for each file in the index). Windows
operating system APIs seem to be much better at scanning the status
of entire directories than checking single files.

Add an lstat implementation that uses a cache for lstat data. Cache misses
read the entire parent directory and add it to the cache. Subsequent lstat
calls for the same directory are served directly from the cache.

Also implement opendir / readdir / closedir so that they create and use
directory listings in the cache.

The cache doesn't track file system changes and doesn't plug into any
modifying file APIs, so it has to be explicitly enabled for git functions
that don't modify the working copy.

Note: in an earlier version of this patch, the cache was always active and
tracked file system changes via ReadDirectoryChangesW. However, this was
much more complex and had negative impact on the performance of modifying
git commands such as 'git checkout'.

Signed-off-by: Karsten Blees <blees@dcon.de>
@buildhive
Copy link

MSysGit - the development behind Git for Windows » git #135 SUCCESS
This pull request looks good
(what's this?)

@childnode
Copy link

hi, I assume that this issue will cover the old but well known issue http://code.google.com/p/msysgit/issues/detail?id=320 !?

@kblees
Copy link
Author

kblees commented Nov 5, 2013

@childnode This patch series solves the UAC issue completely for git status (as can be seen from the benchmark results above). With core.preloadindex enabled, it also speeds up a lot of other commands.

However, there are still a lot of git commands that cannot enable fscache. GetFileAttributes calls by these commands will still suffer from UAC. The only way around this is to store the git repositories on a non-system drive or to disable the luafv driver (adding manifests to git.exe does not help).

@dscho
Copy link
Member

dscho commented Dec 17, 2013

@kblees @sschuberth @patthoyts @kusma how comfortable would y'all be with merging this before releasing Git for Windows 1.8.5.1?

@kblees
Copy link
Author

kblees commented Dec 18, 2013

@dscho I'd be very comfortable, for the following reasons:

  • the MSVC patches are in Git v1.8.5
  • hashmap-v5 is in junio/next
  • fscache is disabled by default...
  • ...and the old lstat and opendir implementations are practically unchanged (except for making them function pointers)
  • the core fscache.[ch] code has been stable for quite some time...
  • ...and has been tested by many who've downloaded the custom installers (unfortunately I don't have statistics)

I've rebased on top of Pat's tentative-1.8.5 branch to update MSVC and hashmap to the upstream versions [1]. Msysgit/master is still on v1.8.4, so I won't force-push this PR (I believe this will pull in all commits since v1.8.4 plus Pat's rebase).

[1] https://github.com/kblees/git/commits/kb/fscache-v4-tentative-1.8.5

@dscho
Copy link
Member

dscho commented Dec 18, 2013

@kblees my suggestion was to merge this branch into pt/tentative-1.8.5 (actually, thicket-1.8.5.1 which is basically a branch-ified pt/tentative-1.8.5 rebased onto v1.8.5.1) and push this into `master'...

@kblees
Copy link
Author

kblees commented Dec 19, 2013

@dscho the problem is that this branch is based on patches that have already been accepted upstream in a newer version. E.g. this branch conflicts with Sebastian's MinGW 4 patches in v1.8.5, and when moving towards v1.9, this branch's hashmap-v3 will most likely conflict with upstream's hashmap-v5.

So IMO the sane thing to do is rebase this branch (six patches) on top of the current upstream versions, which I've already done (see PR #107). I cannot update this PR because it is based on master, which is ~1000 commits behind.

@dscho
Copy link
Member

dscho commented Dec 19, 2013

@kblees actually, that is what I meant ;-) Thanks! If you're okay with closing this pull request (because #107 supersedes it) please make it so!

@kblees kblees closed this Dec 19, 2013
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants