| Message ID | 9dc602f6c4221e2259778842ec3d1eda57508333.1621254292.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Convert index writes to use hashfile API | expand |
"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Derrick Stolee <dstolee@microsoft.com> > > The hashfile API uses a hard-coded buffer size of 8KB and has ever since > it was introduced in c38138c (git-pack-objects: write the pack files > with a SHA1 csum, 2005-06-26). It performs a similar function to the > hashing buffers in read-cache.c, but that code was updated from 8KB to > 128KB in f279894 (read-cache: make the index write buffer size 128K, > 2021-02-18). The justification there was that do_write_index() improves > from 1.02s to 0.72s. > > There is a buffer, check_buffer, that is used to verify the check_fd > file descriptor. When this buffer increases to 128K to fit the data > being flushed, it causes the stack to overflow the limits placed in the > test suite. By moving this to a static buffer, we stop using stack data > for this purpose, but we lose some thread-safety. This change makes it > unsafe to write to multiple hashfiles across different threads. So... is the longer term plan to heap-allocate the buffer itself, and replace the .buffer member of hashfile from an embedded char array to a pointer? The hashfile structure is initialized once per the entire file (like the on-disk index), and the same buffer will be used throughout the life of that hashfile instance, so it may not be too bad to allocate on the heap. Just after the previous step justified its simplification of its progress logic based on how small the buffer is, this step makes it 16 times as big, which felt a tiny bit dishonest. We probably should say somewhere that 128k is still small enough that the rewrite in the previous step is still valid ;-)
On Mon, May 17, 2021 at 12:24:50PM +0000, Derrick Stolee via GitGitGadget wrote: > From: Derrick Stolee <dstolee@microsoft.com> > > The hashfile API uses a hard-coded buffer size of 8KB and has ever since > it was introduced in c38138c (git-pack-objects: write the pack files > with a SHA1 csum, 2005-06-26). It performs a similar function to the > hashing buffers in read-cache.c, but that code was updated from 8KB to > 128KB in f279894 (read-cache: make the index write buffer size 128K, > 2021-02-18). The justification there was that do_write_index() improves > from 1.02s to 0.72s. > > There is a buffer, check_buffer, that is used to verify the check_fd > file descriptor. When this buffer increases to 128K to fit the data > being flushed, it causes the stack to overflow the limits placed in the > test suite. By moving this to a static buffer, we stop using stack data > for this purpose, but we lose some thread-safety. This change makes it > unsafe to write to multiple hashfiles across different threads. > > By adding a new trace2 region in the chunk-format API, we can see that > the writing portion of 'git multi-pack-index write' lowers from ~1.49s > to ~1.47s on a Linux machine. These effects may be more pronounced or > diminished on other filesystems. The end-to-end timing is too noisy to > have a definitive change either way. I think there is one thing missing from this commit message: why we want to do this. You mentioned that read-cache got larger by using a bigger buffer. But here we use a bigger buffer, and it produces no improvement larger than the noise. And on top of it, you describe the static-buffer downsides. So why not just skip it? :) And the answer is in the larger series: we want to be able to make use of the hashfile API in read-cache, but without regressing the performance. One sentence at the end of the first paragraph would clarify that quite a bit, I think. > +static void verify_buffer_or_die(struct hashfile *f, > + const void *buf, > + unsigned int count) > +{ > + static unsigned char check_buffer[WRITE_BUFFER_SIZE]; > + ssize_t ret = read_in_full(f->check_fd, check_buffer, count); > + > + if (ret < 0) > + die_errno("%s: sha1 file read error", f->name); > + if (ret != count) > + die("%s: sha1 file truncated", f->name); > + if (memcmp(buf, check_buffer, count)) > + die("sha1 file '%s' validation error", f->name); > +} Does this have to use the same-size buffer? We could read and check smaller chunks, like: while (count > 0) { static unsigned char chunk[1024]; unsigned int chunk_len = sizeof(chunk) < count ? sizeof(chunk) : count; ssize_t ret = read_in_full(f->check_fd, chunk, chunk_len); if (ret < 0) ... if (ret != count) ... if (memcmp(buf, chunk, chunk_len)) ... buf += chunk_len; count -= chunk_len; } We may prefer to use the larger buffer size for performance, but I think this "check" mode is only used for "index-pack --verify" and similar. The performance may matter a lot less to us there than for more frequently used code paths like index writing. I don't have a strong preference either way, but it's nice to avoid introducing non-reentrancy to a function (Junio's heap suggestion is also quite reasonable). -Peff
On Tue, May 18, 2021 at 06:54:57AM +0900, Junio C Hamano wrote: > Just after the previous step justified its simplification of its > progress logic based on how small the buffer is, this step makes it > 16 times as big, which felt a tiny bit dishonest. We probably > should say somewhere that 128k is still small enough that the > rewrite in the previous step is still valid ;-) I noticed that, too. I'm not sure if still is small enough. For local pack writes, etc, it seems fine. But what about "index-pack --stdin" reading over the network? Updating progress every 8k instead of every 128k seems like it would be more responsive, especially if the network is slow or jittery. I dunno. Maybe that is too small to care about for the modern world, but I just want to make sure we are not being blinded by the fast networks all of us presumably enjoy. :) -Peff
On Tue, May 18, 2021 at 03:31:28AM -0400, Jeff King wrote: > Does this have to use the same-size buffer? We could read and check > smaller chunks, like: > > while (count > 0) { > static unsigned char chunk[1024]; > unsigned int chunk_len = sizeof(chunk) < count ? sizeof(chunk) : count; > ssize_t ret = read_in_full(f->check_fd, chunk, chunk_len); > > if (ret < 0) > ... > if (ret != count) > ... > if (memcmp(buf, chunk, chunk_len)) > ... > buf += chunk_len; > count -= chunk_len; > } That should be "ret != chunk_len" in the middle conditional, of course. In case you do go this route (I typed this straight into my email, so other bugs may be lurking. But I noticed that one. :) ). -Peff
On 5/18/2021 3:33 AM, Jeff King wrote: > On Tue, May 18, 2021 at 06:54:57AM +0900, Junio C Hamano wrote: > >> Just after the previous step justified its simplification of its >> progress logic based on how small the buffer is, this step makes it >> 16 times as big, which felt a tiny bit dishonest. We probably >> should say somewhere that 128k is still small enough that the >> rewrite in the previous step is still valid ;-) > > I noticed that, too. I'm not sure if still is small enough. For local > pack writes, etc, it seems fine. But what about "index-pack --stdin" > reading over the network? > > Updating progress every 8k instead of every 128k seems like it would be > more responsive, especially if the network is slow or jittery. I dunno. > Maybe that is too small to care about for the modern world, but I just > want to make sure we are not being blinded by the fast networks all of > us presumably enjoy. :) This is a good point. If we combine the earlier suggestion of using the heap to store the buffer, then we can change the buffer size based on the use case: we can use 8k for data that might be streaming from a network, and 128k for local-only data (index, commit-graph, multi-pack-index are all probably safe). For the case of NFS, I think we should probably assume that the NFS server is not across a slow connection, which is a case we cannot make for streaming a pack-file from a remote server. Thanks, -Stolee
diff --git a/chunk-format.c b/chunk-format.c index da191e59a29d..1c3dca62e205 100644 --- a/chunk-format.c +++ b/chunk-format.c @@ -58,9 +58,11 @@ void add_chunk(struct chunkfile *cf, int write_chunkfile(struct chunkfile *cf, void *data) { - int i; + int i, result = 0; uint64_t cur_offset = hashfile_total(cf->f); + trace2_region_enter("chunkfile", "write", the_repository); + /* Add the table of contents to the current offset */ cur_offset += (cf->chunks_nr + 1) * CHUNK_TOC_ENTRY_SIZE; @@ -77,10 +79,10 @@ int write_chunkfile(struct chunkfile *cf, void *data) for (i = 0; i < cf->chunks_nr; i++) { off_t start_offset = hashfile_total(cf->f); - int result = cf->chunks[i].write_fn(cf->f, data); + result = cf->chunks[i].write_fn(cf->f, data); if (result) - return result; + goto cleanup; if (hashfile_total(cf->f) - start_offset != cf->chunks[i].size) BUG("expected to write %"PRId64" bytes to chunk %"PRIx32", but wrote %"PRId64" instead", @@ -88,7 +90,9 @@ int write_chunkfile(struct chunkfile *cf, void *data) hashfile_total(cf->f) - start_offset); } - return 0; +cleanup: + trace2_region_leave("chunkfile", "write", the_repository); + return result; } int read_table_of_contents(struct chunkfile *cf, diff --git a/csum-file.c b/csum-file.c index 3c26389d4914..bd9939c49efa 100644 --- a/csum-file.c +++ b/csum-file.c @@ -11,19 +11,25 @@ #include "progress.h" #include "csum-file.h" +static void verify_buffer_or_die(struct hashfile *f, + const void *buf, + unsigned int count) +{ + static unsigned char check_buffer[WRITE_BUFFER_SIZE]; + ssize_t ret = read_in_full(f->check_fd, check_buffer, count); + + if (ret < 0) + die_errno("%s: sha1 file read error", f->name); + if (ret != count) + die("%s: sha1 file truncated", f->name); + if (memcmp(buf, check_buffer, count)) + die("sha1 file '%s' validation error", f->name); +} + static void flush(struct hashfile *f, const void *buf, unsigned int count) { - if (0 <= f->check_fd && count) { - unsigned char check_buffer[8192]; - ssize_t ret = read_in_full(f->check_fd, check_buffer, count); - - if (ret < 0) - die_errno("%s: sha1 file read error", f->name); - if (ret != count) - die("%s: sha1 file truncated", f->name); - if (memcmp(buf, check_buffer, count)) - die("sha1 file '%s' validation error", f->name); - } + if (0 <= f->check_fd && count) + verify_buffer_or_die(f, buf, count); if (write_in_full(f->fd, buf, count) < 0) { if (errno == ENOSPC) diff --git a/csum-file.h b/csum-file.h index e54d53d1d0b3..bc88eb86fc28 100644 --- a/csum-file.h +++ b/csum-file.h @@ -5,6 +5,8 @@ struct progress; +#define WRITE_BUFFER_SIZE (128 * 1024) + /* A SHA1-protected file */ struct hashfile { int fd; @@ -16,7 +18,7 @@ struct hashfile { const char *name; int do_crc; uint32_t crc32; - unsigned char buffer[8192]; + unsigned char buffer[WRITE_BUFFER_SIZE]; }; /* Checkpoint */