{"thread":{"id":"55392","subject":"[PATCH 3/3] read-cache: delete unused hashing methods","startedAt":"2021-03-26T19:12:56Z","lastAt":"2021-11-29T15:42:15Z","messageCount":27,"participants":["Derrick Stolee via GitGitGadget","Derrick Stolee","Junio C Hamano","Jeff King","Ævar Arnfjörð Bjarmason"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"420277","messageId":"pull.916.git.1616785928.gitgitgadget@gmail.com","threadId":"55392","inReplyTo":null,"subject":"[PATCH 0/3] Convert index writes to use hashfile API","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-03-26T19:12:04Z","receivedAt":"2021-03-26T19:12:56Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"As I prepare some ideas on index v5, one thing that strikes me as an\ninteresting direction to try is to use the chunk-format API. This would make\nour extension model extremely simple (they become optional chunks, easily\nidentified by the table of contents).\n\nBut there is a huge hurdle to even starting that investigation: the index\nuses its own hashing methods, separate from the hashfile API in csum-file.c!\n\nThe internals of the algorithms are mostly identical. The only possible\nchange is that the buffer sizes are different: 8KB for hashfile and 128KB in\nread-cache.c. I was unable to find a performance difference in these two\nimplementations, despite testing on several repo sizes.\n\nThere is a subtle point about how the EOIE extension works in that it needs\na hash of just the previous extension data. This is solved by adding a new\n\"nested hashfile\" mechanism that computes the hash at one level and then\npasses the data below to another hashfile. (The good news is that this\nextension will not need to exist at all if we use the chunk-format API to\nmanage extensions.)\n\nThanks, -Stolee\n\nDerrick Stolee (3):\n csum-file: add nested_hashfile()\n read-cache: use hashfile instead of git_hash_ctx\n read-cache: delete unused hashing methods\n\n csum-file.c | 22 +++++++\n csum-file.h | 9 +++\n read-cache.c | 182 ++++++++++++++++-----------------------------------\n 3 files changed, 89 insertions(+), 124 deletions(-)\n\n\nbase-commit: 142430338477d9d1bb25be66267225fb58498d92\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-916%2Fderrickstolee%2Findex-hashfile-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-916/derrickstolee/index-hashfile-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/916\n-- \ngitgitgadget\n"},{"id":"420276","messageId":"e2d5a8dc919be7d4529157032bc6cd5e61482c9b.1616785928.git.gitgitgadget@gmail.com","threadId":"55392","inReplyTo":"pull.916.git.1616785928.gitgitgadget@gmail.com","subject":"[PATCH 3/3] read-cache: delete unused hashing methods","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-03-26T19:12:07Z","receivedAt":"2021-03-26T19:12:57Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThese methods were marked as MAYBE_UNUSED in the previous change to\navoid a complicated diff. Delete them entirely, since we now use the\nhashfile API instead of this custom hashing code.\n\nSigned-off-by: Derrick Stolee \n---\n read-cache.c | 64 ----------------------------------------------------\n 1 file changed, 64 deletions(-)\n\ndiff --git a/read-cache.c b/read-cache.c\nindex b9916350f331..3f3f2e01b9a2 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -2467,46 +2467,6 @@ int repo_index_has_changes(struct repository *repo,\n \t}\n }\n \n-#define WRITE_BUFFER_SIZE (128 * 1024)\n-static unsigned char write_buffer[WRITE_BUFFER_SIZE];\n-static unsigned long write_buffer_len;\n-\n-MAYBE_UNUSED\n-static int ce_write_flush(git_hash_ctx *context, int fd)\n-{\n-\tunsigned int buffered = write_buffer_len;\n-\tif (buffered) {\n-\t\tthe_hash_algo->update_fn(context, write_buffer, buffered);\n-\t\tif (write_in_full(fd, write_buffer, buffered) < 0)\n-\t\t\treturn -1;\n-\t\twrite_buffer_len = 0;\n-\t}\n-\treturn 0;\n-}\n-\n-MAYBE_UNUSED\n-static int ce_write(git_hash_ctx *context, int fd, void *data, unsigned int len)\n-{\n-\twhile (len) {\n-\t\tunsigned int buffered = write_buffer_len;\n-\t\tunsigned int partial = WRITE_BUFFER_SIZE - buffered;\n-\t\tif (partial > len)\n-\t\t\tpartial = len;\n-\t\tmemcpy(write_buffer + buffered, data, partial);\n-\t\tbuffered += partial;\n-\t\tif (buffered == WRITE_BUFFER_SIZE) {\n-\t\t\twrite_buffer_len = buffered;\n-\t\t\tif (ce_write_flush(context, fd))\n-\t\t\t\treturn -1;\n-\t\t\tbuffered = 0;\n-\t\t}\n-\t\twrite_buffer_len = buffered;\n-\t\tlen -= partial;\n-\t\tdata = (char *) data + partial;\n-\t}\n-\treturn 0;\n-}\n-\n static int write_index_ext_header(struct hashfile *f, unsigned int ext, unsigned int sz)\n {\n \thashwrite_be32(f, ext);\n@@ -2514,30 +2474,6 @@ static int write_index_ext_header(struct hashfile *f, unsigned int ext, unsigned\n \treturn 0;\n }\n \n-MAYBE_UNUSED\n-static int ce_flush(git_hash_ctx *context, int fd, unsigned char *hash)\n-{\n-\tunsigned int left = write_buffer_len;\n-\n-\tif (left) {\n-\t\twrite_buffer_len = 0;\n-\t\tthe_hash_algo->update_fn(context, write_buffer, left);\n-\t}\n-\n-\t/* Flush first if not enough space for hash signature */\n-\tif (left + the_hash_algo->rawsz > WRITE_BUFFER_SIZE) {\n-\t\tif (write_in_full(fd, write_buffer, left) < 0)\n-\t\t\treturn -1;\n-\t\tleft = 0;\n-\t}\n-\n-\t/* Append the hash signature at the end */\n-\tthe_hash_algo->final_fn(write_buffer + left, context);\n-\thashcpy(hash, write_buffer + left);\n-\tleft += the_hash_algo->rawsz;\n-\treturn (write_in_full(fd, write_buffer, left) < 0) ? -1 : 0;\n-}\n-\n static void ce_smudge_racily_clean_entry(struct index_state *istate,\n \t\t\t\t\t struct cache_entry *ce)\n {\n-- \ngitgitgadget\n"},{"id":"420278","messageId":"0eca529766fcbe70147bc28ce7f0692e645fd118.1616785928.git.gitgitgadget@gmail.com","threadId":"55392","inReplyTo":"pull.916.git.1616785928.gitgitgadget@gmail.com","subject":"[PATCH 1/3] csum-file: add nested_hashfile()","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-03-26T19:12:05Z","receivedAt":"2021-03-26T19:12:57Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe index writing code in do_write_index() uses a custom set of hashing\ncode, in part because it was introduced before the hashfile API. But\nalso, the End of Index Entries extension computes a hash of just the\nextension data, not the entire file preceding that extension.\n\nBefore converting the index writing code to use the hashfile API, create\na concept of a \"nested hashfile\". By adding a 'base' member to 'struct\nhashfile', we indicate that any writes to this hashfile should be passed\nalong to the base hashfile, too.\n\nIn the next change, the index code will use this to create a new\nhashfile wose base is the hashfile for the index. The outer hashfile\nwill compute the hash just for the extension details. Thus, it will\nfinalize earlier than the base hashfile, hence there is no modification\nto finalize_hashfile() here.\n\nSigned-off-by: Derrick Stolee \n---\n csum-file.c | 22 ++++++++++++++++++++++\n csum-file.h | 9 +++++++++\n 2 files changed, 31 insertions(+)\n\ndiff --git a/csum-file.c b/csum-file.c\nindex 0f35fa5ee47c..e73b35316e66 100644\n--- a/csum-file.c\n+++ b/csum-file.c\n@@ -13,6 +13,9 @@\n \n static void flush(struct hashfile *f, const void *buf, unsigned int count)\n {\n+\tif (f->base)\n+\t\treturn;\n+\n \tif (0 <= f->check_fd && count) {\n \t\tunsigned char check_buffer[8192];\n \t\tssize_t ret = read_in_full(f->check_fd, check_buffer, count);\n@@ -116,6 +119,9 @@ void hashwrite(struct hashfile *f, const void *buf, unsigned int count)\n \t\t}\n \t\tf->offset = offset;\n \t}\n+\n+\tif (f->base)\n+\t\thashwrite(f->base, buf, count);\n }\n \n struct hashfile *hashfd(int fd, const char *name)\n@@ -150,6 +156,7 @@ struct hashfile *hashfd_throughput(int fd, const char *name, struct progress *tp\n \tf->name = name;\n \tf->do_crc = 0;\n \tthe_hash_algo->init_fn(&f->ctx);\n+\tf->base = NULL;\n \treturn f;\n }\n \n@@ -184,3 +191,18 @@ uint32_t crc32_end(struct hashfile *f)\n \tf->do_crc = 0;\n \treturn f->crc32;\n }\n+\n+struct hashfile *nested_hashfile(struct hashfile *f)\n+{\n+\tstruct hashfile *n = xmalloc(sizeof(*f));\n+\tn->fd = -1;\n+\tn->check_fd = -1;\n+\tn->offset = 0;\n+\tn->total = 0;\n+\tn->tp = NULL;\n+\tn->name = NULL;\n+\tn->do_crc = 0;\n+\tthe_hash_algo->init_fn(&n->ctx);\n+\tn->base = f;\n+\treturn n;\n+}\ndiff --git a/csum-file.h b/csum-file.h\nindex e54d53d1d0b3..b8785e7ecb46 100644\n--- a/csum-file.h\n+++ b/csum-file.h\n@@ -16,6 +16,7 @@ struct hashfile {\n \tconst char *name;\n \tint do_crc;\n \tuint32_t crc32;\n+\tstruct hashfile *base;\n \tunsigned char buffer[8192];\n };\n \n@@ -42,6 +43,14 @@ void hashflush(struct hashfile *f);\n void crc32_begin(struct hashfile *);\n uint32_t crc32_end(struct hashfile *);\n \n+/*\n+ * A nested hashfile uses the same interface as a hashfile, and computes\n+ * a hash for the input bytes while passing them to the base hashfile\n+ * instead of writing them to its own file. This is useful for computing\n+ * a hash of a region within a file during the write.\n+ */\n+struct hashfile *nested_hashfile(struct hashfile *f);\n+\n /*\n * Returns the total number of bytes fed to the hashfile so far (including ones\n * that have not been written out to the descriptor yet).\n-- \ngitgitgadget\n\n"},{"id":"420279","messageId":"e2611bbc007a4583af55e006136c2bc74351a7de.1616785928.git.gitgitgadget@gmail.com","threadId":"55392","inReplyTo":"pull.916.git.1616785928.gitgitgadget@gmail.com","subject":"[PATCH 2/3] read-cache: use hashfile instead of git_hash_ctx","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-03-26T19:12:06Z","receivedAt":"2021-03-26T19:12:57Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe do_write_index() method in read-cache.c has its own hashing logic\nand buffering mechanism. Specifically, the ce_write() method was\nintroduced by 4990aadc (Speed up index file writing by chunking it\nnicely, 2005-04-20) and similar mechanisms were introduced a few months\nlater in c38138cd (git-pack-objects: write the pack files with a SHA1\ncsum, 2005-06-26). Based on the timing, in the early days of the Git\ncodebase, I figured that these roughly equivalent code paths were never\nunified only because it got lost in the shuffle. The hashfile API has\nsince been used extensively in other file formats, such as pack-indexes,\nmult-pack-indexes, and commit-graphs. Therefore, it seems prudent to\nunify the index writing code to use the same mechanism.\n\nI discovered this disparity while trying to create a new index format\nthat uses the chunk-format API. That API uses a hashfile as its base, so\nit is incompatible with the custom code in read-cache.c.\n\nThis rewrite of the logic is rather straightforward, except for the\nspecial case of creating a nested hashfile to handle computing the hash\nof the extension data just for the End of Index Entries extension. The\nprevious change introduced the concept for just this purpose.\n\nThe internals of the algorithms are mostly identical. The only\nmeaningful change is that the buffer sizes are different: 8KB for\nhashfile and 128KB in read-cache.c. I was unable to find a performance\ndifference in these two implementations, despite testing on several repo\nsizes. I also tried adjusting the buffer size of the hashfile struct for\na variety of sizes between 8KB and 128KB, and did not see a performance\nchange for any of the commands that currently use hashfiles.\n\nSome static methods become orphaned in this diff, so I marked them as\nMAYBE_UNUSED. The diff is much harder to read if they are deleted during\nthis change. Instead, they will be deleted in the following change.\n\nIn addition to the test suite passing, I computed indexes using the\nprevious binaries and the binaries compiled after this change, and found\nthe index data to be exactly equal. Finally, I did extensive performance\ntesting of \"git update-index --really-refresh\" on repos of various\nsizes, including one with over 2 million paths at HEAD. These tests\ndemonstrated less than 1% difference in behavior, so the performance\nshould be considered identical.\n\nSigned-off-by: Derrick Stolee \n---\n read-cache.c | 126 +++++++++++++++++++++++++--------------------------\n 1 file changed, 62 insertions(+), 64 deletions(-)\n\ndiff --git a/read-cache.c b/read-cache.c\nindex 5a907af2fb52..b9916350f331 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -25,6 +25,7 @@\n #include \"fsmonitor.h\"\n #include \"thread-utils.h\"\n #include \"progress.h\"\n+#include \"csum-file.h\"\n \n /* Mask for the name length in ce_flags in the on-disk index */\n \n@@ -1957,7 +1958,7 @@ static struct index_entry_offset_table *read_ieot_extension(const char *mmap, si\n static void write_ieot_extension(struct strbuf *sb, struct index_entry_offset_table *ieot);\n \n static size_t read_eoie_extension(const char *mmap, size_t mmap_size);\n-static void write_eoie_extension(struct strbuf *sb, git_hash_ctx *eoie_context, size_t offset);\n+static void write_eoie_extension(struct strbuf *sb, const unsigned char *hash, size_t offset);\n \n struct load_index_extensions\n {\n@@ -2470,6 +2471,7 @@ int repo_index_has_changes(struct repository *repo,\n static unsigned char write_buffer[WRITE_BUFFER_SIZE];\n static unsigned long write_buffer_len;\n \n+MAYBE_UNUSED\n static int ce_write_flush(git_hash_ctx *context, int fd)\n {\n \tunsigned int buffered = write_buffer_len;\n@@ -2482,6 +2484,7 @@ static int ce_write_flush(git_hash_ctx *context, int fd)\n \treturn 0;\n }\n \n+MAYBE_UNUSED\n static int ce_write(git_hash_ctx *context, int fd, void *data, unsigned int len)\n {\n \twhile (len) {\n@@ -2504,19 +2507,14 @@ static int ce_write(git_hash_ctx *context, int fd, void *data, unsigned int len)\n \treturn 0;\n }\n \n-static int write_index_ext_header(git_hash_ctx *context, git_hash_ctx *eoie_context,\n-\t\t\t\t int fd, unsigned int ext, unsigned int sz)\n+static int write_index_ext_header(struct hashfile *f, unsigned int ext, unsigned int sz)\n {\n-\text = htonl(ext);\n-\tsz = htonl(sz);\n-\tif (eoie_context) {\n-\t\tthe_hash_algo->update_fn(eoie_context, &ext, 4);\n-\t\tthe_hash_algo->update_fn(eoie_context, &sz, 4);\n-\t}\n-\treturn ((ce_write(context, fd, &ext, 4) < 0) ||\n-\t\t(ce_write(context, fd, &sz, 4) < 0)) ? -1 : 0;\n+\thashwrite_be32(f, ext);\n+\thashwrite_be32(f, sz);\n+\treturn 0;\n }\n \n+MAYBE_UNUSED\n static int ce_flush(git_hash_ctx *context, int fd, unsigned char *hash)\n {\n \tunsigned int left = write_buffer_len;\n@@ -2618,11 +2616,10 @@ static void copy_cache_entry_to_ondisk(struct ondisk_cache_entry *ondisk,\n \t}\n }\n \n-static int ce_write_entry(git_hash_ctx *c, int fd, struct cache_entry *ce,\n+static int ce_write_entry(struct hashfile *f, struct cache_entry *ce,\n \t\t\t struct strbuf *previous_name, struct ondisk_cache_entry *ondisk)\n {\n \tint size;\n-\tint result;\n \tunsigned int saved_namelen;\n \tint stripped_name = 0;\n \tstatic unsigned char padding[8] = { 0x00 };\n@@ -2638,11 +2635,9 @@ static int ce_write_entry(git_hash_ctx *c, int fd, struct cache_entry *ce,\n \tif (!previous_name) {\n \t\tint len = ce_namelen(ce);\n \t\tcopy_cache_entry_to_ondisk(ondisk, ce);\n-\t\tresult = ce_write(c, fd, ondisk, size);\n-\t\tif (!result)\n-\t\t\tresult = ce_write(c, fd, ce->name, len);\n-\t\tif (!result)\n-\t\t\tresult = ce_write(c, fd, padding, align_padding_size(size, len));\n+\t\thashwrite(f, ondisk, size);\n+\t\thashwrite(f, ce->name, len);\n+\t\thashwrite(f, padding, align_padding_size(size, len));\n \t} else {\n \t\tint common, to_remove, prefix_size;\n \t\tunsigned char to_remove_vi[16];\n@@ -2656,13 +2651,10 @@ static int ce_write_entry(git_hash_ctx *c, int fd, struct cache_entry *ce,\n \t\tprefix_size = encode_varint(to_remove, to_remove_vi);\n \n \t\tcopy_cache_entry_to_ondisk(ondisk, ce);\n-\t\tresult = ce_write(c, fd, ondisk, size);\n-\t\tif (!result)\n-\t\t\tresult = ce_write(c, fd, to_remove_vi, prefix_size);\n-\t\tif (!result)\n-\t\t\tresult = ce_write(c, fd, ce->name + common, ce_namelen(ce) - common);\n-\t\tif (!result)\n-\t\t\tresult = ce_write(c, fd, padding, 1);\n+\t\thashwrite(f, ondisk, size);\n+\t\thashwrite(f, to_remove_vi, prefix_size);\n+\t\thashwrite(f, ce->name + common, ce_namelen(ce) - common);\n+\t\thashwrite(f, padding, 1);\n \n \t\tstrbuf_splice(previous_name, common, to_remove,\n \t\t\t ce->name + common, ce_namelen(ce) - common);\n@@ -2672,7 +2664,7 @@ static int ce_write_entry(git_hash_ctx *c, int fd, struct cache_entry *ce,\n \t\tce->ce_flags &= ~CE_STRIP_NAME;\n \t}\n \n-\treturn result;\n+\treturn 0;\n }\n \n /*\n@@ -2784,8 +2776,7 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,\n \t\t\t int strip_extensions)\n {\n \tuint64_t start = getnanotime();\n-\tint newfd = tempfile->fd;\n-\tgit_hash_ctx c, eoie_c;\n+\tstruct hashfile *f, *eoie_f;\n \tstruct cache_header hdr;\n \tint i, err = 0, removed, extended, hdr_version;\n \tstruct cache_entry **cache = istate->cache;\n@@ -2799,6 +2790,8 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,\n \tstruct index_entry_offset_table *ieot = NULL;\n \tint nr, nr_threads;\n \n+\tf = hashfd(tempfile->fd, tempfile->filename.buf);\n+\n \tfor (i = removed = extended = 0; i < entries; i++) {\n \t\tif (cache[i]->ce_flags & CE_REMOVE)\n \t\t\tremoved++;\n@@ -2827,9 +2820,7 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,\n \thdr.hdr_version = htonl(hdr_version);\n \thdr.hdr_entries = htonl(entries - removed);\n \n-\tthe_hash_algo->init_fn(&c);\n-\tif (ce_write(&c, newfd, &hdr, sizeof(hdr)) < 0)\n-\t\treturn -1;\n+\thashwrite(f, &hdr, sizeof(hdr));\n \n \tif (!HAVE_THREADS || git_config_get_index_threads(&nr_threads))\n \t\tnr_threads = 1;\n@@ -2864,12 +2855,12 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,\n \t\t}\n \t}\n \n-\toffset = lseek(newfd, 0, SEEK_CUR);\n+\toffset = lseek(f->fd, 0, SEEK_CUR);\n \tif (offset < 0) {\n \t\tfree(ieot);\n \t\treturn -1;\n \t}\n-\toffset += write_buffer_len;\n+\n \tnr = 0;\n \tprevious_name = (hdr_version == 4) ? &previous_name_buf : NULL;\n \n@@ -2904,14 +2895,14 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,\n \t\t\tif (previous_name)\n \t\t\t\tprevious_name->buf[0] = 0;\n \t\t\tnr = 0;\n-\t\t\toffset = lseek(newfd, 0, SEEK_CUR);\n+\n+\t\t\toffset = lseek(f->fd, 0, SEEK_CUR);\n \t\t\tif (offset < 0) {\n \t\t\t\tfree(ieot);\n \t\t\t\treturn -1;\n \t\t\t}\n-\t\t\toffset += write_buffer_len;\n \t\t}\n-\t\tif (ce_write_entry(&c, newfd, ce, previous_name, (struct ondisk_cache_entry *)&ondisk) < 0)\n+\t\tif (ce_write_entry(f, ce, previous_name, (struct ondisk_cache_entry *)&ondisk) < 0)\n \t\t\terr = -1;\n \n \t\tif (err)\n@@ -2930,14 +2921,21 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,\n \t\treturn err;\n \t}\n \n-\t/* Write extension data here */\n-\toffset = lseek(newfd, 0, SEEK_CUR);\n+\toffset = lseek(f->fd, 0, SEEK_CUR);\n \tif (offset < 0) {\n \t\tfree(ieot);\n \t\treturn -1;\n \t}\n-\toffset += write_buffer_len;\n-\tthe_hash_algo->init_fn(&eoie_c);\n+\n+\t/*\n+\t * The extensions must be hashed on their own for use in the EOIE\n+\t * extension. Use a nested hashfile to compute the hash for this\n+\t * region while passing the buffer to the original hashfile.\n+\t */\n+\tif (offset && record_eoie())\n+\t\teoie_f = nested_hashfile(f);\n+\telse\n+\t\teoie_f = f;\n \n \t/*\n \t * Lets write out CACHE_EXT_INDEXENTRYOFFSETTABLE first so that we\n@@ -2950,8 +2948,8 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,\n \t\tstruct strbuf sb = STRBUF_INIT;\n \n \t\twrite_ieot_extension(&sb, ieot);\n-\t\terr = write_index_ext_header(&c, &eoie_c, newfd, CACHE_EXT_INDEXENTRYOFFSETTABLE, sb.len) < 0\n-\t\t\t|| ce_write(&c, newfd, sb.buf, sb.len) < 0;\n+\t\terr = write_index_ext_header(eoie_f, CACHE_EXT_INDEXENTRYOFFSETTABLE, sb.len) < 0;\n+\t\thashwrite(eoie_f, sb.buf, sb.len);\n \t\tstrbuf_release(&sb);\n \t\tfree(ieot);\n \t\tif (err)\n@@ -2963,9 +2961,9 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,\n \t\tstruct strbuf sb = STRBUF_INIT;\n \n \t\terr = write_link_extension(&sb, istate) < 0 ||\n-\t\t\twrite_index_ext_header(&c, &eoie_c, newfd, CACHE_EXT_LINK,\n-\t\t\t\t\t sb.len) < 0 ||\n-\t\t\tce_write(&c, newfd, sb.buf, sb.len) < 0;\n+\t\t\twrite_index_ext_header(eoie_f, CACHE_EXT_LINK,\n+\t\t\t\t\t sb.len) < 0;\n+\t\thashwrite(eoie_f, sb.buf, sb.len);\n \t\tstrbuf_release(&sb);\n \t\tif (err)\n \t\t\treturn -1;\n@@ -2974,8 +2972,8 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,\n \t\tstruct strbuf sb = STRBUF_INIT;\n \n \t\tcache_tree_write(&sb, istate->cache_tree);\n-\t\terr = write_index_ext_header(&c, &eoie_c, newfd, CACHE_EXT_TREE, sb.len) < 0\n-\t\t\t|| ce_write(&c, newfd, sb.buf, sb.len) < 0;\n+\t\terr = write_index_ext_header(eoie_f, CACHE_EXT_TREE, sb.len) < 0;\n+\t\thashwrite(eoie_f, sb.buf, sb.len);\n \t\tstrbuf_release(&sb);\n \t\tif (err)\n \t\t\treturn -1;\n@@ -2984,9 +2982,9 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,\n \t\tstruct strbuf sb = STRBUF_INIT;\n \n \t\tresolve_undo_write(&sb, istate->resolve_undo);\n-\t\terr = write_index_ext_header(&c, &eoie_c, newfd, CACHE_EXT_RESOLVE_UNDO,\n-\t\t\t\t\t sb.len) < 0\n-\t\t\t|| ce_write(&c, newfd, sb.buf, sb.len) < 0;\n+\t\terr = write_index_ext_header(eoie_f, CACHE_EXT_RESOLVE_UNDO,\n+\t\t\t\t\t sb.len) < 0;\n+\t\thashwrite(eoie_f, sb.buf, sb.len);\n \t\tstrbuf_release(&sb);\n \t\tif (err)\n \t\t\treturn -1;\n@@ -2995,9 +2993,9 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,\n \t\tstruct strbuf sb = STRBUF_INIT;\n \n \t\twrite_untracked_extension(&sb, istate->untracked);\n-\t\terr = write_index_ext_header(&c, &eoie_c, newfd, CACHE_EXT_UNTRACKED,\n-\t\t\t\t\t sb.len) < 0 ||\n-\t\t\tce_write(&c, newfd, sb.buf, sb.len) < 0;\n+\t\terr = write_index_ext_header(eoie_f, CACHE_EXT_UNTRACKED,\n+\t\t\t\t\t sb.len) < 0;\n+\t\thashwrite(eoie_f, sb.buf, sb.len);\n \t\tstrbuf_release(&sb);\n \t\tif (err)\n \t\t\treturn -1;\n@@ -3006,8 +3004,8 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,\n \t\tstruct strbuf sb = STRBUF_INIT;\n \n \t\twrite_fsmonitor_extension(&sb, istate);\n-\t\terr = write_index_ext_header(&c, &eoie_c, newfd, CACHE_EXT_FSMONITOR, sb.len) < 0\n-\t\t\t|| ce_write(&c, newfd, sb.buf, sb.len) < 0;\n+\t\terr = write_index_ext_header(eoie_f, CACHE_EXT_FSMONITOR, sb.len) < 0;\n+\t\thashwrite(eoie_f, sb.buf, sb.len);\n \t\tstrbuf_release(&sb);\n \t\tif (err)\n \t\t\treturn -1;\n@@ -3019,19 +3017,21 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,\n \t * read. Write it out regardless of the strip_extensions parameter as we need it\n \t * when loading the shared index.\n \t */\n-\tif (offset && record_eoie()) {\n+\tif (f != eoie_f) {\n \t\tstruct strbuf sb = STRBUF_INIT;\n+\t\tunsigned char hash[GIT_MAX_RAWSZ];\n \n-\t\twrite_eoie_extension(&sb, &eoie_c, offset);\n-\t\terr = write_index_ext_header(&c, NULL, newfd, CACHE_EXT_ENDOFINDEXENTRIES, sb.len) < 0\n-\t\t\t|| ce_write(&c, newfd, sb.buf, sb.len) < 0;\n+\t\tfinalize_hashfile(eoie_f, hash, 0);\n+\n+\t\twrite_eoie_extension(&sb, hash, offset);\n+\t\terr = write_index_ext_header(f, CACHE_EXT_ENDOFINDEXENTRIES, sb.len) < 0;\n+\t\thashwrite(f, sb.buf, sb.len);\n \t\tstrbuf_release(&sb);\n \t\tif (err)\n \t\t\treturn -1;\n \t}\n \n-\tif (ce_flush(&c, newfd, istate->oid.hash))\n-\t\treturn -1;\n+\tfinalize_hashfile(f, istate->oid.hash, CSUM_FSYNC | CSUM_HASH_IN_STREAM);\n \tif (close_tempfile_gently(tempfile)) {\n \t\terror(_(\"could not close '%s'\"), get_tempfile_path(tempfile));\n \t\treturn -1;\n@@ -3568,17 +3568,15 @@ static size_t read_eoie_extension(const char *mmap, size_t mmap_size)\n \treturn offset;\n }\n \n-static void write_eoie_extension(struct strbuf *sb, git_hash_ctx *eoie_context, size_t offset)\n+static void write_eoie_extension(struct strbuf *sb, const unsigned char *hash, size_t offset)\n {\n \tuint32_t buffer;\n-\tunsigned char hash[GIT_MAX_RAWSZ];\n \n \t/* offset */\n \tput_be32(&buffer, offset);\n \tstrbuf_add(sb, &buffer, sizeof(uint32_t));\n \n \t/* hash */\n-\tthe_hash_algo->final_fn(hash, eoie_context);\n \tstrbuf_add(sb, hash, the_hash_algo->rawsz);\n }\n \n-- \ngitgitgadget\n\n"},{"id":"420281","messageId":"01c1c74d-cc8a-0541-5193-723c41dd877c@gmail.com","threadId":"55392","inReplyTo":"pull.916.git.1616785928.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/3] Convert index writes to use hashfile API","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-03-26T20:16:05Z","receivedAt":"2021-03-26T20:17:05Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 3/26/2021 3:12 PM, Derrick Stolee via GitGitGadget wrote:\n> As I prepare some ideas on index v5, one thing that strikes me as an\n> interesting direction to try is to use the chunk-format API. This would make\n> our extension model extremely simple (they become optional chunks, easily\n> identified by the table of contents).\n> \n> But there is a huge hurdle to even starting that investigation: the index\n> uses its own hashing methods, separate from the hashfile API in csum-file.c!\n> \n> The internals of the algorithms are mostly identical. The only possible\n> change is that the buffer sizes are different: 8KB for hashfile and 128KB in\n> read-cache.c. I was unable to find a performance difference in these two\n> implementations, despite testing on several repo sizes.\n\nOf course, shortly after I send this series (thinking I've checked all the\ndetails carefully) I notice that I was using \"git update-index --really-refresh\"\nfor testing, but what I really wanted was \"git update-index --force-write\".\n\nIn this case, I _do_ see a performance degradation using the hashfile API.\nI will investigate whether this is just a poor implementation of the nesting\nhashfile, or something else more tricky. Changing the buffer size doesn't do\nthe trick.\n\nPlease ignore this series for now. Sorry for the noise.\n\n-Stolee\n"},{"id":"420481","messageId":"8b368872-bb36-db61-20d7-47aa5af0ec37@gmail.com","threadId":"55392","inReplyTo":"e2611bbc007a4583af55e006136c2bc74351a7de.1616785928.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/3] read-cache: use hashfile instead of git_hash_ctx","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-03-29T15:04:05Z","receivedAt":"2021-03-29T15:04:49Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 3/26/2021 3:12 PM, Derrick Stolee via GitGitGadget wrote:\n> \n> -\tif (ce_flush(&c, newfd, istate->oid.hash))\n> -\t\treturn -1;\n> +\tfinalize_hashfile(f, istate->oid.hash, CSUM_FSYNC | CSUM_HASH_IN_STREAM);\n> \tif (close_tempfile_gently(tempfile)) {\n> \t\terror(_(\"could not close '%s'\"), get_tempfile_path(tempfile));\n> \t\treturn -1;\n\nIt was bothering me all weekend why this change made index writes\nslower. The reason was this CSUM_FSYNC. Other performance measurement\n(instructions, branches, branch misses, etc.) are all really close to\nthe old code, so this I/O is the only explanation. And truly, it is\nthe case.\n\nIt seems that we avoid an fsync() on the index exactly for this perf\nreason, but we don't on other applications of the hashfile API because\nwrites are not critical paths.\n\nI'll update this series in a v2 soon that has some other improvements\nthat I noticed while digging deep into things.\n\nThanks,\n-Stolee\n"},{"id":"420503","messageId":"244d8b93-9d53-d660-ce08-d8b2362f75a8@gmail.com","threadId":"55392","inReplyTo":"8b368872-bb36-db61-20d7-47aa5af0ec37@gmail.com","subject":"Re: [PATCH 2/3] read-cache: use hashfile instead of git_hash_ctx","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-03-29T19:10:27Z","receivedAt":"2021-03-29T19:11:17Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 3/29/2021 11:04 AM, Derrick Stolee wrote:\n> On 3/26/2021 3:12 PM, Derrick Stolee via GitGitGadget wrote:\n>> \n>> -\tif (ce_flush(&c, newfd, istate->oid.hash))\n>> -\t\treturn -1;\n>> +\tfinalize_hashfile(f, istate->oid.hash, CSUM_FSYNC | CSUM_HASH_IN_STREAM);\n>> \tif (close_tempfile_gently(tempfile)) {\n>> \t\terror(_(\"could not close '%s'\"), get_tempfile_path(tempfile));\n>> \t\treturn -1;\n> \n> It was bothering me all weekend why this change made index writes\n> slower. The reason was this CSUM_FSYNC. Other performance measurement\n> (instructions, branches, branch misses, etc.) are all really close to\n> the old code, so this I/O is the only explanation. And truly, it is\n> the case.\n> \n> It seems that we avoid an fsync() on the index exactly for this perf\n> reason, but we don't on other applications of the hashfile API because\n> writes are not critical paths.\n> \n> I'll update this series in a v2 soon that has some other improvements\n> that I noticed while digging deep into things.\n\nSmall update: the pull request [1] has the latest changes that I\nthink will work for these needs. However, they will conflict with\nthe upcoming changes to ds/sparse-index to use index extensions.\n\n[1] https://github.com/gitgitgadget/git/pull/916\n\nI'll pause this series and re-send on top of ds/sparse-index\nafter it solidifies.\n\nThanks,\n-Stolee\n"},{"id":"424728","messageId":"pull.916.v2.git.1621254292.gitgitgadget@gmail.com","threadId":"55392","inReplyTo":"pull.916.git.1616785928.gitgitgadget@gmail.com","subject":"[PATCH v2 0/4] Convert index writes to use hashfile API","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-05-17T12:24:48Z","receivedAt":"2021-05-17T12:24:56Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"As I prepare some ideas on index v5, one thing that strikes me as an\ninteresting direction to try is to use the chunk-format API. This would make\nour extension model extremely simple (they become optional chunks, easily\nidentified by the table of contents).\n\nBut there is a huge hurdle to even starting that investigation: the index\nuses its own hashing methods, separate from the hashfile API in csum-file.c!\n\nThe internals of the algorithms are mostly identical. The only possible\nchange is that the buffer sizes are different: 8KB for hashfile and 128KB in\nread-cache.c. This change is actually relatively recent! I was able to see a\nperformance difference before changing that size, so I do make them equal.\nThis improves some other commands, too, such as git multi-pack-index write.\n\nThere is a subtle point about how the EOIE extension works in that it needs\na hash of just the previous extension headers. This won't be needed by the\nchunk-format API, so leave this mostly as-is, except where it collides\nsomewhat with the hashfile changes.\n\n\nUpdates in v2\n=============\n\n * Sorry for the false start on v1. I'm correctly testing index writes this\n time, but I also verified other commands, such as \"git add .\", to be sure\n I was exercising everything. I also previously had incorrect assumptions\n about the EOIE extension, since it is not triggered with \"small\" repos\n like the Linux kernel repository ;).\n\n * This version is rebased onto a recent version of 'master' so the\n conflicts with ds/sparse-index-protections are resolved.\n\n * This version abandons the nested hashfile concept, since that was only\n needed for the incorrect interpretation of the EOIE extension's hash.\n\n * This version does unify the buffer size to 128 KB with justification.\n\n * There is also some more unification of writing logic to use\n write_in_full() in the hashfile API.\n\n * When using check_fd, the hashfile API is no longer thread safe due to a\n stack-allocated buffer moving to the global scope. I'm not sure if there\n is a better way to handle this case.\n\nThanks, -Stolee\n\nDerrick Stolee (4):\n hashfile: use write_in_full()\n csum-file.h: increase hashfile buffer size\n read-cache: use hashfile instead of git_hash_ctx\n read-cache: delete unused hashing methods\n\n chunk-format.c | 12 ++--\n csum-file.c | 45 ++++++------\n csum-file.h | 4 +-\n read-cache.c | 191 ++++++++++++++++---------------------------------\n 4 files changed, 94 insertions(+), 158 deletions(-)\n\n\nbase-commit: 97eea85a0a1ec66d356567808a1e4ca2367e0ce7\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-916%2Fderrickstolee%2Findex-hashfile-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-916/derrickstolee/index-hashfile-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/916\n\nRange-diff vs v1:\n\n -: ------------ > 1: b3e578ac0365 hashfile: use write_in_full()\n 1: 0eca529766fc ! 2: 9dc602f6c422 csum-file: add nested_hashfile()\n @@ Metadata\n Author: Derrick Stolee \n \n ## Commit message ##\n - csum-file: add nested_hashfile()\n + csum-file.h: increase hashfile buffer size\n \n - The index writing code in do_write_index() uses a custom set of hashing\n - code, in part because it was introduced before the hashfile API. But\n - also, the End of Index Entries extension computes a hash of just the\n - extension data, not the entire file preceding that extension.\n + The hashfile API uses a hard-coded buffer size of 8KB and has ever since\n + it was introduced in c38138c (git-pack-objects: write the pack files\n + with a SHA1 csum, 2005-06-26). It performs a similar function to the\n + hashing buffers in read-cache.c, but that code was updated from 8KB to\n + 128KB in f279894 (read-cache: make the index write buffer size 128K,\n + 2021-02-18). The justification there was that do_write_index() improves\n + from 1.02s to 0.72s.\n \n - Before converting the index writing code to use the hashfile API, create\n - a concept of a \"nested hashfile\". By adding a 'base' member to 'struct\n - hashfile', we indicate that any writes to this hashfile should be passed\n - along to the base hashfile, too.\n + There is a buffer, check_buffer, that is used to verify the check_fd\n + file descriptor. When this buffer increases to 128K to fit the data\n + being flushed, it causes the stack to overflow the limits placed in the\n + test suite. By moving this to a static buffer, we stop using stack data\n + for this purpose, but we lose some thread-safety. This change makes it\n + unsafe to write to multiple hashfiles across different threads.\n \n - In the next change, the index code will use this to create a new\n - hashfile wose base is the hashfile for the index. The outer hashfile\n - will compute the hash just for the extension details. Thus, it will\n - finalize earlier than the base hashfile, hence there is no modification\n - to finalize_hashfile() here.\n + By adding a new trace2 region in the chunk-format API, we can see that\n + the writing portion of 'git multi-pack-index write' lowers from ~1.49s\n + to ~1.47s on a Linux machine. These effects may be more pronounced or\n + diminished on other filesystems. The end-to-end timing is too noisy to\n + have a definitive change either way.\n \n Signed-off-by: Derrick Stolee \n \n - ## csum-file.c ##\n -@@\n + ## chunk-format.c ##\n +@@ chunk-format.c: void add_chunk(struct chunkfile *cf,\n \n - static void flush(struct hashfile *f, const void *buf, unsigned int count)\n + int write_chunkfile(struct chunkfile *cf, void *data)\n {\n -+\tif (f->base)\n -+\t\treturn;\n +-\tint i;\n ++\tint i, result = 0;\n + \tuint64_t cur_offset = hashfile_total(cf->f);\n + \n ++\ttrace2_region_enter(\"chunkfile\", \"write\", the_repository);\n +\n - \tif (0 <= f->check_fd && count) {\n - \t\tunsigned char check_buffer[8192];\n - \t\tssize_t ret = read_in_full(f->check_fd, check_buffer, count);\n -@@ csum-file.c: void hashwrite(struct hashfile *f, const void *buf, unsigned int count)\n - \t\t}\n - \t\tf->offset = offset;\n + \t/* Add the table of contents to the current offset */\n + \tcur_offset += (cf->chunks_nr + 1) * CHUNK_TOC_ENTRY_SIZE;\n + \n +@@ chunk-format.c: int write_chunkfile(struct chunkfile *cf, void *data)\n + \n + \tfor (i = 0; i < cf->chunks_nr; i++) {\n + \t\toff_t start_offset = hashfile_total(cf->f);\n +-\t\tint result = cf->chunks[i].write_fn(cf->f, data);\n ++\t\tresult = cf->chunks[i].write_fn(cf->f, data);\n + \n + \t\tif (result)\n +-\t\t\treturn result;\n ++\t\t\tgoto cleanup;\n + \n + \t\tif (hashfile_total(cf->f) - start_offset != cf->chunks[i].size)\n + \t\t\tBUG(\"expected to write %\"PRId64\" bytes to chunk %\"PRIx32\", but wrote %\"PRId64\" instead\",\n +@@ chunk-format.c: int write_chunkfile(struct chunkfile *cf, void *data)\n + \t\t\t hashfile_total(cf->f) - start_offset);\n \t}\n -+\n -+\tif (f->base)\n -+\t\thashwrite(f->base, buf, count);\n - }\n \n - struct hashfile *hashfd(int fd, const char *name)\n -@@ csum-file.c: struct hashfile *hashfd_throughput(int fd, const char *name, struct progress *tp\n - \tf->name = name;\n - \tf->do_crc = 0;\n - \tthe_hash_algo->init_fn(&f->ctx);\n -+\tf->base = NULL;\n - \treturn f;\n +-\treturn 0;\n ++cleanup:\n ++\ttrace2_region_leave(\"chunkfile\", \"write\", the_repository);\n ++\treturn result;\n }\n \n -@@ csum-file.c: uint32_t crc32_end(struct hashfile *f)\n - \tf->do_crc = 0;\n - \treturn f->crc32;\n - }\n -+\n -+struct hashfile *nested_hashfile(struct hashfile *f)\n + int read_table_of_contents(struct chunkfile *cf,\n +\n + ## csum-file.c ##\n +@@\n + #include \"progress.h\"\n + #include \"csum-file.h\"\n + \n ++static void verify_buffer_or_die(struct hashfile *f,\n ++\t\t\t\t const void *buf,\n ++\t\t\t\t unsigned int count)\n +{\n -+\tstruct hashfile *n = xmalloc(sizeof(*f));\n -+\tn->fd = -1;\n -+\tn->check_fd = -1;\n -+\tn->offset = 0;\n -+\tn->total = 0;\n -+\tn->tp = NULL;\n -+\tn->name = NULL;\n -+\tn->do_crc = 0;\n -+\tthe_hash_algo->init_fn(&n->ctx);\n -+\tn->base = f;\n -+\treturn n;\n ++\tstatic unsigned char check_buffer[WRITE_BUFFER_SIZE];\n ++\tssize_t ret = read_in_full(f->check_fd, check_buffer, count);\n ++\n ++\tif (ret < 0)\n ++\t\tdie_errno(\"%s: sha1 file read error\", f->name);\n ++\tif (ret != count)\n ++\t\tdie(\"%s: sha1 file truncated\", f->name);\n ++\tif (memcmp(buf, check_buffer, count))\n ++\t\tdie(\"sha1 file '%s' validation error\", f->name);\n +}\n ++\n + static void flush(struct hashfile *f, const void *buf, unsigned int count)\n + {\n +-\tif (0 <= f->check_fd && count) {\n +-\t\tunsigned char check_buffer[8192];\n +-\t\tssize_t ret = read_in_full(f->check_fd, check_buffer, count);\n +-\n +-\t\tif (ret < 0)\n +-\t\t\tdie_errno(\"%s: sha1 file read error\", f->name);\n +-\t\tif (ret != count)\n +-\t\t\tdie(\"%s: sha1 file truncated\", f->name);\n +-\t\tif (memcmp(buf, check_buffer, count))\n +-\t\t\tdie(\"sha1 file '%s' validation error\", f->name);\n +-\t}\n ++\tif (0 <= f->check_fd && count)\n ++\t\tverify_buffer_or_die(f, buf, count);\n + \n + \tif (write_in_full(f->fd, buf, count) < 0) {\n + \t\tif (errno == ENOSPC)\n \n ## csum-file.h ##\n +@@\n + \n + struct progress;\n + \n ++#define WRITE_BUFFER_SIZE (128 * 1024)\n ++\n + /* A SHA1-protected file */\n + struct hashfile {\n + \tint fd;\n @@ csum-file.h: struct hashfile {\n \tconst char *name;\n \tint do_crc;\n \tuint32_t crc32;\n -+\tstruct hashfile *base;\n - \tunsigned char buffer[8192];\n +-\tunsigned char buffer[8192];\n ++\tunsigned char buffer[WRITE_BUFFER_SIZE];\n };\n \n -@@ csum-file.h: void hashflush(struct hashfile *f);\n - void crc32_begin(struct hashfile *);\n - uint32_t crc32_end(struct hashfile *);\n - \n -+/*\n -+ * A nested hashfile uses the same interface as a hashfile, and computes\n -+ * a hash for the input bytes while passing them to the base hashfile\n -+ * instead of writing them to its own file. This is useful for computing\n -+ * a hash of a region within a file during the write.\n -+ */\n -+struct hashfile *nested_hashfile(struct hashfile *f);\n -+\n - /*\n - * Returns the total number of bytes fed to the hashfile so far (including ones\n - * that have not been written out to the descriptor yet).\n + /* Checkpoint */\n 2: e2611bbc007a ! 3: b94172ccf5e9 read-cache: use hashfile instead of git_hash_ctx\n @@ Commit message\n that uses the chunk-format API. That API uses a hashfile as its base, so\n it is incompatible with the custom code in read-cache.c.\n \n - This rewrite of the logic is rather straightforward, except for the\n - special case of creating a nested hashfile to handle computing the hash\n - of the extension data just for the End of Index Entries extension. The\n - previous change introduced the concept for just this purpose.\n + This rewrite is rather straightforward. It replaces all writes to the\n + temporary file with writes to the hashfile struct. This takes care of\n + many of the direct interactions with the_hash_algo.\n \n - The internals of the algorithms are mostly identical. The only\n - meaningful change is that the buffer sizes are different: 8KB for\n - hashfile and 128KB in read-cache.c. I was unable to find a performance\n - difference in these two implementations, despite testing on several repo\n - sizes. I also tried adjusting the buffer size of the hashfile struct for\n - a variety of sizes between 8KB and 128KB, and did not see a performance\n - change for any of the commands that currently use hashfiles.\n + There are still some remaining: the extension headers are hashed for use\n + in the End of Index Entries (EOIE) extension. This use of the\n + git_hash_ctx is left as-is. There are multiple reasons to not use a\n + hashfile here, including the fact that the data is not actually writing\n + to a file, just a hash computation. These hashes do not block our\n + adoption of the chunk-format API in a future change to the index, so\n + leave it as-is.\n +\n + The internals of the algorithms are mostly identical. Previously, the\n + hashfile API used a smaller 8KB buffer instead of the 128KB buffer from\n + read-cache.c. The previous change already unified these sizes.\n +\n + There is one subtle point: we do not pass the CSUM_FSYNC to the\n + finalize_hashfile() method, which differs from most consumers of the\n + hashfile API. The extra fsync() call indicated by this flag causes a\n + significant peformance degradation that is noticeable for quick\n + commands that write the index, such as \"git add\". Other consumers can\n + absorb this cost with their more complicated data structure\n + organization, and further writing structures such as pack-files and\n + commit-graphs is rarely in the critical path for common user\n + interactions.\n \n Some static methods become orphaned in this diff, so I marked them as\n MAYBE_UNUSED. The diff is much harder to read if they are deleted during\n @@ Commit message\n In addition to the test suite passing, I computed indexes using the\n previous binaries and the binaries compiled after this change, and found\n the index data to be exactly equal. Finally, I did extensive performance\n - testing of \"git update-index --really-refresh\" on repos of various\n - sizes, including one with over 2 million paths at HEAD. These tests\n + testing of \"git update-index --force-write\" on repos of various sizes,\n + including one with over 2 million paths at HEAD. These tests\n demonstrated less than 1% difference in behavior, so the performance\n should be considered identical.\n \n @@ Commit message\n \n ## read-cache.c ##\n @@\n - #include \"fsmonitor.h\"\n #include \"thread-utils.h\"\n #include \"progress.h\"\n + #include \"sparse-index.h\"\n +#include \"csum-file.h\"\n \n /* Mask for the name length in ce_flags in the on-disk index */\n \n -@@ read-cache.c: static struct index_entry_offset_table *read_ieot_extension(const char *mmap, si\n - static void write_ieot_extension(struct strbuf *sb, struct index_entry_offset_table *ieot);\n - \n - static size_t read_eoie_extension(const char *mmap, size_t mmap_size);\n --static void write_eoie_extension(struct strbuf *sb, git_hash_ctx *eoie_context, size_t offset);\n -+static void write_eoie_extension(struct strbuf *sb, const unsigned char *hash, size_t offset);\n - \n - struct load_index_extensions\n - {\n @@ read-cache.c: int repo_index_has_changes(struct repository *repo,\n static unsigned char write_buffer[WRITE_BUFFER_SIZE];\n static unsigned long write_buffer_len;\n @@ read-cache.c: static int ce_write(git_hash_ctx *context, int fd, void *data, uns\n \n -static int write_index_ext_header(git_hash_ctx *context, git_hash_ctx *eoie_context,\n -\t\t\t\t int fd, unsigned int ext, unsigned int sz)\n -+static int write_index_ext_header(struct hashfile *f, unsigned int ext, unsigned int sz)\n ++static int write_index_ext_header(struct hashfile *f,\n ++\t\t\t\t git_hash_ctx *eoie_f,\n ++\t\t\t\t unsigned int ext,\n ++\t\t\t\t unsigned int sz)\n {\n -\text = htonl(ext);\n -\tsz = htonl(sz);\n -\tif (eoie_context) {\n -\t\tthe_hash_algo->update_fn(eoie_context, &ext, 4);\n -\t\tthe_hash_algo->update_fn(eoie_context, &sz, 4);\n --\t}\n --\treturn ((ce_write(context, fd, &ext, 4) < 0) ||\n --\t\t(ce_write(context, fd, &sz, 4) < 0)) ? -1 : 0;\n +\thashwrite_be32(f, ext);\n +\thashwrite_be32(f, sz);\n ++\n ++\tif (eoie_f) {\n ++\t\text = htonl(ext);\n ++\t\tsz = htonl(sz);\n ++\t\tthe_hash_algo->update_fn(eoie_f, &ext, sizeof(ext));\n ++\t\tthe_hash_algo->update_fn(eoie_f, &sz, sizeof(sz));\n + \t}\n +-\treturn ((ce_write(context, fd, &ext, 4) < 0) ||\n +-\t\t(ce_write(context, fd, &sz, 4) < 0)) ? -1 : 0;\n +\treturn 0;\n }\n \n @@ read-cache.c: static int do_write_index(struct index_state *istate, struct tempf\n \tuint64_t start = getnanotime();\n -\tint newfd = tempfile->fd;\n -\tgit_hash_ctx c, eoie_c;\n -+\tstruct hashfile *f, *eoie_f;\n ++\tstruct hashfile *f;\n ++\tgit_hash_ctx *eoie_c = NULL;\n \tstruct cache_header hdr;\n \tint i, err = 0, removed, extended, hdr_version;\n \tstruct cache_entry **cache = istate->cache;\n @@ read-cache.c: static int do_write_index(struct index_state *istate, struct tempf\n \t}\n \n -\toffset = lseek(newfd, 0, SEEK_CUR);\n -+\toffset = lseek(f->fd, 0, SEEK_CUR);\n - \tif (offset < 0) {\n - \t\tfree(ieot);\n - \t\treturn -1;\n - \t}\n +-\tif (offset < 0) {\n +-\t\tfree(ieot);\n +-\t\treturn -1;\n +-\t}\n -\toffset += write_buffer_len;\n ++\toffset = hashfile_total(f);\n +\n \tnr = 0;\n \tprevious_name = (hdr_version == 4) ? &previous_name_buf : NULL;\n @@ read-cache.c: static int do_write_index(struct index_state *istate, struct tempf\n \t\t\t\tprevious_name->buf[0] = 0;\n \t\t\tnr = 0;\n -\t\t\toffset = lseek(newfd, 0, SEEK_CUR);\n -+\n -+\t\t\toffset = lseek(f->fd, 0, SEEK_CUR);\n - \t\t\tif (offset < 0) {\n - \t\t\t\tfree(ieot);\n - \t\t\t\treturn -1;\n - \t\t\t}\n +-\t\t\tif (offset < 0) {\n +-\t\t\t\tfree(ieot);\n +-\t\t\t\treturn -1;\n +-\t\t\t}\n -\t\t\toffset += write_buffer_len;\n ++\n ++\t\t\toffset = hashfile_total(f);\n \t\t}\n -\t\tif (ce_write_entry(&c, newfd, ce, previous_name, (struct ondisk_cache_entry *)&ondisk) < 0)\n +\t\tif (ce_write_entry(f, ce, previous_name, (struct ondisk_cache_entry *)&ondisk) < 0)\n @@ read-cache.c: static int do_write_index(struct index_state *istate, struct tempf\n \n -\t/* Write extension data here */\n -\toffset = lseek(newfd, 0, SEEK_CUR);\n -+\toffset = lseek(f->fd, 0, SEEK_CUR);\n - \tif (offset < 0) {\n - \t\tfree(ieot);\n - \t\treturn -1;\n - \t}\n --\toffset += write_buffer_len;\n --\tthe_hash_algo->init_fn(&eoie_c);\n +-\tif (offset < 0) {\n +-\t\tfree(ieot);\n +-\t\treturn -1;\n ++\toffset = hashfile_total(f);\n +\n +\t/*\n -+\t * The extensions must be hashed on their own for use in the EOIE\n -+\t * extension. Use a nested hashfile to compute the hash for this\n -+\t * region while passing the buffer to the original hashfile.\n ++\t * The extension headers must be hashed on their own for the\n ++\t * EOIE extension. Create a hashfile here to compute that hash.\n +\t */\n -+\tif (offset && record_eoie())\n -+\t\teoie_f = nested_hashfile(f);\n -+\telse\n -+\t\teoie_f = f;\n ++\tif (offset && record_eoie()) {\n ++\t\tCALLOC_ARRAY(eoie_c, 1);\n ++\t\tthe_hash_algo->init_fn(eoie_c);\n + \t}\n +-\toffset += write_buffer_len;\n +-\tthe_hash_algo->init_fn(&eoie_c);\n \n \t/*\n \t * Lets write out CACHE_EXT_INDEXENTRYOFFSETTABLE first so that we\n @@ read-cache.c: static int do_write_index(struct index_state *istate, struct tempf\n \t\twrite_ieot_extension(&sb, ieot);\n -\t\terr = write_index_ext_header(&c, &eoie_c, newfd, CACHE_EXT_INDEXENTRYOFFSETTABLE, sb.len) < 0\n -\t\t\t|| ce_write(&c, newfd, sb.buf, sb.len) < 0;\n -+\t\terr = write_index_ext_header(eoie_f, CACHE_EXT_INDEXENTRYOFFSETTABLE, sb.len) < 0;\n -+\t\thashwrite(eoie_f, sb.buf, sb.len);\n ++\t\terr = write_index_ext_header(f, eoie_c, CACHE_EXT_INDEXENTRYOFFSETTABLE, sb.len) < 0;\n ++\t\thashwrite(f, sb.buf, sb.len);\n \t\tstrbuf_release(&sb);\n \t\tfree(ieot);\n \t\tif (err)\n @@ read-cache.c: static int do_write_index(struct index_state *istate, struct tempf\n -\t\t\twrite_index_ext_header(&c, &eoie_c, newfd, CACHE_EXT_LINK,\n -\t\t\t\t\t sb.len) < 0 ||\n -\t\t\tce_write(&c, newfd, sb.buf, sb.len) < 0;\n -+\t\t\twrite_index_ext_header(eoie_f, CACHE_EXT_LINK,\n ++\t\t\twrite_index_ext_header(f, eoie_c, CACHE_EXT_LINK,\n +\t\t\t\t\t sb.len) < 0;\n -+\t\thashwrite(eoie_f, sb.buf, sb.len);\n ++\t\thashwrite(f, sb.buf, sb.len);\n \t\tstrbuf_release(&sb);\n \t\tif (err)\n \t\t\treturn -1;\n @@ read-cache.c: static int do_write_index(struct index_state *istate, struct tempf\n \t\tcache_tree_write(&sb, istate->cache_tree);\n -\t\terr = write_index_ext_header(&c, &eoie_c, newfd, CACHE_EXT_TREE, sb.len) < 0\n -\t\t\t|| ce_write(&c, newfd, sb.buf, sb.len) < 0;\n -+\t\terr = write_index_ext_header(eoie_f, CACHE_EXT_TREE, sb.len) < 0;\n -+\t\thashwrite(eoie_f, sb.buf, sb.len);\n ++\t\terr = write_index_ext_header(f, eoie_c, CACHE_EXT_TREE, sb.len) < 0;\n ++\t\thashwrite(f, sb.buf, sb.len);\n \t\tstrbuf_release(&sb);\n \t\tif (err)\n \t\t\treturn -1;\n @@ read-cache.c: static int do_write_index(struct index_state *istate, struct tempf\n -\t\terr = write_index_ext_header(&c, &eoie_c, newfd, CACHE_EXT_RESOLVE_UNDO,\n -\t\t\t\t\t sb.len) < 0\n -\t\t\t|| ce_write(&c, newfd, sb.buf, sb.len) < 0;\n -+\t\terr = write_index_ext_header(eoie_f, CACHE_EXT_RESOLVE_UNDO,\n ++\t\terr = write_index_ext_header(f, eoie_c, CACHE_EXT_RESOLVE_UNDO,\n +\t\t\t\t\t sb.len) < 0;\n -+\t\thashwrite(eoie_f, sb.buf, sb.len);\n ++\t\thashwrite(f, sb.buf, sb.len);\n \t\tstrbuf_release(&sb);\n \t\tif (err)\n \t\t\treturn -1;\n @@ read-cache.c: static int do_write_index(struct index_state *istate, struct tempf\n -\t\terr = write_index_ext_header(&c, &eoie_c, newfd, CACHE_EXT_UNTRACKED,\n -\t\t\t\t\t sb.len) < 0 ||\n -\t\t\tce_write(&c, newfd, sb.buf, sb.len) < 0;\n -+\t\terr = write_index_ext_header(eoie_f, CACHE_EXT_UNTRACKED,\n ++\t\terr = write_index_ext_header(f, eoie_c, CACHE_EXT_UNTRACKED,\n +\t\t\t\t\t sb.len) < 0;\n -+\t\thashwrite(eoie_f, sb.buf, sb.len);\n ++\t\thashwrite(f, sb.buf, sb.len);\n \t\tstrbuf_release(&sb);\n \t\tif (err)\n \t\t\treturn -1;\n @@ read-cache.c: static int do_write_index(struct index_state *istate, struct tempf\n \t\twrite_fsmonitor_extension(&sb, istate);\n -\t\terr = write_index_ext_header(&c, &eoie_c, newfd, CACHE_EXT_FSMONITOR, sb.len) < 0\n -\t\t\t|| ce_write(&c, newfd, sb.buf, sb.len) < 0;\n -+\t\terr = write_index_ext_header(eoie_f, CACHE_EXT_FSMONITOR, sb.len) < 0;\n -+\t\thashwrite(eoie_f, sb.buf, sb.len);\n ++\t\terr = write_index_ext_header(f, eoie_c, CACHE_EXT_FSMONITOR, sb.len) < 0;\n ++\t\thashwrite(f, sb.buf, sb.len);\n \t\tstrbuf_release(&sb);\n \t\tif (err)\n \t\t\treturn -1;\n + \t}\n + \tif (istate->sparse_index) {\n +-\t\tif (write_index_ext_header(&c, &eoie_c, newfd, CACHE_EXT_SPARSE_DIRECTORIES, 0) < 0)\n ++\t\tif (write_index_ext_header(f, eoie_c, CACHE_EXT_SPARSE_DIRECTORIES, 0) < 0)\n + \t\t\treturn -1;\n + \t}\n + \n @@ read-cache.c: static int do_write_index(struct index_state *istate, struct tempfile *tempfile,\n \t * read. Write it out regardless of the strip_extensions parameter as we need it\n \t * when loading the shared index.\n \t */\n -\tif (offset && record_eoie()) {\n -+\tif (f != eoie_f) {\n ++\tif (eoie_c) {\n \t\tstruct strbuf sb = STRBUF_INIT;\n -+\t\tunsigned char hash[GIT_MAX_RAWSZ];\n \n -\t\twrite_eoie_extension(&sb, &eoie_c, offset);\n -\t\terr = write_index_ext_header(&c, NULL, newfd, CACHE_EXT_ENDOFINDEXENTRIES, sb.len) < 0\n -\t\t\t|| ce_write(&c, newfd, sb.buf, sb.len) < 0;\n -+\t\tfinalize_hashfile(eoie_f, hash, 0);\n -+\n -+\t\twrite_eoie_extension(&sb, hash, offset);\n -+\t\terr = write_index_ext_header(f, CACHE_EXT_ENDOFINDEXENTRIES, sb.len) < 0;\n ++\t\twrite_eoie_extension(&sb, eoie_c, offset);\n ++\t\terr = write_index_ext_header(f, NULL, CACHE_EXT_ENDOFINDEXENTRIES, sb.len) < 0;\n +\t\thashwrite(f, sb.buf, sb.len);\n \t\tstrbuf_release(&sb);\n \t\tif (err)\n @@ read-cache.c: static int do_write_index(struct index_state *istate, struct tempf\n \n -\tif (ce_flush(&c, newfd, istate->oid.hash))\n -\t\treturn -1;\n -+\tfinalize_hashfile(f, istate->oid.hash, CSUM_FSYNC | CSUM_HASH_IN_STREAM);\n ++\tfinalize_hashfile(f, istate->oid.hash, CSUM_HASH_IN_STREAM);\n \tif (close_tempfile_gently(tempfile)) {\n \t\terror(_(\"could not close '%s'\"), get_tempfile_path(tempfile));\n \t\treturn -1;\n -@@ read-cache.c: static size_t read_eoie_extension(const char *mmap, size_t mmap_size)\n - \treturn offset;\n - }\n - \n --static void write_eoie_extension(struct strbuf *sb, git_hash_ctx *eoie_context, size_t offset)\n -+static void write_eoie_extension(struct strbuf *sb, const unsigned char *hash, size_t offset)\n - {\n - \tuint32_t buffer;\n --\tunsigned char hash[GIT_MAX_RAWSZ];\n - \n - \t/* offset */\n - \tput_be32(&buffer, offset);\n - \tstrbuf_add(sb, &buffer, sizeof(uint32_t));\n - \n - \t/* hash */\n --\tthe_hash_algo->final_fn(hash, eoie_context);\n - \tstrbuf_add(sb, hash, the_hash_algo->rawsz);\n - }\n - \n 3: e2d5a8dc919b ! 4: 4b3814eb4c80 read-cache: delete unused hashing methods\n @@ read-cache.c: int repo_index_has_changes(struct repository *repo,\n -\treturn 0;\n -}\n -\n - static int write_index_ext_header(struct hashfile *f, unsigned int ext, unsigned int sz)\n - {\n - \thashwrite_be32(f, ext);\n -@@ read-cache.c: static int write_index_ext_header(struct hashfile *f, unsigned int ext, unsigned\n + static int write_index_ext_header(struct hashfile *f,\n + \t\t\t\t git_hash_ctx *eoie_f,\n + \t\t\t\t unsigned int ext,\n +@@ read-cache.c: static int write_index_ext_header(struct hashfile *f,\n \treturn 0;\n }\n \n\n-- \ngitgitgadget\n"},{"id":"424729","messageId":"b3e578ac0365fba9ef015f2e9a06593ce85d2814.1621254292.git.gitgitgadget@gmail.com","threadId":"55392","inReplyTo":"pull.916.v2.git.1621254292.gitgitgadget@gmail.com","subject":"[PATCH v2 1/4] hashfile: use write_in_full()","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-05-17T12:24:49Z","receivedAt":"2021-05-17T12:25:02Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe flush() logic in csum-file.c was introduced originally by c38138c\n(git-pack-objects: write the pack files with a SHA1 csum, 2005-06-26)\nand a portion of the logic performs similar utility to write_in_full()\nin wrapper.c. The history of write_in_full() is full of moves and\nrenames, but was originally introduced by 7230e6d (Add write_or_die(), a\nhelper function, 2006-08-21).\n\nThe point of these sections of code are to flush a write buffer using\nxwrite() and report errors in the case of disk space issues or other\ngeneric input/output errors. The logic in flush() can interpret the\noutput of write_in_full() to provide the correct error messages to\nusers.\n\nThe logic in the hashfile API has an additional set of logic to augment\nthe progress indicator between calls to xwrite(). This was introduced by\n2a128d6 (add throughput display to git-push, 2007-10-30). It seems that\nsince the hashfile's buffer is only 8KB, these additional progress\nindicators might not be incredibly necessary. Instead, update the\nprogress only when write_in_full() complete.\n\nSigned-off-by: Derrick Stolee \n---\n csum-file.c | 17 +++++------------\n 1 file changed, 5 insertions(+), 12 deletions(-)\n\ndiff --git a/csum-file.c b/csum-file.c\nindex 7510950fa3e9..3c26389d4914 100644\n--- a/csum-file.c\n+++ b/csum-file.c\n@@ -25,21 +25,14 @@ static void flush(struct hashfile *f, const void *buf, unsigned int count)\n \t\t\tdie(\"sha1 file '%s' validation error\", f->name);\n \t}\n \n-\tfor (;;) {\n-\t\tint ret = xwrite(f->fd, buf, count);\n-\t\tif (ret > 0) {\n-\t\t\tf->total += ret;\n-\t\t\tdisplay_throughput(f->tp, f->total);\n-\t\t\tbuf = (char *) buf + ret;\n-\t\t\tcount -= ret;\n-\t\t\tif (count)\n-\t\t\t\tcontinue;\n-\t\t\treturn;\n-\t\t}\n-\t\tif (!ret)\n+\tif (write_in_full(f->fd, buf, count) < 0) {\n+\t\tif (errno == ENOSPC)\n \t\t\tdie(\"sha1 file '%s' write error. Out of diskspace\", f->name);\n \t\tdie_errno(\"sha1 file '%s' write error\", f->name);\n \t}\n+\n+\tf->total += count;\n+\tdisplay_throughput(f->tp, f->total);\n }\n \n void hashflush(struct hashfile *f)\n-- \ngitgitgadget\n\n"},{"id":"424730","messageId":"b94172ccf5e91fe59a1d32774dbec23e624f1135.1621254292.git.gitgitgadget@gmail.com","threadId":"55392","inReplyTo":"pull.916.v2.git.1621254292.gitgitgadget@gmail.com","subject":"[PATCH v2 3/4] read-cache: use hashfile instead of git_hash_ctx","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-05-17T12:24:51Z","receivedAt":"2021-05-17T12:25:02Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe do_write_index() method in read-cache.c has its own hashing logic\nand buffering mechanism. Specifically, the ce_write() method was\nintroduced by 4990aadc (Speed up index file writing by chunking it\nnicely, 2005-04-20) and similar mechanisms were introduced a few months\nlater in c38138cd (git-pack-objects: write the pack files with a SHA1\ncsum, 2005-06-26). Based on the timing, in the early days of the Git\ncodebase, I figured that these roughly equivalent code paths were never\nunified only because it got lost in the shuffle. The hashfile API has\nsince been used extensively in other file formats, such as pack-indexes,\nmult-pack-indexes, and commit-graphs. Therefore, it seems prudent to\nunify the index writing code to use the same mechanism.\n\nI discovered this disparity while trying to create a new index format\nthat uses the chunk-format API. That API uses a hashfile as its base, so\nit is incompatible with the custom code in read-cache.c.\n\nThis rewrite is rather straightforward. It replaces all writes to the\ntemporary file with writes to the hashfile struct. This takes care of\nmany of the direct interactions with the_hash_algo.\n\nThere are still some remaining: the extension headers are hashed for use\nin the End of Index Entries (EOIE) extension. This use of the\ngit_hash_ctx is left as-is. There are multiple reasons to not use a\nhashfile here, including the fact that the data is not actually writing\nto a file, just a hash computation. These hashes do not block our\nadoption of the chunk-format API in a future change to the index, so\nleave it as-is.\n\nThe internals of the algorithms are mostly identical. Previously, the\nhashfile API used a smaller 8KB buffer instead of the 128KB buffer from\nread-cache.c. The previous change already unified these sizes.\n\nThere is one subtle point: we do not pass the CSUM_FSYNC to the\nfinalize_hashfile() method, which differs from most consumers of the\nhashfile API. The extra fsync() call indicated by this flag causes a\nsignificant peformance degradation that is noticeable for quick\ncommands that write the index, such as \"git add\". Other consumers can\nabsorb this cost with their more complicated data structure\norganization, and further writing structures such as pack-files and\ncommit-graphs is rarely in the critical path for common user\ninteractions.\n\nSome static methods become orphaned in this diff, so I marked them as\nMAYBE_UNUSED. The diff is much harder to read if they are deleted during\nthis change. Instead, they will be deleted in the following change.\n\nIn addition to the test suite passing, I computed indexes using the\nprevious binaries and the binaries compiled after this change, and found\nthe index data to be exactly equal. Finally, I did extensive performance\ntesting of \"git update-index --force-write\" on repos of various sizes,\nincluding one with over 2 million paths at HEAD. These tests\ndemonstrated less than 1% difference in behavior, so the performance\nshould be considered identical.\n\nSigned-off-by: Derrick Stolee \n---\n read-cache.c | 137 +++++++++++++++++++++++++--------------------------\n 1 file changed, 66 insertions(+), 71 deletions(-)\n\ndiff --git a/read-cache.c b/read-cache.c\nindex fbf3a4ce7d5d..1c0bda81e7e7 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -26,6 +26,7 @@\n #include \"thread-utils.h\"\n #include \"progress.h\"\n #include \"sparse-index.h\"\n+#include \"csum-file.h\"\n \n /* Mask for the name length in ce_flags in the on-disk index */\n \n@@ -2519,6 +2520,7 @@ int repo_index_has_changes(struct repository *repo,\n static unsigned char write_buffer[WRITE_BUFFER_SIZE];\n static unsigned long write_buffer_len;\n \n+MAYBE_UNUSED\n static int ce_write_flush(git_hash_ctx *context, int fd)\n {\n \tunsigned int buffered = write_buffer_len;\n@@ -2531,6 +2533,7 @@ static int ce_write_flush(git_hash_ctx *context, int fd)\n \treturn 0;\n }\n \n+MAYBE_UNUSED\n static int ce_write(git_hash_ctx *context, int fd, void *data, unsigned int len)\n {\n \twhile (len) {\n@@ -2553,19 +2556,24 @@ static int ce_write(git_hash_ctx *context, int fd, void *data, unsigned int len)\n \treturn 0;\n }\n \n-static int write_index_ext_header(git_hash_ctx *context, git_hash_ctx *eoie_context,\n-\t\t\t\t int fd, unsigned int ext, unsigned int sz)\n+static int write_index_ext_header(struct hashfile *f,\n+\t\t\t\t git_hash_ctx *eoie_f,\n+\t\t\t\t unsigned int ext,\n+\t\t\t\t unsigned int sz)\n {\n-\text = htonl(ext);\n-\tsz = htonl(sz);\n-\tif (eoie_context) {\n-\t\tthe_hash_algo->update_fn(eoie_context, &ext, 4);\n-\t\tthe_hash_algo->update_fn(eoie_context, &sz, 4);\n+\thashwrite_be32(f, ext);\n+\thashwrite_be32(f, sz);\n+\n+\tif (eoie_f) {\n+\t\text = htonl(ext);\n+\t\tsz = htonl(sz);\n+\t\tthe_hash_algo->update_fn(eoie_f, &ext, sizeof(ext));\n+\t\tthe_hash_algo->update_fn(eoie_f, &sz, sizeof(sz));\n \t}\n-\treturn ((ce_write(context, fd, &ext, 4) < 0) ||\n-\t\t(ce_write(context, fd, &sz, 4) < 0)) ? -1 : 0;\n+\treturn 0;\n }\n \n+MAYBE_UNUSED\n static int ce_flush(git_hash_ctx *context, int fd, unsigned char *hash)\n {\n \tunsigned int left = write_buffer_len;\n@@ -2667,11 +2675,10 @@ static void copy_cache_entry_to_ondisk(struct ondisk_cache_entry *ondisk,\n \t}\n }\n \n-static int ce_write_entry(git_hash_ctx *c, int fd, struct cache_entry *ce,\n+static int ce_write_entry(struct hashfile *f, struct cache_entry *ce,\n \t\t\t struct strbuf *previous_name, struct ondisk_cache_entry *ondisk)\n {\n \tint size;\n-\tint result;\n \tunsigned int saved_namelen;\n \tint stripped_name = 0;\n \tstatic unsigned char padding[8] = { 0x00 };\n@@ -2687,11 +2694,9 @@ static int ce_write_entry(git_hash_ctx *c, int fd, struct cache_entry *ce,\n \tif (!previous_name) {\n \t\tint len = ce_namelen(ce);\n \t\tcopy_cache_entry_to_ondisk(ondisk, ce);\n-\t\tresult = ce_write(c, fd, ondisk, size);\n-\t\tif (!result)\n-\t\t\tresult = ce_write(c, fd, ce->name, len);\n-\t\tif (!result)\n-\t\t\tresult = ce_write(c, fd, padding, align_padding_size(size, len));\n+\t\thashwrite(f, ondisk, size);\n+\t\thashwrite(f, ce->name, len);\n+\t\thashwrite(f, padding, align_padding_size(size, len));\n \t} else {\n \t\tint common, to_remove, prefix_size;\n \t\tunsigned char to_remove_vi[16];\n@@ -2705,13 +2710,10 @@ static int ce_write_entry(git_hash_ctx *c, int fd, struct cache_entry *ce,\n \t\tprefix_size = encode_varint(to_remove, to_remove_vi);\n \n \t\tcopy_cache_entry_to_ondisk(ondisk, ce);\n-\t\tresult = ce_write(c, fd, ondisk, size);\n-\t\tif (!result)\n-\t\t\tresult = ce_write(c, fd, to_remove_vi, prefix_size);\n-\t\tif (!result)\n-\t\t\tresult = ce_write(c, fd, ce->name + common, ce_namelen(ce) - common);\n-\t\tif (!result)\n-\t\t\tresult = ce_write(c, fd, padding, 1);\n+\t\thashwrite(f, ondisk, size);\n+\t\thashwrite(f, to_remove_vi, prefix_size);\n+\t\thashwrite(f, ce->name + common, ce_namelen(ce) - common);\n+\t\thashwrite(f, padding, 1);\n \n \t\tstrbuf_splice(previous_name, common, to_remove,\n \t\t\t ce->name + common, ce_namelen(ce) - common);\n@@ -2721,7 +2723,7 @@ static int ce_write_entry(git_hash_ctx *c, int fd, struct cache_entry *ce,\n \t\tce->ce_flags &= ~CE_STRIP_NAME;\n \t}\n \n-\treturn result;\n+\treturn 0;\n }\n \n /*\n@@ -2833,8 +2835,8 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,\n \t\t\t int strip_extensions)\n {\n \tuint64_t start = getnanotime();\n-\tint newfd = tempfile->fd;\n-\tgit_hash_ctx c, eoie_c;\n+\tstruct hashfile *f;\n+\tgit_hash_ctx *eoie_c = NULL;\n \tstruct cache_header hdr;\n \tint i, err = 0, removed, extended, hdr_version;\n \tstruct cache_entry **cache = istate->cache;\n@@ -2848,6 +2850,8 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,\n \tstruct index_entry_offset_table *ieot = NULL;\n \tint nr, nr_threads;\n \n+\tf = hashfd(tempfile->fd, tempfile->filename.buf);\n+\n \tfor (i = removed = extended = 0; i < entries; i++) {\n \t\tif (cache[i]->ce_flags & CE_REMOVE)\n \t\t\tremoved++;\n@@ -2876,9 +2880,7 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,\n \thdr.hdr_version = htonl(hdr_version);\n \thdr.hdr_entries = htonl(entries - removed);\n \n-\tthe_hash_algo->init_fn(&c);\n-\tif (ce_write(&c, newfd, &hdr, sizeof(hdr)) < 0)\n-\t\treturn -1;\n+\thashwrite(f, &hdr, sizeof(hdr));\n \n \tif (!HAVE_THREADS || git_config_get_index_threads(&nr_threads))\n \t\tnr_threads = 1;\n@@ -2913,12 +2915,8 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,\n \t\t}\n \t}\n \n-\toffset = lseek(newfd, 0, SEEK_CUR);\n-\tif (offset < 0) {\n-\t\tfree(ieot);\n-\t\treturn -1;\n-\t}\n-\toffset += write_buffer_len;\n+\toffset = hashfile_total(f);\n+\n \tnr = 0;\n \tprevious_name = (hdr_version == 4) ? &previous_name_buf : NULL;\n \n@@ -2953,14 +2951,10 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,\n \t\t\tif (previous_name)\n \t\t\t\tprevious_name->buf[0] = 0;\n \t\t\tnr = 0;\n-\t\t\toffset = lseek(newfd, 0, SEEK_CUR);\n-\t\t\tif (offset < 0) {\n-\t\t\t\tfree(ieot);\n-\t\t\t\treturn -1;\n-\t\t\t}\n-\t\t\toffset += write_buffer_len;\n+\n+\t\t\toffset = hashfile_total(f);\n \t\t}\n-\t\tif (ce_write_entry(&c, newfd, ce, previous_name, (struct ondisk_cache_entry *)&ondisk) < 0)\n+\t\tif (ce_write_entry(f, ce, previous_name, (struct ondisk_cache_entry *)&ondisk) < 0)\n \t\t\terr = -1;\n \n \t\tif (err)\n@@ -2979,14 +2973,16 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,\n \t\treturn err;\n \t}\n \n-\t/* Write extension data here */\n-\toffset = lseek(newfd, 0, SEEK_CUR);\n-\tif (offset < 0) {\n-\t\tfree(ieot);\n-\t\treturn -1;\n+\toffset = hashfile_total(f);\n+\n+\t/*\n+\t * The extension headers must be hashed on their own for the\n+\t * EOIE extension. Create a hashfile here to compute that hash.\n+\t */\n+\tif (offset && record_eoie()) {\n+\t\tCALLOC_ARRAY(eoie_c, 1);\n+\t\tthe_hash_algo->init_fn(eoie_c);\n \t}\n-\toffset += write_buffer_len;\n-\tthe_hash_algo->init_fn(&eoie_c);\n \n \t/*\n \t * Lets write out CACHE_EXT_INDEXENTRYOFFSETTABLE first so that we\n@@ -2999,8 +2995,8 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,\n \t\tstruct strbuf sb = STRBUF_INIT;\n \n \t\twrite_ieot_extension(&sb, ieot);\n-\t\terr = write_index_ext_header(&c, &eoie_c, newfd, CACHE_EXT_INDEXENTRYOFFSETTABLE, sb.len) < 0\n-\t\t\t|| ce_write(&c, newfd, sb.buf, sb.len) < 0;\n+\t\terr = write_index_ext_header(f, eoie_c, CACHE_EXT_INDEXENTRYOFFSETTABLE, sb.len) < 0;\n+\t\thashwrite(f, sb.buf, sb.len);\n \t\tstrbuf_release(&sb);\n \t\tfree(ieot);\n \t\tif (err)\n@@ -3012,9 +3008,9 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,\n \t\tstruct strbuf sb = STRBUF_INIT;\n \n \t\terr = write_link_extension(&sb, istate) < 0 ||\n-\t\t\twrite_index_ext_header(&c, &eoie_c, newfd, CACHE_EXT_LINK,\n-\t\t\t\t\t sb.len) < 0 ||\n-\t\t\tce_write(&c, newfd, sb.buf, sb.len) < 0;\n+\t\t\twrite_index_ext_header(f, eoie_c, CACHE_EXT_LINK,\n+\t\t\t\t\t sb.len) < 0;\n+\t\thashwrite(f, sb.buf, sb.len);\n \t\tstrbuf_release(&sb);\n \t\tif (err)\n \t\t\treturn -1;\n@@ -3023,8 +3019,8 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,\n \t\tstruct strbuf sb = STRBUF_INIT;\n \n \t\tcache_tree_write(&sb, istate->cache_tree);\n-\t\terr = write_index_ext_header(&c, &eoie_c, newfd, CACHE_EXT_TREE, sb.len) < 0\n-\t\t\t|| ce_write(&c, newfd, sb.buf, sb.len) < 0;\n+\t\terr = write_index_ext_header(f, eoie_c, CACHE_EXT_TREE, sb.len) < 0;\n+\t\thashwrite(f, sb.buf, sb.len);\n \t\tstrbuf_release(&sb);\n \t\tif (err)\n \t\t\treturn -1;\n@@ -3033,9 +3029,9 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,\n \t\tstruct strbuf sb = STRBUF_INIT;\n \n \t\tresolve_undo_write(&sb, istate->resolve_undo);\n-\t\terr = write_index_ext_header(&c, &eoie_c, newfd, CACHE_EXT_RESOLVE_UNDO,\n-\t\t\t\t\t sb.len) < 0\n-\t\t\t|| ce_write(&c, newfd, sb.buf, sb.len) < 0;\n+\t\terr = write_index_ext_header(f, eoie_c, CACHE_EXT_RESOLVE_UNDO,\n+\t\t\t\t\t sb.len) < 0;\n+\t\thashwrite(f, sb.buf, sb.len);\n \t\tstrbuf_release(&sb);\n \t\tif (err)\n \t\t\treturn -1;\n@@ -3044,9 +3040,9 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,\n \t\tstruct strbuf sb = STRBUF_INIT;\n \n \t\twrite_untracked_extension(&sb, istate->untracked);\n-\t\terr = write_index_ext_header(&c, &eoie_c, newfd, CACHE_EXT_UNTRACKED,\n-\t\t\t\t\t sb.len) < 0 ||\n-\t\t\tce_write(&c, newfd, sb.buf, sb.len) < 0;\n+\t\terr = write_index_ext_header(f, eoie_c, CACHE_EXT_UNTRACKED,\n+\t\t\t\t\t sb.len) < 0;\n+\t\thashwrite(f, sb.buf, sb.len);\n \t\tstrbuf_release(&sb);\n \t\tif (err)\n \t\t\treturn -1;\n@@ -3055,14 +3051,14 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,\n \t\tstruct strbuf sb = STRBUF_INIT;\n \n \t\twrite_fsmonitor_extension(&sb, istate);\n-\t\terr = write_index_ext_header(&c, &eoie_c, newfd, CACHE_EXT_FSMONITOR, sb.len) < 0\n-\t\t\t|| ce_write(&c, newfd, sb.buf, sb.len) < 0;\n+\t\terr = write_index_ext_header(f, eoie_c, CACHE_EXT_FSMONITOR, sb.len) < 0;\n+\t\thashwrite(f, sb.buf, sb.len);\n \t\tstrbuf_release(&sb);\n \t\tif (err)\n \t\t\treturn -1;\n \t}\n \tif (istate->sparse_index) {\n-\t\tif (write_index_ext_header(&c, &eoie_c, newfd, CACHE_EXT_SPARSE_DIRECTORIES, 0) < 0)\n+\t\tif (write_index_ext_header(f, eoie_c, CACHE_EXT_SPARSE_DIRECTORIES, 0) < 0)\n \t\t\treturn -1;\n \t}\n \n@@ -3072,19 +3068,18 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,\n \t * read. Write it out regardless of the strip_extensions parameter as we need it\n \t * when loading the shared index.\n \t */\n-\tif (offset && record_eoie()) {\n+\tif (eoie_c) {\n \t\tstruct strbuf sb = STRBUF_INIT;\n \n-\t\twrite_eoie_extension(&sb, &eoie_c, offset);\n-\t\terr = write_index_ext_header(&c, NULL, newfd, CACHE_EXT_ENDOFINDEXENTRIES, sb.len) < 0\n-\t\t\t|| ce_write(&c, newfd, sb.buf, sb.len) < 0;\n+\t\twrite_eoie_extension(&sb, eoie_c, offset);\n+\t\terr = write_index_ext_header(f, NULL, CACHE_EXT_ENDOFINDEXENTRIES, sb.len) < 0;\n+\t\thashwrite(f, sb.buf, sb.len);\n \t\tstrbuf_release(&sb);\n \t\tif (err)\n \t\t\treturn -1;\n \t}\n \n-\tif (ce_flush(&c, newfd, istate->oid.hash))\n-\t\treturn -1;\n+\tfinalize_hashfile(f, istate->oid.hash, CSUM_HASH_IN_STREAM);\n \tif (close_tempfile_gently(tempfile)) {\n \t\terror(_(\"could not close '%s'\"), get_tempfile_path(tempfile));\n \t\treturn -1;\n-- \ngitgitgadget\n\n"},{"id":"424731","messageId":"9dc602f6c4221e2259778842ec3d1eda57508333.1621254292.git.gitgitgadget@gmail.com","threadId":"55392","inReplyTo":"pull.916.v2.git.1621254292.gitgitgadget@gmail.com","subject":"[PATCH v2 2/4] csum-file.h: increase hashfile buffer size","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-05-17T12:24:50Z","receivedAt":"2021-05-17T12:25:03Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe hashfile API uses a hard-coded buffer size of 8KB and has ever since\nit was introduced in c38138c (git-pack-objects: write the pack files\nwith a SHA1 csum, 2005-06-26). It performs a similar function to the\nhashing buffers in read-cache.c, but that code was updated from 8KB to\n128KB in f279894 (read-cache: make the index write buffer size 128K,\n2021-02-18). The justification there was that do_write_index() improves\nfrom 1.02s to 0.72s.\n\nThere is a buffer, check_buffer, that is used to verify the check_fd\nfile descriptor. When this buffer increases to 128K to fit the data\nbeing flushed, it causes the stack to overflow the limits placed in the\ntest suite. By moving this to a static buffer, we stop using stack data\nfor this purpose, but we lose some thread-safety. This change makes it\nunsafe to write to multiple hashfiles across different threads.\n\nBy adding a new trace2 region in the chunk-format API, we can see that\nthe writing portion of 'git multi-pack-index write' lowers from ~1.49s\nto ~1.47s on a Linux machine. These effects may be more pronounced or\ndiminished on other filesystems. The end-to-end timing is too noisy to\nhave a definitive change either way.\n\nSigned-off-by: Derrick Stolee \n---\n chunk-format.c | 12 ++++++++----\n csum-file.c | 28 +++++++++++++++++-----------\n csum-file.h | 4 +++-\n 3 files changed, 28 insertions(+), 16 deletions(-)\n\ndiff --git a/chunk-format.c b/chunk-format.c\nindex da191e59a29d..1c3dca62e205 100644\n--- a/chunk-format.c\n+++ b/chunk-format.c\n@@ -58,9 +58,11 @@ void add_chunk(struct chunkfile *cf,\n \n int write_chunkfile(struct chunkfile *cf, void *data)\n {\n-\tint i;\n+\tint i, result = 0;\n \tuint64_t cur_offset = hashfile_total(cf->f);\n \n+\ttrace2_region_enter(\"chunkfile\", \"write\", the_repository);\n+\n \t/* Add the table of contents to the current offset */\n \tcur_offset += (cf->chunks_nr + 1) * CHUNK_TOC_ENTRY_SIZE;\n \n@@ -77,10 +79,10 @@ int write_chunkfile(struct chunkfile *cf, void *data)\n \n \tfor (i = 0; i < cf->chunks_nr; i++) {\n \t\toff_t start_offset = hashfile_total(cf->f);\n-\t\tint result = cf->chunks[i].write_fn(cf->f, data);\n+\t\tresult = cf->chunks[i].write_fn(cf->f, data);\n \n \t\tif (result)\n-\t\t\treturn result;\n+\t\t\tgoto cleanup;\n \n \t\tif (hashfile_total(cf->f) - start_offset != cf->chunks[i].size)\n \t\t\tBUG(\"expected to write %\"PRId64\" bytes to chunk %\"PRIx32\", but wrote %\"PRId64\" instead\",\n@@ -88,7 +90,9 @@ int write_chunkfile(struct chunkfile *cf, void *data)\n \t\t\t hashfile_total(cf->f) - start_offset);\n \t}\n \n-\treturn 0;\n+cleanup:\n+\ttrace2_region_leave(\"chunkfile\", \"write\", the_repository);\n+\treturn result;\n }\n \n int read_table_of_contents(struct chunkfile *cf,\ndiff --git a/csum-file.c b/csum-file.c\nindex 3c26389d4914..bd9939c49efa 100644\n--- a/csum-file.c\n+++ b/csum-file.c\n@@ -11,19 +11,25 @@\n #include \"progress.h\"\n #include \"csum-file.h\"\n \n+static void verify_buffer_or_die(struct hashfile *f,\n+\t\t\t\t const void *buf,\n+\t\t\t\t unsigned int count)\n+{\n+\tstatic unsigned char check_buffer[WRITE_BUFFER_SIZE];\n+\tssize_t ret = read_in_full(f->check_fd, check_buffer, count);\n+\n+\tif (ret < 0)\n+\t\tdie_errno(\"%s: sha1 file read error\", f->name);\n+\tif (ret != count)\n+\t\tdie(\"%s: sha1 file truncated\", f->name);\n+\tif (memcmp(buf, check_buffer, count))\n+\t\tdie(\"sha1 file '%s' validation error\", f->name);\n+}\n+\n static void flush(struct hashfile *f, const void *buf, unsigned int count)\n {\n-\tif (0 <= f->check_fd && count) {\n-\t\tunsigned char check_buffer[8192];\n-\t\tssize_t ret = read_in_full(f->check_fd, check_buffer, count);\n-\n-\t\tif (ret < 0)\n-\t\t\tdie_errno(\"%s: sha1 file read error\", f->name);\n-\t\tif (ret != count)\n-\t\t\tdie(\"%s: sha1 file truncated\", f->name);\n-\t\tif (memcmp(buf, check_buffer, count))\n-\t\t\tdie(\"sha1 file '%s' validation error\", f->name);\n-\t}\n+\tif (0 <= f->check_fd && count)\n+\t\tverify_buffer_or_die(f, buf, count);\n \n \tif (write_in_full(f->fd, buf, count) < 0) {\n \t\tif (errno == ENOSPC)\ndiff --git a/csum-file.h b/csum-file.h\nindex e54d53d1d0b3..bc88eb86fc28 100644\n--- a/csum-file.h\n+++ b/csum-file.h\n@@ -5,6 +5,8 @@\n \n struct progress;\n \n+#define WRITE_BUFFER_SIZE (128 * 1024)\n+\n /* A SHA1-protected file */\n struct hashfile {\n \tint fd;\n@@ -16,7 +18,7 @@ struct hashfile {\n \tconst char *name;\n \tint do_crc;\n \tuint32_t crc32;\n-\tunsigned char buffer[8192];\n+\tunsigned char buffer[WRITE_BUFFER_SIZE];\n };\n \n /* Checkpoint */\n-- \ngitgitgadget\n\n"},{"id":"424732","messageId":"4b3814eb4c80617d3b180dd576348a3b2f26b35e.1621254292.git.gitgitgadget@gmail.com","threadId":"55392","inReplyTo":"pull.916.v2.git.1621254292.gitgitgadget@gmail.com","subject":"[PATCH v2 4/4] read-cache: delete unused hashing methods","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-05-17T12:24:52Z","receivedAt":"2021-05-17T12:25:05Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThese methods were marked as MAYBE_UNUSED in the previous change to\navoid a complicated diff. Delete them entirely, since we now use the\nhashfile API instead of this custom hashing code.\n\nSigned-off-by: Derrick Stolee \n---\n read-cache.c | 64 ----------------------------------------------------\n 1 file changed, 64 deletions(-)\n\ndiff --git a/read-cache.c b/read-cache.c\nindex 1c0bda81e7e7..aa6751c6a092 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -2516,46 +2516,6 @@ int repo_index_has_changes(struct repository *repo,\n \t}\n }\n \n-#define WRITE_BUFFER_SIZE (128 * 1024)\n-static unsigned char write_buffer[WRITE_BUFFER_SIZE];\n-static unsigned long write_buffer_len;\n-\n-MAYBE_UNUSED\n-static int ce_write_flush(git_hash_ctx *context, int fd)\n-{\n-\tunsigned int buffered = write_buffer_len;\n-\tif (buffered) {\n-\t\tthe_hash_algo->update_fn(context, write_buffer, buffered);\n-\t\tif (write_in_full(fd, write_buffer, buffered) < 0)\n-\t\t\treturn -1;\n-\t\twrite_buffer_len = 0;\n-\t}\n-\treturn 0;\n-}\n-\n-MAYBE_UNUSED\n-static int ce_write(git_hash_ctx *context, int fd, void *data, unsigned int len)\n-{\n-\twhile (len) {\n-\t\tunsigned int buffered = write_buffer_len;\n-\t\tunsigned int partial = WRITE_BUFFER_SIZE - buffered;\n-\t\tif (partial > len)\n-\t\t\tpartial = len;\n-\t\tmemcpy(write_buffer + buffered, data, partial);\n-\t\tbuffered += partial;\n-\t\tif (buffered == WRITE_BUFFER_SIZE) {\n-\t\t\twrite_buffer_len = buffered;\n-\t\t\tif (ce_write_flush(context, fd))\n-\t\t\t\treturn -1;\n-\t\t\tbuffered = 0;\n-\t\t}\n-\t\twrite_buffer_len = buffered;\n-\t\tlen -= partial;\n-\t\tdata = (char *) data + partial;\n-\t}\n-\treturn 0;\n-}\n-\n static int write_index_ext_header(struct hashfile *f,\n \t\t\t\t git_hash_ctx *eoie_f,\n \t\t\t\t unsigned int ext,\n@@ -2573,30 +2533,6 @@ static int write_index_ext_header(struct hashfile *f,\n \treturn 0;\n }\n \n-MAYBE_UNUSED\n-static int ce_flush(git_hash_ctx *context, int fd, unsigned char *hash)\n-{\n-\tunsigned int left = write_buffer_len;\n-\n-\tif (left) {\n-\t\twrite_buffer_len = 0;\n-\t\tthe_hash_algo->update_fn(context, write_buffer, left);\n-\t}\n-\n-\t/* Flush first if not enough space for hash signature */\n-\tif (left + the_hash_algo->rawsz > WRITE_BUFFER_SIZE) {\n-\t\tif (write_in_full(fd, write_buffer, left) < 0)\n-\t\t\treturn -1;\n-\t\tleft = 0;\n-\t}\n-\n-\t/* Append the hash signature at the end */\n-\tthe_hash_algo->final_fn(write_buffer + left, context);\n-\thashcpy(hash, write_buffer + left);\n-\tleft += the_hash_algo->rawsz;\n-\treturn (write_in_full(fd, write_buffer, left) < 0) ? -1 : 0;\n-}\n-\n static void ce_smudge_racily_clean_entry(struct index_state *istate,\n \t\t\t\t\t struct cache_entry *ce)\n {\n-- \ngitgitgadget\n"},{"id":"424789","messageId":"xmqqk0nx58lq.fsf@gitster.g","threadId":"55392","inReplyTo":"9dc602f6c4221e2259778842ec3d1eda57508333.1621254292.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 2/4] csum-file.h: increase hashfile buffer size","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-05-17T21:54:57Z","receivedAt":"2021-05-17T21:55:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Derrick Stolee via GitGitGadget\" writes:\n\n> From: Derrick Stolee \n>\n> The hashfile API uses a hard-coded buffer size of 8KB and has ever since\n> it was introduced in c38138c (git-pack-objects: write the pack files\n> with a SHA1 csum, 2005-06-26). It performs a similar function to the\n> hashing buffers in read-cache.c, but that code was updated from 8KB to\n> 128KB in f279894 (read-cache: make the index write buffer size 128K,\n> 2021-02-18). The justification there was that do_write_index() improves\n> from 1.02s to 0.72s.\n>\n> There is a buffer, check_buffer, that is used to verify the check_fd\n> file descriptor. When this buffer increases to 128K to fit the data\n> being flushed, it causes the stack to overflow the limits placed in the\n> test suite. By moving this to a static buffer, we stop using stack data\n> for this purpose, but we lose some thread-safety. This change makes it\n> unsafe to write to multiple hashfiles across different threads.\n\nSo... is the longer term plan to heap-allocate the buffer itself,\nand replace the .buffer member of hashfile from an embedded char\narray to a pointer? The hashfile structure is initialized once per\nthe entire file (like the on-disk index), and the same buffer will\nbe used throughout the life of that hashfile instance, so it may not\nbe too bad to allocate on the heap.\n\nJust after the previous step justified its simplification of its\nprogress logic based on how small the buffer is, this step makes it\n16 times as big, which felt a tiny bit dishonest. We probably\nshould say somewhere that 128k is still small enough that the\nrewrite in the previous step is still valid ;-)\n\n"},{"id":"424790","messageId":"xmqqfsyl57q2.fsf@gitster.g","threadId":"55392","inReplyTo":"b94172ccf5e91fe59a1d32774dbec23e624f1135.1621254292.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 3/4] read-cache: use hashfile instead of git_hash_ctx","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-05-17T22:13:57Z","receivedAt":"2021-05-17T22:14:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Derrick Stolee via GitGitGadget\" writes:\n\n> ...\n> mult-pack-indexes, and commit-graphs. Therefore, it seems prudent to\n\nmulti-pack, I would say.\n\n> There are still some remaining: the extension headers are hashed for use\n\nsome remaining what? I first read an unwritten word as \"issues\",\nbut I think the answer is \"uses of git_hash_ctx\".\n\n> in the End of Index Entries (EOIE) extension. This use of the\n> git_hash_ctx is left as-is. There are multiple reasons to not use a\n> hashfile here, including ...\n\n> In addition to the test suite passing, I computed indexes using the\n> previous binaries and the binaries compiled after this change, and found\n> the index data to be exactly equal. Finally, I did extensive performance\n> testing of \"git update-index --force-write\" on repos of various sizes,\n> including one with over 2 million paths at HEAD. These tests\n> demonstrated less than 1% difference in behavior, so the performance\n> should be considered identical.\n\nHmph, does that mean 128k buffer is overkill and if we wanted to\nunify the buffer sizes we should have used 8k instead?\n\nWait, the removal of fsync has made things faster in general, hasn't\nit? Did something else degrade performance to cancel that gain?\n\nThe patch looks an obvious improvement. What was open-coded in\nlonghand is now a well structured series of API calls and the result\nis much easier to follow and maintain.\n\nThanks.\n"},{"id":"424838","messageId":"YKNtUKIHoCCOMYmn@coredump.intra.peff.net","threadId":"55392","inReplyTo":"9dc602f6c4221e2259778842ec3d1eda57508333.1621254292.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 2/4] csum-file.h: increase hashfile buffer size","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-05-18T07:31:28Z","receivedAt":"2021-05-18T07:31:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 17, 2021 at 12:24:50PM +0000, Derrick Stolee via GitGitGadget wrote:\n\n> From: Derrick Stolee \n> \n> The hashfile API uses a hard-coded buffer size of 8KB and has ever since\n> it was introduced in c38138c (git-pack-objects: write the pack files\n> with a SHA1 csum, 2005-06-26). It performs a similar function to the\n> hashing buffers in read-cache.c, but that code was updated from 8KB to\n> 128KB in f279894 (read-cache: make the index write buffer size 128K,\n> 2021-02-18). The justification there was that do_write_index() improves\n> from 1.02s to 0.72s.\n> \n> There is a buffer, check_buffer, that is used to verify the check_fd\n> file descriptor. When this buffer increases to 128K to fit the data\n> being flushed, it causes the stack to overflow the limits placed in the\n> test suite. By moving this to a static buffer, we stop using stack data\n> for this purpose, but we lose some thread-safety. This change makes it\n> unsafe to write to multiple hashfiles across different threads.\n> \n> By adding a new trace2 region in the chunk-format API, we can see that\n> the writing portion of 'git multi-pack-index write' lowers from ~1.49s\n> to ~1.47s on a Linux machine. These effects may be more pronounced or\n> diminished on other filesystems. The end-to-end timing is too noisy to\n> have a definitive change either way.\n\nI think there is one thing missing from this commit message: why we want\nto do this. You mentioned that read-cache got larger by using a bigger\nbuffer. But here we use a bigger buffer, and it produces no improvement\nlarger than the noise. And on top of it, you describe the static-buffer\ndownsides. So why not just skip it? :)\n\nAnd the answer is in the larger series: we want to be able to make use\nof the hashfile API in read-cache, but without regressing the\nperformance. One sentence at the end of the first paragraph would\nclarify that quite a bit, I think.\n\n> +static void verify_buffer_or_die(struct hashfile *f,\n> +\t\t\t\t const void *buf,\n> +\t\t\t\t unsigned int count)\n> +{\n> +\tstatic unsigned char check_buffer[WRITE_BUFFER_SIZE];\n> +\tssize_t ret = read_in_full(f->check_fd, check_buffer, count);\n> +\n> +\tif (ret < 0)\n> +\t\tdie_errno(\"%s: sha1 file read error\", f->name);\n> +\tif (ret != count)\n> +\t\tdie(\"%s: sha1 file truncated\", f->name);\n> +\tif (memcmp(buf, check_buffer, count))\n> +\t\tdie(\"sha1 file '%s' validation error\", f->name);\n> +}\n\nDoes this have to use the same-size buffer? We could read and check\nsmaller chunks, like:\n\n while (count > 0) {\n\tstatic unsigned char chunk[1024];\n\tunsigned int chunk_len = sizeof(chunk) < count ? sizeof(chunk) : count;\n\tssize_t ret = read_in_full(f->check_fd, chunk, chunk_len);\n\n if (ret < 0)\n\t ...\n\tif (ret != count)\n\t ...\n\tif (memcmp(buf, chunk, chunk_len))\n\t ...\n\tbuf += chunk_len;\n\tcount -= chunk_len;\n }\n\nWe may prefer to use the larger buffer size for performance, but I think\nthis \"check\" mode is only used for \"index-pack --verify\" and similar.\nThe performance may matter a lot less to us there than for more\nfrequently used code paths like index writing.\n\nI don't have a strong preference either way, but it's nice to avoid\nintroducing non-reentrancy to a function (Junio's heap suggestion is\nalso quite reasonable).\n\n-Peff\n"},{"id":"424839","messageId":"YKNtxUtDXoWwaVrc@coredump.intra.peff.net","threadId":"55392","inReplyTo":"xmqqk0nx58lq.fsf@gitster.g","subject":"Re: [PATCH v2 2/4] csum-file.h: increase hashfile buffer size","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-05-18T07:33:25Z","receivedAt":"2021-05-18T07:33:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 18, 2021 at 06:54:57AM +0900, Junio C Hamano wrote:\n\n> Just after the previous step justified its simplification of its\n> progress logic based on how small the buffer is, this step makes it\n> 16 times as big, which felt a tiny bit dishonest. We probably\n> should say somewhere that 128k is still small enough that the\n> rewrite in the previous step is still valid ;-)\n\nI noticed that, too. I'm not sure if still is small enough. For local\npack writes, etc, it seems fine. But what about \"index-pack --stdin\"\nreading over the network?\n\nUpdating progress every 8k instead of every 128k seems like it would be\nmore responsive, especially if the network is slow or jittery. I dunno.\nMaybe that is too small to care about for the modern world, but I just\nwant to make sure we are not being blinded by the fast networks all of\nus presumably enjoy. :)\n\n-Peff\n"},{"id":"424840","messageId":"YKNv0zeobEFgHsad@coredump.intra.peff.net","threadId":"55392","inReplyTo":"YKNtUKIHoCCOMYmn@coredump.intra.peff.net","subject":"Re: [PATCH v2 2/4] csum-file.h: increase hashfile buffer size","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-05-18T07:42:11Z","receivedAt":"2021-05-18T07:42:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 18, 2021 at 03:31:28AM -0400, Jeff King wrote:\n\n> Does this have to use the same-size buffer? We could read and check\n> smaller chunks, like:\n> \n> while (count > 0) {\n> \tstatic unsigned char chunk[1024];\n> \tunsigned int chunk_len = sizeof(chunk) < count ? sizeof(chunk) : count;\n> \tssize_t ret = read_in_full(f->check_fd, chunk, chunk_len);\n> \n> \tif (ret < 0)\n> \t ...\n> \tif (ret != count)\n> \t ...\n> \tif (memcmp(buf, chunk, chunk_len))\n> \t ...\n> \tbuf += chunk_len;\n> \tcount -= chunk_len;\n> }\n\nThat should be \"ret != chunk_len\" in the middle conditional, of course.\nIn case you do go this route (I typed this straight into my email, so\nother bugs may be lurking. But I noticed that one. :) ).\n\n-Peff\n"},{"id":"424876","messageId":"1641be46-c8d7-08ae-ebe0-7f3eb3589b27@gmail.com","threadId":"55392","inReplyTo":"xmqqfsyl57q2.fsf@gitster.g","subject":"Re: [PATCH v2 3/4] read-cache: use hashfile instead of git_hash_ctx","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-05-18T14:16:22Z","receivedAt":"2021-05-18T14:16:27Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 5/17/2021 6:13 PM, Junio C Hamano wrote:\n> \"Derrick Stolee via GitGitGadget\" writes:\n> \n>> ...\n>> mult-pack-indexes, and commit-graphs. Therefore, it seems prudent to\n> \n> multi-pack, I would say.\n> \n>> There are still some remaining: the extension headers are hashed for use\n> \n> some remaining what? I first read an unwritten word as \"issues\",\n> but I think the answer is \"uses of git_hash_ctx\".\n\nThanks for pointing these out. I will fix them.\n\n>> in the End of Index Entries (EOIE) extension. This use of the\n>> git_hash_ctx is left as-is. There are multiple reasons to not use a\n>> hashfile here, including ...\n> \n>> In addition to the test suite passing, I computed indexes using the\n>> previous binaries and the binaries compiled after this change, and found\n>> the index data to be exactly equal. Finally, I did extensive performance\n>> testing of \"git update-index --force-write\" on repos of various sizes,\n>> including one with over 2 million paths at HEAD. These tests\n>> demonstrated less than 1% difference in behavior, so the performance\n>> should be considered identical.\n> \n> Hmph, does that mean 128k buffer is overkill and if we wanted to\n> unify the buffer sizes we should have used 8k instead?\n\nThe buffer was previously increased to 128k because it makes a\ndifference in performance when writing the index.\n\nThe thing I'm measuring here is the difference between the old\nwriting code and the new hashfile code. Using the hashfile API\n(with an identical buffer size) does not have a meaningful\nperformance impact, as it should.\n\nI can make this clearer.\n\n> Wait, the removal of fsync has made things faster in general, hasn't\n> it? Did something else degrade performance to cancel that gain?\n\nAre you thinking about [1], which originally was talking about a\nchange to fsync() calls, but really ended up just making the same\nbehavior more readable?\n\n[1] https://lore.kernel.org/git/pull.914.v2.git.1616762291574.gitgitgadget@gmail.com/\n\nI was focused on that because I had initially seen a performance\ndegradation when I did this refactor. It turns out that my measurements\nwere not robust enough to the noise, which has been remedied.\n\n> The patch looks an obvious improvement. What was open-coded in\n> longhand is now a well structured series of API calls and the result\n> is much easier to follow and maintain.\n\nThat is the goal. I'm glad you agree.\n\nThanks,\n-Stolee\n"},{"id":"424881","messageId":"6baf7f30-f183-b827-9495-96502ddf1756@gmail.com","threadId":"55392","inReplyTo":"YKNtxUtDXoWwaVrc@coredump.intra.peff.net","subject":"Re: [PATCH v2 2/4] csum-file.h: increase hashfile buffer size","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-05-18T14:44:17Z","receivedAt":"2021-05-18T14:44:45Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 5/18/2021 3:33 AM, Jeff King wrote:\n> On Tue, May 18, 2021 at 06:54:57AM +0900, Junio C Hamano wrote:\n> \n>> Just after the previous step justified its simplification of its\n>> progress logic based on how small the buffer is, this step makes it\n>> 16 times as big, which felt a tiny bit dishonest. We probably\n>> should say somewhere that 128k is still small enough that the\n>> rewrite in the previous step is still valid ;-)\n> \n> I noticed that, too. I'm not sure if still is small enough. For local\n> pack writes, etc, it seems fine. But what about \"index-pack --stdin\"\n> reading over the network?\n> \n> Updating progress every 8k instead of every 128k seems like it would be\n> more responsive, especially if the network is slow or jittery. I dunno.\n> Maybe that is too small to care about for the modern world, but I just\n> want to make sure we are not being blinded by the fast networks all of\n> us presumably enjoy. :)\n\nThis is a good point.\n\nIf we combine the earlier suggestion of using the heap to store the\nbuffer, then we can change the buffer size based on the use case: we\ncan use 8k for data that might be streaming from a network, and 128k\nfor local-only data (index, commit-graph, multi-pack-index are all\nprobably safe).\n\nFor the case of NFS, I think we should probably assume that the NFS\nserver is not across a slow connection, which is a case we cannot\nmake for streaming a pack-file from a remote server.\n\nThanks,\n-Stolee\n"},{"id":"424893","messageId":"64ffddd791160895b8e6730ebcddfac8458653f2.1621362768.git.gitgitgadget@gmail.com","threadId":"55392","inReplyTo":"pull.916.v3.git.1621362768.gitgitgadget@gmail.com","subject":"[PATCH v3 2/4] csum-file.h: increase hashfile buffer size","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-05-18T18:32:46Z","receivedAt":"2021-05-18T18:33:01Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe hashfile API uses a hard-coded buffer size of 8KB and has ever since\nit was introduced in c38138c (git-pack-objects: write the pack files\nwith a SHA1 csum, 2005-06-26). It performs a similar function to the\nhashing buffers in read-cache.c, but that code was updated from 8KB to\n128KB in f279894 (read-cache: make the index write buffer size 128K,\n2021-02-18). The justification there was that do_write_index() improves\nfrom 1.02s to 0.72s. Since our end goal is to have the index writing\ncode use the hashfile API, we need to unify this buffer size to avoid a\nperformance regression.\n\nThere is a buffer, 'check_buffer', that is used to verify the check_fd\nfile descriptor. When this buffer increases to 128K to fit the data\nbeing flushed, it causes the stack to overflow the limits placed in the\ntest suite. To avoid issues with stack size, move both 'buffer' and\n'check_buffer' to be heap pointers within 'struct hashfile'. The\n'check_buffer' member is left as NULL unless check_fd is set in\nhashfd_check(). Both buffers are cleared as part of finalize_hashfile()\nwhich also frees the full structure.\n\nSince these buffers are now on the heap, we can adjust their size based\non the needs of the consumer. In particular, callers to\nhashfd_throughput() are expecting to report progress indicators as the\nbuffer flushes. These callers would prefer the smaller 8k buffer to\navoid large delays between updates, especially for users with slower\nnetworks. When the progress indicator is not used, the larger buffer is\npreferrable.\n\nBy adding a new trace2 region in the chunk-format API, we can see that\nthe writing portion of 'git multi-pack-index write' lowers from ~1.49s\nto ~1.47s on a Linux machine. These effects may be more pronounced or\ndiminished on other filesystems. The end-to-end timing is too noisy to\nhave a definitive change either way.\n\nSigned-off-by: Derrick Stolee \n---\n chunk-format.c | 12 +++++---\n csum-file.c | 77 +++++++++++++++++++++++++++++++++++++-------------\n csum-file.h | 4 ++-\n 3 files changed, 68 insertions(+), 25 deletions(-)\n\ndiff --git a/chunk-format.c b/chunk-format.c\nindex da191e59a29d..1c3dca62e205 100644\n--- a/chunk-format.c\n+++ b/chunk-format.c\n@@ -58,9 +58,11 @@ void add_chunk(struct chunkfile *cf,\n \n int write_chunkfile(struct chunkfile *cf, void *data)\n {\n-\tint i;\n+\tint i, result = 0;\n \tuint64_t cur_offset = hashfile_total(cf->f);\n \n+\ttrace2_region_enter(\"chunkfile\", \"write\", the_repository);\n+\n \t/* Add the table of contents to the current offset */\n \tcur_offset += (cf->chunks_nr + 1) * CHUNK_TOC_ENTRY_SIZE;\n \n@@ -77,10 +79,10 @@ int write_chunkfile(struct chunkfile *cf, void *data)\n \n \tfor (i = 0; i < cf->chunks_nr; i++) {\n \t\toff_t start_offset = hashfile_total(cf->f);\n-\t\tint result = cf->chunks[i].write_fn(cf->f, data);\n+\t\tresult = cf->chunks[i].write_fn(cf->f, data);\n \n \t\tif (result)\n-\t\t\treturn result;\n+\t\t\tgoto cleanup;\n \n \t\tif (hashfile_total(cf->f) - start_offset != cf->chunks[i].size)\n \t\t\tBUG(\"expected to write %\"PRId64\" bytes to chunk %\"PRIx32\", but wrote %\"PRId64\" instead\",\n@@ -88,7 +90,9 @@ int write_chunkfile(struct chunkfile *cf, void *data)\n \t\t\t hashfile_total(cf->f) - start_offset);\n \t}\n \n-\treturn 0;\n+cleanup:\n+\ttrace2_region_leave(\"chunkfile\", \"write\", the_repository);\n+\treturn result;\n }\n \n int read_table_of_contents(struct chunkfile *cf,\ndiff --git a/csum-file.c b/csum-file.c\nindex 3c26389d4914..3487d28ed7ad 100644\n--- a/csum-file.c\n+++ b/csum-file.c\n@@ -11,19 +11,24 @@\n #include \"progress.h\"\n #include \"csum-file.h\"\n \n+static void verify_buffer_or_die(struct hashfile *f,\n+\t\t\t\t const void *buf,\n+\t\t\t\t unsigned int count)\n+{\n+\tssize_t ret = read_in_full(f->check_fd, f->check_buffer, count);\n+\n+\tif (ret < 0)\n+\t\tdie_errno(\"%s: sha1 file read error\", f->name);\n+\tif (ret != count)\n+\t\tdie(\"%s: sha1 file truncated\", f->name);\n+\tif (memcmp(buf, f->check_buffer, count))\n+\t\tdie(\"sha1 file '%s' validation error\", f->name);\n+}\n+\n static void flush(struct hashfile *f, const void *buf, unsigned int count)\n {\n-\tif (0 <= f->check_fd && count) {\n-\t\tunsigned char check_buffer[8192];\n-\t\tssize_t ret = read_in_full(f->check_fd, check_buffer, count);\n-\n-\t\tif (ret < 0)\n-\t\t\tdie_errno(\"%s: sha1 file read error\", f->name);\n-\t\tif (ret != count)\n-\t\t\tdie(\"%s: sha1 file truncated\", f->name);\n-\t\tif (memcmp(buf, check_buffer, count))\n-\t\t\tdie(\"sha1 file '%s' validation error\", f->name);\n-\t}\n+\tif (0 <= f->check_fd && count)\n+\t\tverify_buffer_or_die(f, buf, count);\n \n \tif (write_in_full(f->fd, buf, count) < 0) {\n \t\tif (errno == ENOSPC)\n@@ -46,6 +51,13 @@ void hashflush(struct hashfile *f)\n \t}\n }\n \n+static void free_hashfile(struct hashfile *f)\n+{\n+\tfree(f->buffer);\n+\tfree(f->check_buffer);\n+\tfree(f);\n+}\n+\n int finalize_hashfile(struct hashfile *f, unsigned char *result, unsigned int flags)\n {\n \tint fd;\n@@ -75,20 +87,20 @@ int finalize_hashfile(struct hashfile *f, unsigned char *result, unsigned int fl\n \t\tif (close(f->check_fd))\n \t\t\tdie_errno(\"%s: sha1 file error on close\", f->name);\n \t}\n-\tfree(f);\n+\tfree_hashfile(f);\n \treturn fd;\n }\n \n void hashwrite(struct hashfile *f, const void *buf, unsigned int count)\n {\n \twhile (count) {\n-\t\tunsigned left = sizeof(f->buffer) - f->offset;\n+\t\tunsigned left = f->buffer_len - f->offset;\n \t\tunsigned nr = count > left ? left : count;\n \n \t\tif (f->do_crc)\n \t\t\tf->crc32 = crc32(f->crc32, buf, nr);\n \n-\t\tif (nr == sizeof(f->buffer)) {\n+\t\tif (nr == f->buffer_len) {\n \t\t\t/*\n \t\t\t * Flush a full batch worth of data directly\n \t\t\t * from the input, skipping the memcpy() to\n@@ -114,11 +126,6 @@ void hashwrite(struct hashfile *f, const void *buf, unsigned int count)\n \t}\n }\n \n-struct hashfile *hashfd(int fd, const char *name)\n-{\n-\treturn hashfd_throughput(fd, name, NULL);\n-}\n-\n struct hashfile *hashfd_check(const char *name)\n {\n \tint sink, check;\n@@ -132,10 +139,14 @@ struct hashfile *hashfd_check(const char *name)\n \t\tdie_errno(\"unable to open '%s'\", name);\n \tf = hashfd(sink, name);\n \tf->check_fd = check;\n+\tf->check_buffer = xmalloc(f->buffer_len);\n+\n \treturn f;\n }\n \n-struct hashfile *hashfd_throughput(int fd, const char *name, struct progress *tp)\n+static struct hashfile *hashfd_internal(int fd, const char *name,\n+\t\t\t\t\tstruct progress *tp,\n+\t\t\t\t\tsize_t buffer_len)\n {\n \tstruct hashfile *f = xmalloc(sizeof(*f));\n \tf->fd = fd;\n@@ -146,9 +157,35 @@ struct hashfile *hashfd_throughput(int fd, const char *name, struct progress *tp\n \tf->name = name;\n \tf->do_crc = 0;\n \tthe_hash_algo->init_fn(&f->ctx);\n+\n+\tf->buffer_len = buffer_len;\n+\tf->buffer = xmalloc(buffer_len);\n+\tf->check_buffer = NULL;\n+\n \treturn f;\n }\n \n+struct hashfile *hashfd(int fd, const char *name)\n+{\n+\t/*\n+\t * Since we are not going to use a progress meter to\n+\t * measure the rate of data passing through this hashfile,\n+\t * use a larger buffer size to reduce fsync() calls.\n+\t */\n+\treturn hashfd_internal(fd, name, NULL, 128 * 1024);\n+}\n+\n+struct hashfile *hashfd_throughput(int fd, const char *name, struct progress *tp)\n+{\n+\t/*\n+\t * Since we are expecting to report progress of the\n+\t * write into this hashfile, use a smaller buffer\n+\t * size so the progress indicators arrive at a more\n+\t * frequent rate.\n+\t */\n+\treturn hashfd_internal(fd, name, tp, 8 * 1024);\n+}\n+\n void hashfile_checkpoint(struct hashfile *f, struct hashfile_checkpoint *checkpoint)\n {\n \thashflush(f);\ndiff --git a/csum-file.h b/csum-file.h\nindex e54d53d1d0b3..3044bd19ab65 100644\n--- a/csum-file.h\n+++ b/csum-file.h\n@@ -16,7 +16,9 @@ struct hashfile {\n \tconst char *name;\n \tint do_crc;\n \tuint32_t crc32;\n-\tunsigned char buffer[8192];\n+\tsize_t buffer_len;\n+\tunsigned char *buffer;\n+\tunsigned char *check_buffer;\n };\n \n /* Checkpoint */\n-- \ngitgitgadget\n\n"},{"id":"424894","messageId":"pull.916.v3.git.1621362768.gitgitgadget@gmail.com","threadId":"55392","inReplyTo":"pull.916.v2.git.1621254292.gitgitgadget@gmail.com","subject":"[PATCH v3 0/4] Convert index writes to use hashfile API","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-05-18T18:32:44Z","receivedAt":"2021-05-18T18:33:02Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"As I prepare some ideas on index v5, one thing that strikes me as an\ninteresting direction to try is to use the chunk-format API. This would make\nour extension model extremely simple (they become optional chunks, easily\nidentified by the table of contents).\n\nBut there is a huge hurdle to even starting that investigation: the index\nuses its own hashing methods, separate from the hashfile API in csum-file.c!\n\nThe internals of the algorithms are mostly identical. The only possible\nchange is that the buffer sizes are different: 8KB for hashfile and 128KB in\nread-cache.c. This change is actually relatively recent! I was able to see a\nperformance difference before changing that size, so I do make them equal.\nThis improves some other commands, too, such as git multi-pack-index write.\n\nThere is a subtle point about how the EOIE extension works in that it needs\na hash of just the previous extension headers. This won't be needed by the\nchunk-format API, so leave this mostly as-is, except where it collides\nsomewhat with the hashfile changes.\n\n\nUpdates in v3\n=============\n\n * Fixed typos in commit messages.\n\n * Updated buffer and check_buffer to be heap-allocated members of struct\n hashfile.\n\n * These buffers have variable size, allowing the uses that calculate a\n throughput progress bar can still use an 8k buffer, while the others grow\n to 128k.\n\n * The heap-allocated check_buffer allows the hashfile API to remain as\n thread safe as before (parallel threads can write to independent\n hashfiles, not the same one).\n\n\nUpdates in v2\n=============\n\n * Sorry for the false start on v1. I'm correctly testing index writes this\n time, but I also verified other commands, such as \"git add .\", to be sure\n I was exercising everything. I also previously had incorrect assumptions\n about the EOIE extension, since it is not triggered with \"small\" repos\n like the Linux kernel repository ;).\n\n * This version is rebased onto a recent version of 'master' so the\n conflicts with ds/sparse-index-protections are resolved.\n\n * This version abandons the nested hashfile concept, since that was only\n needed for the incorrect interpretation of the EOIE extension's hash.\n\n * This version does unify the buffer size to 128 KB with justification.\n\n * There is also some more unification of writing logic to use\n write_in_full() in the hashfile API.\n\n * When using check_fd, the hashfile API is no longer thread safe due to a\n stack-allocated buffer moving to the global scope. I'm not sure if there\n is a better way to handle this case.\n\nThanks, -Stolee\n\nDerrick Stolee (4):\n hashfile: use write_in_full()\n csum-file.h: increase hashfile buffer size\n read-cache: use hashfile instead of git_hash_ctx\n read-cache: delete unused hashing methods\n\n chunk-format.c | 12 ++--\n csum-file.c | 94 +++++++++++++++---------\n csum-file.h | 4 +-\n read-cache.c | 191 ++++++++++++++++---------------------------------\n 4 files changed, 134 insertions(+), 167 deletions(-)\n\n\nbase-commit: 97eea85a0a1ec66d356567808a1e4ca2367e0ce7\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-916%2Fderrickstolee%2Findex-hashfile-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-916/derrickstolee/index-hashfile-v3\nPull-Request: https://github.com/gitgitgadget/git/pull/916\n\nRange-diff vs v2:\n\n 1: b3e578ac0365 = 1: b3e578ac0365 hashfile: use write_in_full()\n 2: 9dc602f6c422 ! 2: 64ffddd79116 csum-file.h: increase hashfile buffer size\n @@ Commit message\n hashing buffers in read-cache.c, but that code was updated from 8KB to\n 128KB in f279894 (read-cache: make the index write buffer size 128K,\n 2021-02-18). The justification there was that do_write_index() improves\n - from 1.02s to 0.72s.\n + from 1.02s to 0.72s. Since our end goal is to have the index writing\n + code use the hashfile API, we need to unify this buffer size to avoid a\n + performance regression.\n \n - There is a buffer, check_buffer, that is used to verify the check_fd\n + There is a buffer, 'check_buffer', that is used to verify the check_fd\n file descriptor. When this buffer increases to 128K to fit the data\n being flushed, it causes the stack to overflow the limits placed in the\n - test suite. By moving this to a static buffer, we stop using stack data\n - for this purpose, but we lose some thread-safety. This change makes it\n - unsafe to write to multiple hashfiles across different threads.\n + test suite. To avoid issues with stack size, move both 'buffer' and\n + 'check_buffer' to be heap pointers within 'struct hashfile'. The\n + 'check_buffer' member is left as NULL unless check_fd is set in\n + hashfd_check(). Both buffers are cleared as part of finalize_hashfile()\n + which also frees the full structure.\n +\n + Since these buffers are now on the heap, we can adjust their size based\n + on the needs of the consumer. In particular, callers to\n + hashfd_throughput() are expecting to report progress indicators as the\n + buffer flushes. These callers would prefer the smaller 8k buffer to\n + avoid large delays between updates, especially for users with slower\n + networks. When the progress indicator is not used, the larger buffer is\n + preferrable.\n \n By adding a new trace2 region in the chunk-format API, we can see that\n the writing portion of 'git multi-pack-index write' lowers from ~1.49s\n @@ csum-file.c\n +\t\t\t\t const void *buf,\n +\t\t\t\t unsigned int count)\n +{\n -+\tstatic unsigned char check_buffer[WRITE_BUFFER_SIZE];\n -+\tssize_t ret = read_in_full(f->check_fd, check_buffer, count);\n ++\tssize_t ret = read_in_full(f->check_fd, f->check_buffer, count);\n +\n +\tif (ret < 0)\n +\t\tdie_errno(\"%s: sha1 file read error\", f->name);\n +\tif (ret != count)\n +\t\tdie(\"%s: sha1 file truncated\", f->name);\n -+\tif (memcmp(buf, check_buffer, count))\n ++\tif (memcmp(buf, f->check_buffer, count))\n +\t\tdie(\"sha1 file '%s' validation error\", f->name);\n +}\n +\n @@ csum-file.c\n \n \tif (write_in_full(f->fd, buf, count) < 0) {\n \t\tif (errno == ENOSPC)\n -\n - ## csum-file.h ##\n -@@\n - \n - struct progress;\n +@@ csum-file.c: void hashflush(struct hashfile *f)\n + \t}\n + }\n \n -+#define WRITE_BUFFER_SIZE (128 * 1024)\n ++static void free_hashfile(struct hashfile *f)\n ++{\n ++\tfree(f->buffer);\n ++\tfree(f->check_buffer);\n ++\tfree(f);\n ++}\n +\n - /* A SHA1-protected file */\n - struct hashfile {\n + int finalize_hashfile(struct hashfile *f, unsigned char *result, unsigned int flags)\n + {\n \tint fd;\n +@@ csum-file.c: int finalize_hashfile(struct hashfile *f, unsigned char *result, unsigned int fl\n + \t\tif (close(f->check_fd))\n + \t\t\tdie_errno(\"%s: sha1 file error on close\", f->name);\n + \t}\n +-\tfree(f);\n ++\tfree_hashfile(f);\n + \treturn fd;\n + }\n + \n + void hashwrite(struct hashfile *f, const void *buf, unsigned int count)\n + {\n + \twhile (count) {\n +-\t\tunsigned left = sizeof(f->buffer) - f->offset;\n ++\t\tunsigned left = f->buffer_len - f->offset;\n + \t\tunsigned nr = count > left ? left : count;\n + \n + \t\tif (f->do_crc)\n + \t\t\tf->crc32 = crc32(f->crc32, buf, nr);\n + \n +-\t\tif (nr == sizeof(f->buffer)) {\n ++\t\tif (nr == f->buffer_len) {\n + \t\t\t/*\n + \t\t\t * Flush a full batch worth of data directly\n + \t\t\t * from the input, skipping the memcpy() to\n +@@ csum-file.c: void hashwrite(struct hashfile *f, const void *buf, unsigned int count)\n + \t}\n + }\n + \n +-struct hashfile *hashfd(int fd, const char *name)\n +-{\n +-\treturn hashfd_throughput(fd, name, NULL);\n +-}\n +-\n + struct hashfile *hashfd_check(const char *name)\n + {\n + \tint sink, check;\n +@@ csum-file.c: struct hashfile *hashfd_check(const char *name)\n + \t\tdie_errno(\"unable to open '%s'\", name);\n + \tf = hashfd(sink, name);\n + \tf->check_fd = check;\n ++\tf->check_buffer = xmalloc(f->buffer_len);\n ++\n + \treturn f;\n + }\n + \n +-struct hashfile *hashfd_throughput(int fd, const char *name, struct progress *tp)\n ++static struct hashfile *hashfd_internal(int fd, const char *name,\n ++\t\t\t\t\tstruct progress *tp,\n ++\t\t\t\t\tsize_t buffer_len)\n + {\n + \tstruct hashfile *f = xmalloc(sizeof(*f));\n + \tf->fd = fd;\n +@@ csum-file.c: struct hashfile *hashfd_throughput(int fd, const char *name, struct progress *tp\n + \tf->name = name;\n + \tf->do_crc = 0;\n + \tthe_hash_algo->init_fn(&f->ctx);\n ++\n ++\tf->buffer_len = buffer_len;\n ++\tf->buffer = xmalloc(buffer_len);\n ++\tf->check_buffer = NULL;\n ++\n + \treturn f;\n + }\n + \n ++struct hashfile *hashfd(int fd, const char *name)\n ++{\n ++\t/*\n ++\t * Since we are not going to use a progress meter to\n ++\t * measure the rate of data passing through this hashfile,\n ++\t * use a larger buffer size to reduce fsync() calls.\n ++\t */\n ++\treturn hashfd_internal(fd, name, NULL, 128 * 1024);\n ++}\n ++\n ++struct hashfile *hashfd_throughput(int fd, const char *name, struct progress *tp)\n ++{\n ++\t/*\n ++\t * Since we are expecting to report progress of the\n ++\t * write into this hashfile, use a smaller buffer\n ++\t * size so the progress indicators arrive at a more\n ++\t * frequent rate.\n ++\t */\n ++\treturn hashfd_internal(fd, name, tp, 8 * 1024);\n ++}\n ++\n + void hashfile_checkpoint(struct hashfile *f, struct hashfile_checkpoint *checkpoint)\n + {\n + \thashflush(f);\n +\n + ## csum-file.h ##\n @@ csum-file.h: struct hashfile {\n \tconst char *name;\n \tint do_crc;\n \tuint32_t crc32;\n -\tunsigned char buffer[8192];\n -+\tunsigned char buffer[WRITE_BUFFER_SIZE];\n ++\tsize_t buffer_len;\n ++\tunsigned char *buffer;\n ++\tunsigned char *check_buffer;\n };\n \n /* Checkpoint */\n 3: b94172ccf5e9 ! 3: afd278bef3ab read-cache: use hashfile instead of git_hash_ctx\n @@ Commit message\n codebase, I figured that these roughly equivalent code paths were never\n unified only because it got lost in the shuffle. The hashfile API has\n since been used extensively in other file formats, such as pack-indexes,\n - mult-pack-indexes, and commit-graphs. Therefore, it seems prudent to\n + multi-pack-indexes, and commit-graphs. Therefore, it seems prudent to\n unify the index writing code to use the same mechanism.\n \n I discovered this disparity while trying to create a new index format\n @@ Commit message\n temporary file with writes to the hashfile struct. This takes care of\n many of the direct interactions with the_hash_algo.\n \n - There are still some remaining: the extension headers are hashed for use\n - in the End of Index Entries (EOIE) extension. This use of the\n - git_hash_ctx is left as-is. There are multiple reasons to not use a\n - hashfile here, including the fact that the data is not actually writing\n - to a file, just a hash computation. These hashes do not block our\n - adoption of the chunk-format API in a future change to the index, so\n + There are still some git_hash_ctx uses remaining: the extension headers\n + are hashed for use in the End of Index Entries (EOIE) extension. This\n + use of the git_hash_ctx is left as-is. There are multiple reasons to not\n + use a hashfile here, including the fact that the data is not actually\n + writing to a file, just a hash computation. These hashes do not block\n + our adoption of the chunk-format API in a future change to the index, so\n leave it as-is.\n \n The internals of the algorithms are mostly identical. Previously, the\n @@ Commit message\n the index data to be exactly equal. Finally, I did extensive performance\n testing of \"git update-index --force-write\" on repos of various sizes,\n including one with over 2 million paths at HEAD. These tests\n - demonstrated less than 1% difference in behavior, so the performance\n - should be considered identical.\n + demonstrated less than 1% difference in behavior. As expected, the\n + performance should be considered unchanged. The previous changes to\n + increase the hashfile buffer size from 8K to 128K ensured this change\n + would not create a peformance regression.\n \n Signed-off-by: Derrick Stolee \n \n 4: 4b3814eb4c80 = 4: 42fb10fb2998 read-cache: delete unused hashing methods\n\n-- \ngitgitgadget\n"},{"id":"424895","messageId":"b3e578ac0365fba9ef015f2e9a06593ce85d2814.1621362768.git.gitgitgadget@gmail.com","threadId":"55392","inReplyTo":"pull.916.v3.git.1621362768.gitgitgadget@gmail.com","subject":"[PATCH v3 1/4] hashfile: use write_in_full()","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-05-18T18:32:45Z","receivedAt":"2021-05-18T18:33:04Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe flush() logic in csum-file.c was introduced originally by c38138c\n(git-pack-objects: write the pack files with a SHA1 csum, 2005-06-26)\nand a portion of the logic performs similar utility to write_in_full()\nin wrapper.c. The history of write_in_full() is full of moves and\nrenames, but was originally introduced by 7230e6d (Add write_or_die(), a\nhelper function, 2006-08-21).\n\nThe point of these sections of code are to flush a write buffer using\nxwrite() and report errors in the case of disk space issues or other\ngeneric input/output errors. The logic in flush() can interpret the\noutput of write_in_full() to provide the correct error messages to\nusers.\n\nThe logic in the hashfile API has an additional set of logic to augment\nthe progress indicator between calls to xwrite(). This was introduced by\n2a128d6 (add throughput display to git-push, 2007-10-30). It seems that\nsince the hashfile's buffer is only 8KB, these additional progress\nindicators might not be incredibly necessary. Instead, update the\nprogress only when write_in_full() complete.\n\nSigned-off-by: Derrick Stolee \n---\n csum-file.c | 17 +++++------------\n 1 file changed, 5 insertions(+), 12 deletions(-)\n\ndiff --git a/csum-file.c b/csum-file.c\nindex 7510950fa3e9..3c26389d4914 100644\n--- a/csum-file.c\n+++ b/csum-file.c\n@@ -25,21 +25,14 @@ static void flush(struct hashfile *f, const void *buf, unsigned int count)\n \t\t\tdie(\"sha1 file '%s' validation error\", f->name);\n \t}\n \n-\tfor (;;) {\n-\t\tint ret = xwrite(f->fd, buf, count);\n-\t\tif (ret > 0) {\n-\t\t\tf->total += ret;\n-\t\t\tdisplay_throughput(f->tp, f->total);\n-\t\t\tbuf = (char *) buf + ret;\n-\t\t\tcount -= ret;\n-\t\t\tif (count)\n-\t\t\t\tcontinue;\n-\t\t\treturn;\n-\t\t}\n-\t\tif (!ret)\n+\tif (write_in_full(f->fd, buf, count) < 0) {\n+\t\tif (errno == ENOSPC)\n \t\t\tdie(\"sha1 file '%s' write error. Out of diskspace\", f->name);\n \t\tdie_errno(\"sha1 file '%s' write error\", f->name);\n \t}\n+\n+\tf->total += count;\n+\tdisplay_throughput(f->tp, f->total);\n }\n \n void hashflush(struct hashfile *f)\n-- \ngitgitgadget\n\n"},{"id":"424896","messageId":"afd278bef3abce511b0ff90dd9b28e135baccc54.1621362768.git.gitgitgadget@gmail.com","threadId":"55392","inReplyTo":"pull.916.v3.git.1621362768.gitgitgadget@gmail.com","subject":"[PATCH v3 3/4] read-cache: use hashfile instead of git_hash_ctx","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-05-18T18:32:47Z","receivedAt":"2021-05-18T18:33:04Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe do_write_index() method in read-cache.c has its own hashing logic\nand buffering mechanism. Specifically, the ce_write() method was\nintroduced by 4990aadc (Speed up index file writing by chunking it\nnicely, 2005-04-20) and similar mechanisms were introduced a few months\nlater in c38138cd (git-pack-objects: write the pack files with a SHA1\ncsum, 2005-06-26). Based on the timing, in the early days of the Git\ncodebase, I figured that these roughly equivalent code paths were never\nunified only because it got lost in the shuffle. The hashfile API has\nsince been used extensively in other file formats, such as pack-indexes,\nmulti-pack-indexes, and commit-graphs. Therefore, it seems prudent to\nunify the index writing code to use the same mechanism.\n\nI discovered this disparity while trying to create a new index format\nthat uses the chunk-format API. That API uses a hashfile as its base, so\nit is incompatible with the custom code in read-cache.c.\n\nThis rewrite is rather straightforward. It replaces all writes to the\ntemporary file with writes to the hashfile struct. This takes care of\nmany of the direct interactions with the_hash_algo.\n\nThere are still some git_hash_ctx uses remaining: the extension headers\nare hashed for use in the End of Index Entries (EOIE) extension. This\nuse of the git_hash_ctx is left as-is. There are multiple reasons to not\nuse a hashfile here, including the fact that the data is not actually\nwriting to a file, just a hash computation. These hashes do not block\nour adoption of the chunk-format API in a future change to the index, so\nleave it as-is.\n\nThe internals of the algorithms are mostly identical. Previously, the\nhashfile API used a smaller 8KB buffer instead of the 128KB buffer from\nread-cache.c. The previous change already unified these sizes.\n\nThere is one subtle point: we do not pass the CSUM_FSYNC to the\nfinalize_hashfile() method, which differs from most consumers of the\nhashfile API. The extra fsync() call indicated by this flag causes a\nsignificant peformance degradation that is noticeable for quick\ncommands that write the index, such as \"git add\". Other consumers can\nabsorb this cost with their more complicated data structure\norganization, and further writing structures such as pack-files and\ncommit-graphs is rarely in the critical path for common user\ninteractions.\n\nSome static methods become orphaned in this diff, so I marked them as\nMAYBE_UNUSED. The diff is much harder to read if they are deleted during\nthis change. Instead, they will be deleted in the following change.\n\nIn addition to the test suite passing, I computed indexes using the\nprevious binaries and the binaries compiled after this change, and found\nthe index data to be exactly equal. Finally, I did extensive performance\ntesting of \"git update-index --force-write\" on repos of various sizes,\nincluding one with over 2 million paths at HEAD. These tests\ndemonstrated less than 1% difference in behavior. As expected, the\nperformance should be considered unchanged. The previous changes to\nincrease the hashfile buffer size from 8K to 128K ensured this change\nwould not create a peformance regression.\n\nSigned-off-by: Derrick Stolee \n---\n read-cache.c | 137 +++++++++++++++++++++++++--------------------------\n 1 file changed, 66 insertions(+), 71 deletions(-)\n\ndiff --git a/read-cache.c b/read-cache.c\nindex fbf3a4ce7d5d..1c0bda81e7e7 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -26,6 +26,7 @@\n #include \"thread-utils.h\"\n #include \"progress.h\"\n #include \"sparse-index.h\"\n+#include \"csum-file.h\"\n \n /* Mask for the name length in ce_flags in the on-disk index */\n \n@@ -2519,6 +2520,7 @@ int repo_index_has_changes(struct repository *repo,\n static unsigned char write_buffer[WRITE_BUFFER_SIZE];\n static unsigned long write_buffer_len;\n \n+MAYBE_UNUSED\n static int ce_write_flush(git_hash_ctx *context, int fd)\n {\n \tunsigned int buffered = write_buffer_len;\n@@ -2531,6 +2533,7 @@ static int ce_write_flush(git_hash_ctx *context, int fd)\n \treturn 0;\n }\n \n+MAYBE_UNUSED\n static int ce_write(git_hash_ctx *context, int fd, void *data, unsigned int len)\n {\n \twhile (len) {\n@@ -2553,19 +2556,24 @@ static int ce_write(git_hash_ctx *context, int fd, void *data, unsigned int len)\n \treturn 0;\n }\n \n-static int write_index_ext_header(git_hash_ctx *context, git_hash_ctx *eoie_context,\n-\t\t\t\t int fd, unsigned int ext, unsigned int sz)\n+static int write_index_ext_header(struct hashfile *f,\n+\t\t\t\t git_hash_ctx *eoie_f,\n+\t\t\t\t unsigned int ext,\n+\t\t\t\t unsigned int sz)\n {\n-\text = htonl(ext);\n-\tsz = htonl(sz);\n-\tif (eoie_context) {\n-\t\tthe_hash_algo->update_fn(eoie_context, &ext, 4);\n-\t\tthe_hash_algo->update_fn(eoie_context, &sz, 4);\n+\thashwrite_be32(f, ext);\n+\thashwrite_be32(f, sz);\n+\n+\tif (eoie_f) {\n+\t\text = htonl(ext);\n+\t\tsz = htonl(sz);\n+\t\tthe_hash_algo->update_fn(eoie_f, &ext, sizeof(ext));\n+\t\tthe_hash_algo->update_fn(eoie_f, &sz, sizeof(sz));\n \t}\n-\treturn ((ce_write(context, fd, &ext, 4) < 0) ||\n-\t\t(ce_write(context, fd, &sz, 4) < 0)) ? -1 : 0;\n+\treturn 0;\n }\n \n+MAYBE_UNUSED\n static int ce_flush(git_hash_ctx *context, int fd, unsigned char *hash)\n {\n \tunsigned int left = write_buffer_len;\n@@ -2667,11 +2675,10 @@ static void copy_cache_entry_to_ondisk(struct ondisk_cache_entry *ondisk,\n \t}\n }\n \n-static int ce_write_entry(git_hash_ctx *c, int fd, struct cache_entry *ce,\n+static int ce_write_entry(struct hashfile *f, struct cache_entry *ce,\n \t\t\t struct strbuf *previous_name, struct ondisk_cache_entry *ondisk)\n {\n \tint size;\n-\tint result;\n \tunsigned int saved_namelen;\n \tint stripped_name = 0;\n \tstatic unsigned char padding[8] = { 0x00 };\n@@ -2687,11 +2694,9 @@ static int ce_write_entry(git_hash_ctx *c, int fd, struct cache_entry *ce,\n \tif (!previous_name) {\n \t\tint len = ce_namelen(ce);\n \t\tcopy_cache_entry_to_ondisk(ondisk, ce);\n-\t\tresult = ce_write(c, fd, ondisk, size);\n-\t\tif (!result)\n-\t\t\tresult = ce_write(c, fd, ce->name, len);\n-\t\tif (!result)\n-\t\t\tresult = ce_write(c, fd, padding, align_padding_size(size, len));\n+\t\thashwrite(f, ondisk, size);\n+\t\thashwrite(f, ce->name, len);\n+\t\thashwrite(f, padding, align_padding_size(size, len));\n \t} else {\n \t\tint common, to_remove, prefix_size;\n \t\tunsigned char to_remove_vi[16];\n@@ -2705,13 +2710,10 @@ static int ce_write_entry(git_hash_ctx *c, int fd, struct cache_entry *ce,\n \t\tprefix_size = encode_varint(to_remove, to_remove_vi);\n \n \t\tcopy_cache_entry_to_ondisk(ondisk, ce);\n-\t\tresult = ce_write(c, fd, ondisk, size);\n-\t\tif (!result)\n-\t\t\tresult = ce_write(c, fd, to_remove_vi, prefix_size);\n-\t\tif (!result)\n-\t\t\tresult = ce_write(c, fd, ce->name + common, ce_namelen(ce) - common);\n-\t\tif (!result)\n-\t\t\tresult = ce_write(c, fd, padding, 1);\n+\t\thashwrite(f, ondisk, size);\n+\t\thashwrite(f, to_remove_vi, prefix_size);\n+\t\thashwrite(f, ce->name + common, ce_namelen(ce) - common);\n+\t\thashwrite(f, padding, 1);\n \n \t\tstrbuf_splice(previous_name, common, to_remove,\n \t\t\t ce->name + common, ce_namelen(ce) - common);\n@@ -2721,7 +2723,7 @@ static int ce_write_entry(git_hash_ctx *c, int fd, struct cache_entry *ce,\n \t\tce->ce_flags &= ~CE_STRIP_NAME;\n \t}\n \n-\treturn result;\n+\treturn 0;\n }\n \n /*\n@@ -2833,8 +2835,8 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,\n \t\t\t int strip_extensions)\n {\n \tuint64_t start = getnanotime();\n-\tint newfd = tempfile->fd;\n-\tgit_hash_ctx c, eoie_c;\n+\tstruct hashfile *f;\n+\tgit_hash_ctx *eoie_c = NULL;\n \tstruct cache_header hdr;\n \tint i, err = 0, removed, extended, hdr_version;\n \tstruct cache_entry **cache = istate->cache;\n@@ -2848,6 +2850,8 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,\n \tstruct index_entry_offset_table *ieot = NULL;\n \tint nr, nr_threads;\n \n+\tf = hashfd(tempfile->fd, tempfile->filename.buf);\n+\n \tfor (i = removed = extended = 0; i < entries; i++) {\n \t\tif (cache[i]->ce_flags & CE_REMOVE)\n \t\t\tremoved++;\n@@ -2876,9 +2880,7 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,\n \thdr.hdr_version = htonl(hdr_version);\n \thdr.hdr_entries = htonl(entries - removed);\n \n-\tthe_hash_algo->init_fn(&c);\n-\tif (ce_write(&c, newfd, &hdr, sizeof(hdr)) < 0)\n-\t\treturn -1;\n+\thashwrite(f, &hdr, sizeof(hdr));\n \n \tif (!HAVE_THREADS || git_config_get_index_threads(&nr_threads))\n \t\tnr_threads = 1;\n@@ -2913,12 +2915,8 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,\n \t\t}\n \t}\n \n-\toffset = lseek(newfd, 0, SEEK_CUR);\n-\tif (offset < 0) {\n-\t\tfree(ieot);\n-\t\treturn -1;\n-\t}\n-\toffset += write_buffer_len;\n+\toffset = hashfile_total(f);\n+\n \tnr = 0;\n \tprevious_name = (hdr_version == 4) ? &previous_name_buf : NULL;\n \n@@ -2953,14 +2951,10 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,\n \t\t\tif (previous_name)\n \t\t\t\tprevious_name->buf[0] = 0;\n \t\t\tnr = 0;\n-\t\t\toffset = lseek(newfd, 0, SEEK_CUR);\n-\t\t\tif (offset < 0) {\n-\t\t\t\tfree(ieot);\n-\t\t\t\treturn -1;\n-\t\t\t}\n-\t\t\toffset += write_buffer_len;\n+\n+\t\t\toffset = hashfile_total(f);\n \t\t}\n-\t\tif (ce_write_entry(&c, newfd, ce, previous_name, (struct ondisk_cache_entry *)&ondisk) < 0)\n+\t\tif (ce_write_entry(f, ce, previous_name, (struct ondisk_cache_entry *)&ondisk) < 0)\n \t\t\terr = -1;\n \n \t\tif (err)\n@@ -2979,14 +2973,16 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,\n \t\treturn err;\n \t}\n \n-\t/* Write extension data here */\n-\toffset = lseek(newfd, 0, SEEK_CUR);\n-\tif (offset < 0) {\n-\t\tfree(ieot);\n-\t\treturn -1;\n+\toffset = hashfile_total(f);\n+\n+\t/*\n+\t * The extension headers must be hashed on their own for the\n+\t * EOIE extension. Create a hashfile here to compute that hash.\n+\t */\n+\tif (offset && record_eoie()) {\n+\t\tCALLOC_ARRAY(eoie_c, 1);\n+\t\tthe_hash_algo->init_fn(eoie_c);\n \t}\n-\toffset += write_buffer_len;\n-\tthe_hash_algo->init_fn(&eoie_c);\n \n \t/*\n \t * Lets write out CACHE_EXT_INDEXENTRYOFFSETTABLE first so that we\n@@ -2999,8 +2995,8 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,\n \t\tstruct strbuf sb = STRBUF_INIT;\n \n \t\twrite_ieot_extension(&sb, ieot);\n-\t\terr = write_index_ext_header(&c, &eoie_c, newfd, CACHE_EXT_INDEXENTRYOFFSETTABLE, sb.len) < 0\n-\t\t\t|| ce_write(&c, newfd, sb.buf, sb.len) < 0;\n+\t\terr = write_index_ext_header(f, eoie_c, CACHE_EXT_INDEXENTRYOFFSETTABLE, sb.len) < 0;\n+\t\thashwrite(f, sb.buf, sb.len);\n \t\tstrbuf_release(&sb);\n \t\tfree(ieot);\n \t\tif (err)\n@@ -3012,9 +3008,9 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,\n \t\tstruct strbuf sb = STRBUF_INIT;\n \n \t\terr = write_link_extension(&sb, istate) < 0 ||\n-\t\t\twrite_index_ext_header(&c, &eoie_c, newfd, CACHE_EXT_LINK,\n-\t\t\t\t\t sb.len) < 0 ||\n-\t\t\tce_write(&c, newfd, sb.buf, sb.len) < 0;\n+\t\t\twrite_index_ext_header(f, eoie_c, CACHE_EXT_LINK,\n+\t\t\t\t\t sb.len) < 0;\n+\t\thashwrite(f, sb.buf, sb.len);\n \t\tstrbuf_release(&sb);\n \t\tif (err)\n \t\t\treturn -1;\n@@ -3023,8 +3019,8 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,\n \t\tstruct strbuf sb = STRBUF_INIT;\n \n \t\tcache_tree_write(&sb, istate->cache_tree);\n-\t\terr = write_index_ext_header(&c, &eoie_c, newfd, CACHE_EXT_TREE, sb.len) < 0\n-\t\t\t|| ce_write(&c, newfd, sb.buf, sb.len) < 0;\n+\t\terr = write_index_ext_header(f, eoie_c, CACHE_EXT_TREE, sb.len) < 0;\n+\t\thashwrite(f, sb.buf, sb.len);\n \t\tstrbuf_release(&sb);\n \t\tif (err)\n \t\t\treturn -1;\n@@ -3033,9 +3029,9 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,\n \t\tstruct strbuf sb = STRBUF_INIT;\n \n \t\tresolve_undo_write(&sb, istate->resolve_undo);\n-\t\terr = write_index_ext_header(&c, &eoie_c, newfd, CACHE_EXT_RESOLVE_UNDO,\n-\t\t\t\t\t sb.len) < 0\n-\t\t\t|| ce_write(&c, newfd, sb.buf, sb.len) < 0;\n+\t\terr = write_index_ext_header(f, eoie_c, CACHE_EXT_RESOLVE_UNDO,\n+\t\t\t\t\t sb.len) < 0;\n+\t\thashwrite(f, sb.buf, sb.len);\n \t\tstrbuf_release(&sb);\n \t\tif (err)\n \t\t\treturn -1;\n@@ -3044,9 +3040,9 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,\n \t\tstruct strbuf sb = STRBUF_INIT;\n \n \t\twrite_untracked_extension(&sb, istate->untracked);\n-\t\terr = write_index_ext_header(&c, &eoie_c, newfd, CACHE_EXT_UNTRACKED,\n-\t\t\t\t\t sb.len) < 0 ||\n-\t\t\tce_write(&c, newfd, sb.buf, sb.len) < 0;\n+\t\terr = write_index_ext_header(f, eoie_c, CACHE_EXT_UNTRACKED,\n+\t\t\t\t\t sb.len) < 0;\n+\t\thashwrite(f, sb.buf, sb.len);\n \t\tstrbuf_release(&sb);\n \t\tif (err)\n \t\t\treturn -1;\n@@ -3055,14 +3051,14 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,\n \t\tstruct strbuf sb = STRBUF_INIT;\n \n \t\twrite_fsmonitor_extension(&sb, istate);\n-\t\terr = write_index_ext_header(&c, &eoie_c, newfd, CACHE_EXT_FSMONITOR, sb.len) < 0\n-\t\t\t|| ce_write(&c, newfd, sb.buf, sb.len) < 0;\n+\t\terr = write_index_ext_header(f, eoie_c, CACHE_EXT_FSMONITOR, sb.len) < 0;\n+\t\thashwrite(f, sb.buf, sb.len);\n \t\tstrbuf_release(&sb);\n \t\tif (err)\n \t\t\treturn -1;\n \t}\n \tif (istate->sparse_index) {\n-\t\tif (write_index_ext_header(&c, &eoie_c, newfd, CACHE_EXT_SPARSE_DIRECTORIES, 0) < 0)\n+\t\tif (write_index_ext_header(f, eoie_c, CACHE_EXT_SPARSE_DIRECTORIES, 0) < 0)\n \t\t\treturn -1;\n \t}\n \n@@ -3072,19 +3068,18 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,\n \t * read. Write it out regardless of the strip_extensions parameter as we need it\n \t * when loading the shared index.\n \t */\n-\tif (offset && record_eoie()) {\n+\tif (eoie_c) {\n \t\tstruct strbuf sb = STRBUF_INIT;\n \n-\t\twrite_eoie_extension(&sb, &eoie_c, offset);\n-\t\terr = write_index_ext_header(&c, NULL, newfd, CACHE_EXT_ENDOFINDEXENTRIES, sb.len) < 0\n-\t\t\t|| ce_write(&c, newfd, sb.buf, sb.len) < 0;\n+\t\twrite_eoie_extension(&sb, eoie_c, offset);\n+\t\terr = write_index_ext_header(f, NULL, CACHE_EXT_ENDOFINDEXENTRIES, sb.len) < 0;\n+\t\thashwrite(f, sb.buf, sb.len);\n \t\tstrbuf_release(&sb);\n \t\tif (err)\n \t\t\treturn -1;\n \t}\n \n-\tif (ce_flush(&c, newfd, istate->oid.hash))\n-\t\treturn -1;\n+\tfinalize_hashfile(f, istate->oid.hash, CSUM_HASH_IN_STREAM);\n \tif (close_tempfile_gently(tempfile)) {\n \t\terror(_(\"could not close '%s'\"), get_tempfile_path(tempfile));\n \t\treturn -1;\n-- \ngitgitgadget\n\n"},{"id":"424897","messageId":"42fb10fb29982fa828c1ae819a9a1379b41cbd5e.1621362768.git.gitgitgadget@gmail.com","threadId":"55392","inReplyTo":"pull.916.v3.git.1621362768.gitgitgadget@gmail.com","subject":"[PATCH v3 4/4] read-cache: delete unused hashing methods","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-05-18T18:32:48Z","receivedAt":"2021-05-18T18:33:04Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThese methods were marked as MAYBE_UNUSED in the previous change to\navoid a complicated diff. Delete them entirely, since we now use the\nhashfile API instead of this custom hashing code.\n\nSigned-off-by: Derrick Stolee \n---\n read-cache.c | 64 ----------------------------------------------------\n 1 file changed, 64 deletions(-)\n\ndiff --git a/read-cache.c b/read-cache.c\nindex 1c0bda81e7e7..aa6751c6a092 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -2516,46 +2516,6 @@ int repo_index_has_changes(struct repository *repo,\n \t}\n }\n \n-#define WRITE_BUFFER_SIZE (128 * 1024)\n-static unsigned char write_buffer[WRITE_BUFFER_SIZE];\n-static unsigned long write_buffer_len;\n-\n-MAYBE_UNUSED\n-static int ce_write_flush(git_hash_ctx *context, int fd)\n-{\n-\tunsigned int buffered = write_buffer_len;\n-\tif (buffered) {\n-\t\tthe_hash_algo->update_fn(context, write_buffer, buffered);\n-\t\tif (write_in_full(fd, write_buffer, buffered) < 0)\n-\t\t\treturn -1;\n-\t\twrite_buffer_len = 0;\n-\t}\n-\treturn 0;\n-}\n-\n-MAYBE_UNUSED\n-static int ce_write(git_hash_ctx *context, int fd, void *data, unsigned int len)\n-{\n-\twhile (len) {\n-\t\tunsigned int buffered = write_buffer_len;\n-\t\tunsigned int partial = WRITE_BUFFER_SIZE - buffered;\n-\t\tif (partial > len)\n-\t\t\tpartial = len;\n-\t\tmemcpy(write_buffer + buffered, data, partial);\n-\t\tbuffered += partial;\n-\t\tif (buffered == WRITE_BUFFER_SIZE) {\n-\t\t\twrite_buffer_len = buffered;\n-\t\t\tif (ce_write_flush(context, fd))\n-\t\t\t\treturn -1;\n-\t\t\tbuffered = 0;\n-\t\t}\n-\t\twrite_buffer_len = buffered;\n-\t\tlen -= partial;\n-\t\tdata = (char *) data + partial;\n-\t}\n-\treturn 0;\n-}\n-\n static int write_index_ext_header(struct hashfile *f,\n \t\t\t\t git_hash_ctx *eoie_f,\n \t\t\t\t unsigned int ext,\n@@ -2573,30 +2533,6 @@ static int write_index_ext_header(struct hashfile *f,\n \treturn 0;\n }\n \n-MAYBE_UNUSED\n-static int ce_flush(git_hash_ctx *context, int fd, unsigned char *hash)\n-{\n-\tunsigned int left = write_buffer_len;\n-\n-\tif (left) {\n-\t\twrite_buffer_len = 0;\n-\t\tthe_hash_algo->update_fn(context, write_buffer, left);\n-\t}\n-\n-\t/* Flush first if not enough space for hash signature */\n-\tif (left + the_hash_algo->rawsz > WRITE_BUFFER_SIZE) {\n-\t\tif (write_in_full(fd, write_buffer, left) < 0)\n-\t\t\treturn -1;\n-\t\tleft = 0;\n-\t}\n-\n-\t/* Append the hash signature at the end */\n-\tthe_hash_algo->final_fn(write_buffer + left, context);\n-\thashcpy(hash, write_buffer + left);\n-\tleft += the_hash_algo->rawsz;\n-\treturn (write_in_full(fd, write_buffer, left) < 0) ? -1 : 0;\n-}\n-\n static void ce_smudge_racily_clean_entry(struct index_state *istate,\n \t\t\t\t\t struct cache_entry *ce)\n {\n-- \ngitgitgadget\n"},{"id":"442305","messageId":"211125.86pmqoifp8.gmgdl@evledraar.gmail.com","threadId":"55392","inReplyTo":"64ffddd791160895b8e6730ebcddfac8458653f2.1621362768.git.gitgitgadget@gmail.com","subject":"t4216-log-bloom.sh fails with -v (but not --verbose-log)","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-11-25T12:14:45Z","receivedAt":"2021-11-25T12:28:38Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"I haven't looked much into $subject, but there's an interesting\nregression in 2ca245f8be5 (csum-file.h: increase hashfile buffer size,\n2021-05-18) where it fails with -v, but not --verbose-log. Discovered\nwhile running it manually.\n\nThis is a regression in v2.33.0 (not v2.34.0!), so nothing urgent, and\nthis is pretty obscure anyway.\n\nFor the original change see:\nhttps://lore.kernel.org/git/64ffddd791160895b8e6730ebcddfac8458653f2.1621362768.git.gitgitgadget@gmail.com/\n\nTo reproduce it:\n\n make test T=t4216-log-bloom.sh GIT_TEST_OPTS=-v DEFAULT_TEST_TARGET=test\n\nRemoving the -v, or making it --verbose-log will make it succeed. It\nfails on:\n \n + GIT_TEST_BLOOM_SETTINGS_MAX_CHANGED_PATHS=10 GIT_TRACE2_EVENT=/home/avar/g/git/t/trash directory.t4216-log-bloom/limits/trace git commit-graph write --reachable --changed-paths\n + test_max_changed_paths 10 trace\n + grep \"max_changed_paths\":10 trace\n error: last command exited with $?=1\n not ok 135 - correctly report changes over limit\n\nIf it's run with/without a debugging shimmy to save away the \"trace\" we\ncan see that it doesn't include the max_changed_paths trace2 payload\nwith -v. I.e. for some reason we either don't run\ntrace2_bloom_filter_settings(), or don't log that data in the same way.\n\nIt's probably easy to figure out for someone familiar with this code,\nbut I couldn't see it from some quick skimming. Hence this\nE-Mail.\n\nFYI This is our only failure when running the whole test suite with -j1\n-x (I didn't test a full run with just -v, takes a while, I was using\n-j1 to get sensible output at the end without needing to scroll up).\n"},{"id":"442340","messageId":"YaBdxL3QH1/GrWY1@coredump.intra.peff.net","threadId":"55392","inReplyTo":"211125.86pmqoifp8.gmgdl@evledraar.gmail.com","subject":"Re: t4216-log-bloom.sh fails with -v (but not --verbose-log)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-11-26T04:08:36Z","receivedAt":"2021-11-26T04:10:43Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 25, 2021 at 01:14:45PM +0100, Ævar Arnfjörð Bjarmason wrote:\n\n> I haven't looked much into $subject, but there's an interesting\n> regression in 2ca245f8be5 (csum-file.h: increase hashfile buffer size,\n> 2021-05-18) where it fails with -v, but not --verbose-log. Discovered\n> while running it manually.\n> \n> This is a regression in v2.33.0 (not v2.34.0!), so nothing urgent, and\n> this is pretty obscure anyway.\n> \n> For the original change see:\n> https://lore.kernel.org/git/64ffddd791160895b8e6730ebcddfac8458653f2.1621362768.git.gitgitgadget@gmail.com/\n\nInteresting. This patch makes it go away (the \"5\" is cargo-culted from\nearlier in the script):\n\ndiff --git a/t/t4216-log-bloom.sh b/t/t4216-log-bloom.sh\nindex 50f206db55..2f3a1cd210 100755\n--- a/t/t4216-log-bloom.sh\n+++ b/t/t4216-log-bloom.sh\n@@ -231,6 +231,7 @@ test_expect_success 'correctly report changes over limit' '\n \n \t\t# Commit has 7 file and 4 directory adds\n \t\tGIT_TEST_BLOOM_SETTINGS_MAX_CHANGED_PATHS=10 \\\n+\t\t\tGIT_TRACE2_EVENT_NESTING=5 \\\n \t\t\tGIT_TRACE2_EVENT=\"$(pwd)/trace\" \\\n \t\t\tgit commit-graph write --reachable --changed-paths &&\n \t\ttest_max_changed_paths 10 trace &&\n@@ -263,6 +264,7 @@ test_expect_success 'correctly report changes over limit' '\n \t\t# start from scratch and rebuild\n \t\trm -f .git/objects/info/commit-graph &&\n \t\tGIT_TEST_BLOOM_SETTINGS_MAX_CHANGED_PATHS=10 \\\n+\t\t\tGIT_TRACE2_EVENT_NESTING=5 \\\n \t\t\tGIT_TRACE2_EVENT=\"$(pwd)/trace-edit\" \\\n \t\t\tgit commit-graph write --reachable --changed-paths &&\n \t\ttest_max_changed_paths 10 trace-edit &&\n@@ -280,6 +282,7 @@ test_expect_success 'correctly report changes over limit' '\n \t\t# start from scratch and rebuild\n \t\trm -f .git/objects/info/commit-graph &&\n \t\tGIT_TEST_BLOOM_SETTINGS_MAX_CHANGED_PATHS=11 \\\n+\t\t\tGIT_TRACE2_EVENT_NESTING=5 \\\n \t\t\tGIT_TRACE2_EVENT=\"$(pwd)/trace-update\" \\\n \t\t\tgit commit-graph write --reachable --changed-paths &&\n \t\ttest_max_changed_paths 11 trace-update &&\n\nThe commit in question (2ca245f8be) puts the writing into a new trace2\nregion (\"chunkfile\"), so it makes sense that the nesting increases by\none. But what's interesting is that the nesting is different depending\non whether stderr is a terminal. I guess because the progress code\nstarts its own region.\n\nThe default nesting max for trace2 is 2. That seems kind of low given\nthis example, but I don't know enough about the tradeoffs to say what\nbad things might happen if it's raised. But the above patch really seems\nlike a hack, and that this quiet omission would absolutely confuse real\nusers who are trying to use trace2 for debugging.\n\n-Peff\n"},{"id":"442444","messageId":"d41a9117-ee7c-d708-39f8-03e05a863dab@gmail.com","threadId":"55392","inReplyTo":"YaBdxL3QH1/GrWY1@coredump.intra.peff.net","subject":"Re: t4216-log-bloom.sh fails with -v (but not --verbose-log)","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-11-29T13:49:44Z","receivedAt":"2021-11-29T15:42:15Z","isPatch":false,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 11/25/2021 11:08 PM, Jeff King wrote:\n> On Thu, Nov 25, 2021 at 01:14:45PM +0100, Ævar Arnfjörð Bjarmason wrote:\n> \n>> I haven't looked much into $subject, but there's an interesting\n>> regression in 2ca245f8be5 (csum-file.h: increase hashfile buffer size,\n>> 2021-05-18) where it fails with -v, but not --verbose-log. Discovered\n>> while running it manually.\n>>\n>> This is a regression in v2.33.0 (not v2.34.0!), so nothing urgent, and\n>> this is pretty obscure anyway.\n>>\n>> For the original change see:\n>> https://lore.kernel.org/git/64ffddd791160895b8e6730ebcddfac8458653f2.1621362768.git.gitgitgadget@gmail.com/\n> \n> Interesting. This patch makes it go away (the \"5\" is cargo-culted from\n> earlier in the script):\n...\n> \t\t# Commit has 7 file and 4 directory adds\n> \t\tGIT_TEST_BLOOM_SETTINGS_MAX_CHANGED_PATHS=10 \\\n> +\t\t\tGIT_TRACE2_EVENT_NESTING=5 \\\n> \t\t\tGIT_TRACE2_EVENT=\"$(pwd)/trace\" \\\n> \t\t\tgit commit-graph write --reachable --changed-paths &&\n> \t\ttest_max_changed_paths 10 trace &&\n...\n> The commit in question (2ca245f8be) puts the writing into a new trace2\n> region (\"chunkfile\"), so it makes sense that the nesting increases by\n> one. But what's interesting is that the nesting is different depending\n> on whether stderr is a terminal. I guess because the progress code\n> starts its own region.\n> \n> The default nesting max for trace2 is 2. That seems kind of low given\n> this example, but I don't know enough about the tradeoffs to say what\n> bad things might happen if it's raised. But the above patch really seems\n> like a hack, and that this quiet omission would absolutely confuse real\n> users who are trying to use trace2 for debugging.\n\nThanks, both, for identifying the problem and the root cause. I have\nsent a patch series [1] that sets a deeper GIT_TRACE2_EVENT_NESTING\nacross the test suite to avoid this kind of issue in the future (along\nwith removing the existing uses scattered across the tests).\n\nThanks,\n-Stolee\n\n[1] https://lore.kernel.org/git/pull.1085.git.1638193666.gitgitgadget@gmail.com/\n"}]}