indexer: Avoid one mmap(2)/munmap(2) pair per git_indexer_append call - #6039
ethomson merged 4 commits into
Conversation
Sorry, something went wrong.
…` call This change makes `append_to_pack` completely rely on `p_pwrite` to do all its I/O instead of splitting it between `p_pwrite` and a `mmap(2)`/`munmap(2)`+`memcpy(3)`. This saves a good chunk of user CPU time and avoids making two syscalls per round, but doesn't really cut down a lot of wall time (~1% on cloning the [git](https://github.com/git/git.git) repository).
|
It seems to me that this will make it possible to remove the p_ftruncate in git_indexer_commit diff --git a/src/indexer.c b/src/indexer.c
index d546888cc..eb56ccc92 100644
--- a/src/indexer.c
+++ b/src/indexer.c
@@ -1279,12 +1279,6 @@ int git_indexer_commit(git_indexer *idx, git_indexer_progress *stats)
if (git_mwindow_free_all(&idx->pack->mwf) < 0)
goto on_error;
- /* Truncate file to undo rounding up to next page_size in append_to_pack */
- if (p_ftruncate(idx->pack->mwf.fd, idx->pack->mwf.size) < 0) {
- git_error_set(GIT_ERROR_OS, "failed to truncate pack file '%s'", idx->pack->pack_name);
- return -1;
- }
-
if (idx->do_fsync && p_fsync(idx->pack->mwf.fd) < 0) {
git_error_set(GIT_ERROR_OS, "failed to fsync packfile");
goto on_error;With that being the only use of ftruncate it can be removed from the posix layer and the tests too, but I don't see any harm in keeping it around in case it is needed in the future. |
Sorry, something went wrong.
Now that we're not using `mmap(2)` for writing stuff, we don't need to truncate the file afterwards, since it'll have the correct size at the end of the process. Whee~!
neat! |
Sorry, something went wrong.
|
This seems reasonable to me. @carlosmn any objections? |
Sorry, something went wrong.
|
This looks fine except this probably re-opens #2338 which is why we're so mmap-heavy. The issue here is that Windows may keep different views to a networked file for the mmap- and open-accessed versions of the file, so writing via write() is not reflected on the data mmap() is able to read. IIUC this is a Windows quirk so you could disable the mmap stuff on non-Windows systems to keep it working there. |
Sorry, something went wrong.
It turns out that if we use `mmap(2)`, non-Windows remote filesystems break due to permissions. If we don't, _Windows_ remote filesystems break due to lack of coherence between memory mapped views of the file and direct I/O operations done to the files. To break out of this impossible situation, conditionally-compile versions of Windows-specific `write_at` and `append_to_pack`.
turns out, supporting multiple platforms is hard! thanks for the pointer, i added a comment to explain the oddness of the code for future spelunkers. i also tested this locally (Linux) with NFS and SMB and they both work! \o/ |
Sorry, something went wrong.
Turns out, double negatives are harder to parse than positive statements.
|
And it also works in AWS-EFS per the other thread \o/². |
Sorry, something went wrong.
This change makes append_to_pack completely rely on p_pwrite to do
all its I/O instead of splitting it between p_pwrite and a
mmap(2)/munmap(2)+memcpy(3). This saves a good chunk of user CPU
time and avoids making two syscalls per round, but doesn't really cut
down a lot of wall time (~1% on cloning the
git repository).
Part of: #6038
Fixes: #5556