@matheusbsbs / matheus-bagliano / commits / 410334ed52

read-cache: use hashfile instead of git_hash_ctx

The do_write_index() method in read-cache.c has its own hashing logic and buffering mechanism. Specifically, the ce_write() method was introduced by 4990aadc (Speed up index file writing by chunking it nicely, 2005-04-20) and similar mechanisms were introduced a few months later in c38138cd (git-pack-objects: write the pack files with a SHA1 csum, 2005-06-26). Based on the timing, in the early days of the Git codebase, I figured that these roughly equivalent code paths were never unified only because it got lost in the shuffle. The hashfile API has since been used extensively in other file formats, such as pack-indexes, multi-pack-indexes, and commit-graphs. Therefore, it seems prudent to unify the index writing code to use the same mechanism. I discovered this disparity while trying to create a new index format that uses the chunk-format API. That API uses a hashfile as its base, so it is incompatible with the custom code in read-cache.c. This rewrite is rather straightforward. It replaces all writes to the temporary file with writes to the hashfile struct. This takes care of many of the direct interactions with the_hash_algo. There are still some git_hash_ctx uses remaining: the extension headers are hashed for use in the End of Index Entries (EOIE) extension. This use of the git_hash_ctx is left as-is. There are multiple reasons to not use a hashfile here, including the fact that the data is not actually writing to a file, just a hash computation. These hashes do not block our adoption of the chunk-format API in a future change to the index, so leave it as-is. The internals of the algorithms are mostly identical. Previously, the hashfile API used a smaller 8KB buffer instead of the 128KB buffer from read-cache.c. The previous change already unified these sizes. There is one subtle point: we do not pass the CSUM_FSYNC to the finalize_hashfile() method, which differs from most consumers of the hashfile API. The extra fsync() call indicated by this flag causes a significant peformance degradation that is noticeable for quick commands that write the index, such as "git add". Other consumers can absorb this cost with their more complicated data structure organization, and further writing structures such as pack-files and commit-graphs is rarely in the critical path for common user interactions. Some static methods become orphaned in this diff, so I marked them as MAYBE_UNUSED. The diff is much harder to read if they are deleted during this change. Instead, they will be deleted in the following change. In addition to the test suite passing, I computed indexes using the previous binaries and the binaries compiled after this change, and found the index data to be exactly equal. Finally, I did extensive performance testing of "git update-index --force-write" on repos of various sizes, including one with over 2 million paths at HEAD. These tests demonstrated less than 1% difference in behavior. As expected, the performance should be considered unchanged. The previous changes to increase the hashfile buffer size from 8K to 128K ensured this change would not create a peformance regression. Signed-off-by: Derrick Stolee <dstolee@microsoft.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Derrick Stolee committed May 18, 2021 at 18:32 UTC 410334ed5212979a3ff6c8ca119ca14290ee7790

1 file changed +66 -71

read-cache.c

+66 -71

@@ -26,6 +26,7 @@

26 #include "thread-utils.h"

27 #include "progress.h"

28 #include "sparse-index.h"

29 +#include "csum-file.h"

31 /* Mask for the name length in ce_flags in the on-disk index */

