[PATCH v2 0/4] Optionally skip hashing index on write

["Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com>]

Writing the index is a critical action that takes place in multiple Git
commands. The recent performance improvements available with the sparse
index show how often the I/O costs around the index can affect different Git
commands, although reading the index takes place more often than a write.

The index is written through the hashfile API, both as a buffered write and
as a computation of its trailing hash. This trailing hash is an expectation
of the file format: several optional index extensions are provided using
indicators immediately preceding the trailing hash. 'git fsck' will complain
if the trailing hash does not match the contents of the file up to that
point.

However, computing the hash is expensive. This is even more expensive now
that we've moved to sha1dc instead of hardware-accelerated SHA1
computations.

This series provides a new config option, index.skipHash, that allows users
to disable computing the hash as Git writes the index. In this case, the
trailing hash is stored as the null hash (all bytes are zero).

The implementation is rather simple, but the patches organize different
aspects in a way that we could split things out:

 * Patch 1 adds a 'skip_hash' option to 'struct hashfile' that creates this
   as a general option to the hashwrite API.
 * The motivation in Patch 1 avoids the talk about the chunk-format API and
   instead focuses on the index as the first example that could make use of
   this.
 * Patch 2 creates the index.skipHash config option and updates 'git fsck'
   to ignore a null hash on the index. This is demonstrated by a very simple
   test.
 * Patch 3 creates a test helper to get the trailing hash of a file, and
   adds a concrete check on the trailing hash when index.skipHash=true.
 * Patch 4 (which could be deferred until later) adds index.skipHash=true as
   an implication of feature.manyFiles and as a setting in Scalar.

The end result is that index writes speed up significantly, enough that 'git
update-index --force-write' improves by 1.75x.

Similar patches appeared in my earlier refs RFC [1].

[1]
https://lore.kernel.org/git/pull.1408.git.1667846164.gitgitgadget@gmail.com/


Updates since v1
================

 * In Patch 1, use hashcler() instead of memset().
 * In Patches 2 and 4, be explicit about which Git versions will exhibit
   different behavior when reading an index with a null trailing hash.
 * In Patch 2, used a more compact way of loading from config.
 * In Patch 2 (and following) dropped the sub-shell in t1600-index.sh.
 * In Patch 3, dropped back-ticks when using a multi-line pipe.
 * In Patch 4, use "! cmp" instead of "! test_cmp"


Updates since RFC
=================

 * The config name is now index.skipHash to make it more clear that the
   default is to not skip the hash, and choosing this option requires a true
   value.
 * Use oidread() instead of an ad-hoc loop.
 * Patches 3 and 4 are new. 3 adds some extra test verification that wasn't
   in the RFC.
 * I think that patch 4 is helpful to see now, but I could understand if we
   wanted to defer that patch until after index.skipHash has had more time
   to bake.

Thanks, -Stolee

Derrick Stolee (4):
  hashfile: allow skipping the hash function
  read-cache: add index.skipHash config option
  test-lib-functions: add helper for trailing hash
  features: feature.manyFiles implies fast index writes

 Documentation/config/feature.txt |  5 +++++
 Documentation/config/index.txt   |  9 +++++++++
 csum-file.c                      | 14 +++++++++++---
 csum-file.h                      |  7 +++++++
 read-cache.c                     | 15 ++++++++++++++-
 repo-settings.c                  |  2 ++
 repository.h                     |  1 +
 scalar.c                         |  1 +
 t/t1600-index.sh                 | 20 ++++++++++++++++++++
 t/test-lib-functions.sh          |  8 ++++++++
 10 files changed, 78 insertions(+), 4 deletions(-)


base-commit: 805265fcf7a737664a8321aaf4a0587b78435184
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1439%2Fderrickstolee%2Fhashfile-skip-hash-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1439/derrickstolee/hashfile-skip-hash-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1439

Range-diff vs v1:

 1:  40ee8dbaef0 ! 1:  b582d681581 hashfile: allow skipping the hash function
     @@ csum-file.c: int finalize_hashfile(struct hashfile *f, unsigned char *result,
      -	the_hash_algo->final_fn(f->buffer, &f->ctx);
      +
      +	if (f->skip_hash)
     -+		memset(f->buffer, 0, the_hash_algo->rawsz);
     ++		hashclr(f->buffer);
      +	else
      +		the_hash_algo->final_fn(f->buffer, &f->ctx);
      +
 2:  5fb4b5a36ac ! 2:  c8a2fd3a470 read-cache: add index.skipHash config option
     @@ Commit message
          The one exception is 'git fsck' which checks the hash of the index file.
          This used to be a check on every index read, but was split out to just
          the index in a33fc72fe91 (read-cache: force_verify_index_checksum,
     -    2017-04-14).
     +    2017-04-14) and released first in Git 2.13.0. Document the versions that
     +    relaxed these restrictions, with the optimistic expectation that this
     +    change will be included in Git 2.40.0.
      
          Here, we disable this check if the trailing hash is all zeroes. We add a
          warning to the config option that this may cause undesirable behavior
          with older Git versions.
      
          As a quick comparison, I tested 'git update-index --force-write' with
     -    and without index.computeHash=false on a copy of the Linux kernel
     +    and without index.skipHash=true on a copy of the Linux kernel
          repository.
      
          Benchmark 1: with hash
     @@ Documentation/config/index.txt: index.version::
      +	Instead, write a trailing set of bytes with value zero, indicating
      +	that the computation was skipped.
      ++
     -+If you enable `index.skipHash`, then older Git clients may report that
     -+your index is corrupt during `git fsck`.
     ++If you enable `index.skipHash`, then Git clients older than 2.13.0 will
     ++refuse to parse the index and Git clients older than 2.40.0 will report an
     ++error during `git fsck`.
      
       ## read-cache.c ##
      @@ read-cache.c: static int verify_hdr(const struct cache_header *hdr, unsigned long size)
     @@ read-cache.c: static int verify_hdr(const struct cache_header *hdr, unsigned lon
       	return 0;
       }
      @@ read-cache.c: static int do_write_index(struct index_state *istate, struct tempfile *tempfile,
     - 	int ieot_entries = 1;
     - 	struct index_entry_offset_table *ieot = NULL;
     - 	int nr, nr_threads;
     -+	int skip_hash;
       
       	f = hashfd(tempfile->fd, tempfile->filename.buf);
       
     -+	if (!git_config_get_maybe_bool("index.skiphash", &skip_hash))
     -+		f->skip_hash = skip_hash;
     ++	git_config_get_maybe_bool("index.skiphash", (int *)&f->skip_hash);
      +
       	for (i = removed = extended = 0; i < entries; i++) {
       		if (cache[i]->ce_flags & CE_REMOVE)
     @@ t/t1600-index.sh: test_expect_success 'out of bounds index.version issues warnin
       '
       
      +test_expect_success 'index.skipHash config option' '
     -+	(
     -+		rm -f .git/index &&
     -+		git -c index.skipHash=true add a &&
     -+		git fsck
     -+	)
     ++	rm -f .git/index &&
     ++	git -c index.skipHash=true add a &&
     ++	git fsck
      +'
      +
       test_index_version () {
 3:  a20bf8de864 ! 3:  813e81a0582 test-lib-functions: add helper for trailing hash
     @@ Commit message
          Signed-off-by: Derrick Stolee <derrickstolee@github.com>
      
       ## t/t1600-index.sh ##
     -@@ t/t1600-index.sh: test_expect_success 'index.skipHash config option' '
     - 	(
     - 		rm -f .git/index &&
     - 		git -c index.skipHash=true add a &&
     -+		test_trailing_hash .git/index >hash &&
     -+		echo $(test_oid zero) >expect &&
     -+		test_cmp expect hash &&
     - 		git fsck
     - 	)
     +@@ t/t1600-index.sh: test_expect_success 'out of bounds index.version issues warning' '
     + test_expect_success 'index.skipHash config option' '
     + 	rm -f .git/index &&
     + 	git -c index.skipHash=true add a &&
     ++	test_trailing_hash .git/index >hash &&
     ++	echo $(test_oid zero) >expect &&
     ++	test_cmp expect hash &&
     + 	git fsck
       '
     + 
      
       ## t/test-lib-functions.sh ##
      @@ t/test-lib-functions.sh: test_cmp_config_output () {
     @@ t/test-lib-functions.sh: test_cmp_config_output () {
      +# Given a filename, extract its trailing hash as a hex string
      +test_trailing_hash () {
      +	local file="$1" &&
     -+	tail -c $(test_oid rawsz) "$file" | \
     -+		test-tool hexdump | \
     ++	tail -c $(test_oid rawsz) "$file" |
     ++		test-tool hexdump |
      +		sed "s/ //g"
      +}
 4:  77bf5d5ff27 ! 4:  e640dff53dd features: feature.manyFiles implies fast index writes
     @@ Documentation/config/feature.txt: feature.manyFiles::
       	`git checkout` may be slow and these new defaults improve performance:
       +
      +* `index.skipHash=true` speeds up index writes by not computing a trailing
     -+  checksum.
     ++  checksum. Note that this will cause Git versions earlier than 2.13.0 to
     ++  refuse to parse the index and Git versions earlier than 2.40.0 will report
     ++  a corrupted index during `git fsck`.
      ++
       * `index.version=4` enables path-prefix compression in the index.
       +
     @@ Documentation/config/feature.txt: feature.manyFiles::
      
       ## read-cache.c ##
      @@ read-cache.c: static int do_write_index(struct index_state *istate, struct tempfile *tempfile,
     - 	int ieot_entries = 1;
     - 	struct index_entry_offset_table *ieot = NULL;
     - 	int nr, nr_threads;
     --	int skip_hash;
       
       	f = hashfd(tempfile->fd, tempfile->filename.buf);
       
     --	if (!git_config_get_maybe_bool("index.skiphash", &skip_hash))
     --		f->skip_hash = skip_hash;
     +-	git_config_get_maybe_bool("index.skiphash", (int *)&f->skip_hash);
      +	if (istate->repo) {
      +		prepare_repo_settings(istate->repo);
      +		f->skip_hash = istate->repo->settings.index_skip_hash;
     @@ scalar.c: static int set_recommended_config(int reconfigure)
      
       ## t/t1600-index.sh ##
      @@ t/t1600-index.sh: test_expect_success 'index.skipHash config option' '
     - 		test_trailing_hash .git/index >hash &&
     - 		echo $(test_oid zero) >expect &&
     - 		test_cmp expect hash &&
     --		git fsck
     -+		git fsck &&
     + 	test_trailing_hash .git/index >hash &&
     + 	echo $(test_oid zero) >expect &&
     + 	test_cmp expect hash &&
     +-	git fsck
     ++	git fsck &&
      +
     -+		rm -f .git/index &&
     -+		git -c feature.manyFiles=true add a &&
     -+		test_trailing_hash .git/index >hash &&
     -+		test_cmp expect hash &&
     ++	rm -f .git/index &&
     ++	git -c feature.manyFiles=true add a &&
     ++	test_trailing_hash .git/index >hash &&
     ++	test_cmp expect hash &&
      +
     -+		rm -f .git/index &&
     -+		git -c feature.manyFiles=true \
     -+		    -c index.skipHash=false add a &&
     -+		test_trailing_hash .git/index >hash &&
     -+		! test_cmp expect hash
     - 	)
     ++	rm -f .git/index &&
     ++	git -c feature.manyFiles=true \
     ++		-c index.skipHash=false add a &&
     ++	test_trailing_hash .git/index >hash &&
     ++	! cmp expect hash
       '
       
     + test_index_version () {

-- 
gitgitgadget