@@ -2525,6 +2526,7 @@ int repo_index_has_changes(struct repository *repo,

2526 static unsigned char write_buffer[WRITE_BUFFER_SIZE];

2527 static unsigned long write_buffer_len;

2528

2529 +MAYBE_UNUSED

2530 static int ce_write_flush(git_hash_ctx *context, int fd)

2531 {

2532 unsigned int buffered = write_buffer_len;

@@ -2537,6 +2539,7 @@ static int ce_write_flush(git_hash_ctx *context, int fd)

2539 return 0;

2540 }

2541

2542 +MAYBE_UNUSED

2543 static int ce_write(git_hash_ctx *context, int fd, void *data, unsigned int len)

2544 {

2545 while (len) {

@@ -2559,19 +2562,24 @@ static int ce_write(git_hash_ctx *context, int fd, void *data, unsigned int len)

2562 return 0;

2563 }

2564

2562 -static int write_index_ext_header(git_hash_ctx *context, git_hash_ctx *eoie_context,

2563 - int fd, unsigned int ext, unsigned int sz)

2565 +static int write_index_ext_header(struct hashfile *f,

2566 + git_hash_ctx *eoie_f,

2567 + unsigned int ext,

2568 + unsigned int sz)

2569 {

2565 - ext = htonl(ext);

2566 - sz = htonl(sz);

2567 - if (eoie_context) {

2568 - the_hash_algo->update_fn(eoie_context, &ext, 4);

2569 - the_hash_algo->update_fn(eoie_context, &sz, 4);

2570 + hashwrite_be32(f, ext);

2571 + hashwrite_be32(f, sz);

2572 +

2573 + if (eoie_f) {

2574 + ext = htonl(ext);

2575 + sz = htonl(sz);

2576 + the_hash_algo->update_fn(eoie_f, &ext, sizeof(ext));

2577 + the_hash_algo->update_fn(eoie_f, &sz, sizeof(sz));

2578 }

2571 - return ((ce_write(context, fd, &ext, 4) < 0) ||

2572 - (ce_write(context, fd, &sz, 4) < 0)) ? -1 : 0;

2579 + return 0;

2580 }

2581

2582 +MAYBE_UNUSED

2583 static int ce_flush(git_hash_ctx *context, int fd, unsigned char *hash)

2584 {

2585 unsigned int left = write_buffer_len;

@@ -2673,11 +2681,10 @@ static void copy_cache_entry_to_ondisk(struct ondisk_cache_entry *ondisk,

2681 }

2682 }

2683

2676 -static int ce_write_entry(git_hash_ctx *c, int fd, struct cache_entry *ce,

2684 +static int ce_write_entry(struct hashfile *f, struct cache_entry *ce,

2685 struct strbuf *previous_name, struct ondisk_cache_entry *ondisk)

2686 {

2687 int size;

2680 - int result;

2688 unsigned int saved_namelen;

2689 int stripped_name = 0;

2690 static unsigned char padding[8] = { 0x00 };

@@ -2693,11 +2700,9 @@ static int ce_write_entry(git_hash_ctx *c, int fd, struct cache_entry *ce,

2700 if (!previous_name) {

2701 int len = ce_namelen(ce);

2702 copy_cache_entry_to_ondisk(ondisk, ce);

2696 - result = ce_write(c, fd, ondisk, size);

2697 - if (!result)

2698 - result = ce_write(c, fd, ce->name, len);

2699 - if (!result)

2700 - result = ce_write(c, fd, padding, align_padding_size(size, len));

2703 + hashwrite(f, ondisk, size);

2704 + hashwrite(f, ce->name, len);

2705 + hashwrite(f, padding, align_padding_size(size, len));

2706 } else {

2707 int common, to_remove, prefix_size;

2708 unsigned char to_remove_vi[16];

@@ -2711,13 +2716,10 @@ static int ce_write_entry(git_hash_ctx *c, int fd, struct cache_entry *ce,

2716 prefix_size = encode_varint(to_remove, to_remove_vi);

2717

2718 copy_cache_entry_to_ondisk(ondisk, ce);

2714 - result = ce_write(c, fd, ondisk, size);

2715 - if (!result)

2716 - result = ce_write(c, fd, to_remove_vi, prefix_size);

2717 - if (!result)

2718 - result = ce_write(c, fd, ce->name + common, ce_namelen(ce) - common);

2719 - if (!result)

2720 - result = ce_write(c, fd, padding, 1);

2719 + hashwrite(f, ondisk, size);

2720 + hashwrite(f, to_remove_vi, prefix_size);

2721 + hashwrite(f, ce->name + common, ce_namelen(ce) - common);

2722 + hashwrite(f, padding, 1);

2723

2724 strbuf_splice(previous_name, common, to_remove,

2725 ce->name + common, ce_namelen(ce) - common);

@@ -2727,7 +2729,7 @@ static int ce_write_entry(git_hash_ctx *c, int fd, struct cache_entry *ce,

2729 ce->ce_flags &= ~CE_STRIP_NAME;

2730 }

2731

2730 - return result;

2732 + return 0;

2733 }

2734

2735 /*

@@ -2839,8 +2841,8 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,

2841 int strip_extensions)

2842 {

2843 uint64_t start = getnanotime();

2842 - int newfd = tempfile->fd;

2843 - git_hash_ctx c, eoie_c;

2844 + struct hashfile *f;

2845 + git_hash_ctx *eoie_c = NULL;

2846 struct cache_header hdr;

2847 int i, err = 0, removed, extended, hdr_version;

2848 struct cache_entry **cache = istate->cache;

@@ -2854,6 +2856,8 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,

2856 struct index_entry_offset_table *ieot = NULL;

2857 int nr, nr_threads;

2858

2859 + f = hashfd(tempfile->fd, tempfile->filename.buf);

2860 +

2861 for (i = removed = extended = 0; i < entries; i++) {

2862 if (cache[i]->ce_flags & CE_REMOVE)

2863 removed++;

@@ -2882,9 +2886,7 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,

2886 hdr.hdr_version = htonl(hdr_version);

2887 hdr.hdr_entries = htonl(entries - removed);

2888

2885 - the_hash_algo->init_fn(&c);

2886 - if (ce_write(&c, newfd, &hdr, sizeof(hdr)) < 0)

2887 - return -1;

2889 + hashwrite(f, &hdr, sizeof(hdr));

2890

2891 if (!HAVE_THREADS || git_config_get_index_threads(&nr_threads))

2892 nr_threads = 1;

@@ -2919,12 +2921,8 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,

2921 }

2922 }

2923

2922 - offset = lseek(newfd, 0, SEEK_CUR);

2923 - if (offset < 0) {

2924 - free(ieot);

2925 - return -1;

2926 - }

2927 - offset += write_buffer_len;

2924 + offset = hashfile_total(f);

2925 +

2926 nr = 0;

2927 previous_name = (hdr_version == 4) ? &previous_name_buf : NULL;

2928

@@ -2959,14 +2957,10 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,

2957 if (previous_name)

2958 previous_name->buf[0] = 0;

2959 nr = 0;

2962 - offset = lseek(newfd, 0, SEEK_CUR);

2963 - if (offset < 0) {

2964 - free(ieot);

2965 - return -1;

2966 - }

2967 - offset += write_buffer_len;

2960 +

2961 + offset = hashfile_total(f);

2962 }

2969 - if (ce_write_entry(&c, newfd, ce, previous_name, (struct ondisk_cache_entry *)&ondisk) < 0)

2963 + if (ce_write_entry(f, ce, previous_name, (struct ondisk_cache_entry *)&ondisk) < 0)

2964 err = -1;

2965

2966 if (err)

@@ -2985,14 +2979,16 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,

2979 return err;

2980 }

2981

2988 - /* Write extension data here */

2989 - offset = lseek(newfd, 0, SEEK_CUR);

2990 - if (offset < 0) {

2991 - free(ieot);

2992 - return -1;

2982 + offset = hashfile_total(f);

2983 +

2984 + /*

2985 + * The extension headers must be hashed on their own for the

2986 + * EOIE extension. Create a hashfile here to compute that hash.

2987 + */

2988 + if (offset && record_eoie()) {

2989 + CALLOC_ARRAY(eoie_c, 1);

2990 + the_hash_algo->init_fn(eoie_c);

2991 }

2994 - offset += write_buffer_len;

2995 - the_hash_algo->init_fn(&eoie_c);

2992

2993 /*

2994 * Lets write out CACHE_EXT_INDEXENTRYOFFSETTABLE first so that we

@@ -3005,8 +3001,8 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,

3001 struct strbuf sb = STRBUF_INIT;

3002

3003 write_ieot_extension(&sb, ieot);

3008 - err = write_index_ext_header(&c, &eoie_c, newfd, CACHE_EXT_INDEXENTRYOFFSETTABLE, sb.len) < 0

3009 - || ce_write(&c, newfd, sb.buf, sb.len) < 0;

3004 + err = write_index_ext_header(f, eoie_c, CACHE_EXT_INDEXENTRYOFFSETTABLE, sb.len) < 0;

3005 + hashwrite(f, sb.buf, sb.len);

3006 strbuf_release(&sb);

3007 free(ieot);

3008 if (err)

@@ -3018,9 +3014,9 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,

3014 struct strbuf sb = STRBUF_INIT;

3015

3016 err = write_link_extension(&sb, istate) < 0 ||

3021 - write_index_ext_header(&c, &eoie_c, newfd, CACHE_EXT_LINK,

3022 - sb.len) < 0 ||

3023 - ce_write(&c, newfd, sb.buf, sb.len) < 0;

3017 + write_index_ext_header(f, eoie_c, CACHE_EXT_LINK,

3018 + sb.len) < 0;

3019 + hashwrite(f, sb.buf, sb.len);

3020 strbuf_release(&sb);

3021 if (err)

3022 return -1;

@@ -3029,8 +3025,8 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,

3025 struct strbuf sb = STRBUF_INIT;

3026

3027 cache_tree_write(&sb, istate->cache_tree);

3032 - err = write_index_ext_header(&c, &eoie_c, newfd, CACHE_EXT_TREE, sb.len) < 0

3033 - || ce_write(&c, newfd, sb.buf, sb.len) < 0;

3028 + err = write_index_ext_header(f, eoie_c, CACHE_EXT_TREE, sb.len) < 0;

3029 + hashwrite(f, sb.buf, sb.len);

3030 strbuf_release(&sb);

3031 if (err)

3032 return -1;

@@ -3039,9 +3035,9 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,

3035 struct strbuf sb = STRBUF_INIT;

3036

3037 resolve_undo_write(&sb, istate->resolve_undo);

3042 - err = write_index_ext_header(&c, &eoie_c, newfd, CACHE_EXT_RESOLVE_UNDO,

3043 - sb.len) < 0

3044 - || ce_write(&c, newfd, sb.buf, sb.len) < 0;

3038 + err = write_index_ext_header(f, eoie_c, CACHE_EXT_RESOLVE_UNDO,

3039 + sb.len) < 0;

3040 + hashwrite(f, sb.buf, sb.len);

3041 strbuf_release(&sb);

3042 if (err)

3043 return -1;

@@ -3050,9 +3046,9 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,

3046 struct strbuf sb = STRBUF_INIT;

3047

3048 write_untracked_extension(&sb, istate->untracked);

3053 - err = write_index_ext_header(&c, &eoie_c, newfd, CACHE_EXT_UNTRACKED,

3054 - sb.len) < 0 ||

3055 - ce_write(&c, newfd, sb.buf, sb.len) < 0;

3049 + err = write_index_ext_header(f, eoie_c, CACHE_EXT_UNTRACKED,

3050 + sb.len) < 0;

3051 + hashwrite(f, sb.buf, sb.len);

3052 strbuf_release(&sb);

3053 if (err)

3054 return -1;

@@ -3061,14 +3057,14 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,

3057 struct strbuf sb = STRBUF_INIT;

3058

3059 write_fsmonitor_extension(&sb, istate);

3064 - err = write_index_ext_header(&c, &eoie_c, newfd, CACHE_EXT_FSMONITOR, sb.len) < 0

3065 - || ce_write(&c, newfd, sb.buf, sb.len) < 0;

3060 + err = write_index_ext_header(f, eoie_c, CACHE_EXT_FSMONITOR, sb.len) < 0;

3061 + hashwrite(f, sb.buf, sb.len);

3062 strbuf_release(&sb);

3063 if (err)

3064 return -1;

3065 }

3066 if (istate->sparse_index) {

3071 - if (write_index_ext_header(&c, &eoie_c, newfd, CACHE_EXT_SPARSE_DIRECTORIES, 0) < 0)

3067 + if (write_index_ext_header(f, eoie_c, CACHE_EXT_SPARSE_DIRECTORIES, 0) < 0)

3068 return -1;

3069 }

3070

@@ -3078,19 +3074,18 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,

3074 * read. Write it out regardless of the strip_extensions parameter as we need it

3075 * when loading the shared index.

3076 */

3081 - if (offset && record_eoie()) {

3077 + if (eoie_c) {

3078 struct strbuf sb = STRBUF_INIT;

3079

3084 - write_eoie_extension(&sb, &eoie_c, offset);

3085 - err = write_index_ext_header(&c, NULL, newfd, CACHE_EXT_ENDOFINDEXENTRIES, sb.len) < 0

3086 - || ce_write(&c, newfd, sb.buf, sb.len) < 0;

3080 + write_eoie_extension(&sb, eoie_c, offset);

3081 + err = write_index_ext_header(f, NULL, CACHE_EXT_ENDOFINDEXENTRIES, sb.len) < 0;

3082 + hashwrite(f, sb.buf, sb.len);

3083 strbuf_release(&sb);

3084 if (err)

3085 return -1;

3086 }

3087

3092 - if (ce_flush(&c, newfd, istate->oid.hash))

3093 - return -1;

3088 + finalize_hashfile(f, istate->oid.hash, CSUM_HASH_IN_STREAM);

3089 if (close_tempfile_gently(tempfile)) {

3090 error(_("could not close '%s'"), get_tempfile_path(tempfile));

3091 return -1;