{"thread":{"id":"6094","subject":"[PATCH 2/7] Switch git_mmap to use pread.","startedAt":"2006-12-24T05:45:47Z","lastAt":"2006-12-24T20:34:48Z","messageCount":13,"participants":["Shawn O. Pearce","Johannes Schindelin","Alon Ziv","Shawn Pearce","Linus Torvalds"],"isPatch":true,"patchVersion":1,"patchTotal":7},"messages":[{"id":"30221","messageId":"20061224054547.GB8146@spearce.org","threadId":"6094","inReplyTo":"487c7d0ea81f2f82f330e277e0aea38a66ca7cfe.1166939109.git.spearce@spearce.org","subject":"[PATCH 2/7] Switch git_mmap to use pread.","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2006-12-24T05:45:47Z","receivedAt":"2006-12-24T05:45:47Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Now that Git depends on pread in index-pack its safe to say we can\nalso depend on it within the git_mmap emulation we activate when\nNO_MMAP is set.  On most systems pread should be slightly faster\nthan an lseek/read/lseek sequence as its one system call vs. three\nsystem calls.\n\nWe also now honor EAGAIN and EINTR error codes from pread and\nrestart the prior read.\n\nSigned-off-by: Shawn O. Pearce <spearce@spearce.org>\n---\n compat/mmap.c |   17 ++++-------------\n 1 files changed, 4 insertions(+), 13 deletions(-)\n\ndiff --git a/compat/mmap.c b/compat/mmap.c\nindex bb34c7e..98056f0 100644\n--- a/compat/mmap.c\n+++ b/compat/mmap.c\n@@ -2,17 +2,11 @@\n \n void *git_mmap(void *start, size_t length, int prot, int flags, int fd, off_t offset)\n {\n-\tint n = 0;\n-\toff_t current_offset = lseek(fd, 0, SEEK_CUR);\n+\tsize_t n = 0;\n \n \tif (start != NULL || !(flags & MAP_PRIVATE))\n \t\tdie(\"Invalid usage of mmap when built with NO_MMAP\");\n \n-\tif (lseek(fd, offset, SEEK_SET) < 0) {\n-\t\terrno = EINVAL;\n-\t\treturn MAP_FAILED;\n-\t}\n-\n \tstart = xmalloc(length);\n \tif (start == NULL) {\n \t\terrno = ENOMEM;\n@@ -20,7 +14,7 @@ void *git_mmap(void *start, size_t length, int prot, int flags, int fd, off_t of\n \t}\n \n \twhile (n < length) {\n-\t\tint count = read(fd, start+n, length-n);\n+\t\tssize_t count = pread(fd, start + n, length - n, offset + n);\n \n \t\tif (count == 0) {\n \t\t\tmemset(start+n, 0, length-n);\n@@ -28,6 +22,8 @@ void *git_mmap(void *start, size_t length, int prot, int flags, int fd, off_t of\n \t\t}\n \n \t\tif (count < 0) {\n+\t\t\tif (errno == EAGAIN || errno == EINTR)\n+\t\t\t\tcontinue;\n \t\t\tfree(start);\n \t\t\terrno = EACCES;\n \t\t\treturn MAP_FAILED;\n@@ -36,11 +32,6 @@ void *git_mmap(void *start, size_t length, int prot, int flags, int fd, off_t of\n \t\tn += count;\n \t}\n \n-\tif (current_offset != lseek(fd, current_offset, SEEK_SET)) {\n-\t\terrno = EINVAL;\n-\t\treturn MAP_FAILED;\n-\t}\n-\n \treturn start;\n }\n \n-- \n1.4.4.3.g2e63\n"},{"id":"30222","messageId":"20061224054604.GC8146@spearce.org","threadId":"6094","inReplyTo":"487c7d0ea81f2f82f330e277e0aea38a66ca7cfe.1166939109.git.spearce@spearce.org","subject":"[PATCH 3/7] Ensure packed_git.next is initialized to NULL.","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2006-12-24T05:46:04Z","receivedAt":"2006-12-24T05:46:04Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Junio noticed while reviewing this patch series that I removed the\ninitialization of packed_git.next = NULL in 88078baa.  That removal\nwas not intended so I'm restoring the initialization where necessary.\n\nSigned-off-by: Shawn O. Pearce <spearce@spearce.org>\n---\n sha1_file.c |    2 ++\n 1 files changed, 2 insertions(+), 0 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 1a87f95..8de8ce0 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -676,6 +676,7 @@ struct packed_git *add_packed_git(char *path, int path_len, int local)\n \tp->index_size = idx_size;\n \tp->pack_size = st.st_size;\n \tp->index_base = idx_map;\n+\tp->next = NULL;\n \tp->windows = NULL;\n \tp->pack_fd = -1;\n \tp->pack_local = local;\n@@ -707,6 +708,7 @@ struct packed_git *parse_pack_index_file(const unsigned char *sha1, char *idx_pa\n \tp->index_size = idx_size;\n \tp->pack_size = 0;\n \tp->index_base = idx_map;\n+\tp->next = NULL;\n \tp->windows = NULL;\n \tp->pack_fd = -1;\n \thashcpy(p->sha1, sha1);\n-- \n1.4.4.3.g2e63\n"},{"id":"30223","messageId":"20061224054613.GD8146@spearce.org","threadId":"6094","inReplyTo":"487c7d0ea81f2f82f330e277e0aea38a66ca7cfe.1166939109.git.spearce@spearce.org","subject":"[PATCH 4/7] Default core.packdGitWindowSize to 1 MiB if NO_MMAP.","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2006-12-24T05:46:13Z","receivedAt":"2006-12-24T05:46:13Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"If the compiler has asked us to disable use of mmap() on their\nplatform then we are forced to use git_mmap and its emulation via\npread.  In this case large (e.g. 32 MiB) windows for pack access\nare simply too big as a command will wind up reading a lot more\ndata than it will ever need, significantly reducing response time.\n\nTo prevent a high latency when NO_MMAP has been selected we now\nuse a default of 1 MiB for core.packedGitWindowSize.  Credit goes\nto Linus and Junio for recommending this more reasonable setting.\n\nSigned-off-by: Shawn O. Pearce <spearce@spearce.org>\n---\n environment.c     |    2 +-\n git-compat-util.h |    2 ++\n 2 files changed, 3 insertions(+), 1 deletions(-)\n\ndiff --git a/environment.c b/environment.c\nindex 289fc84..e89aab4 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -22,7 +22,7 @@ char git_commit_encoding[MAX_ENCODING_LENGTH] = \"utf-8\";\n int shared_repository = PERM_UMASK;\n const char *apply_default_whitespace;\n int zlib_compression_level = Z_DEFAULT_COMPRESSION;\n-size_t packed_git_window_size = 32 * 1024 * 1024;\n+size_t packed_git_window_size = DEFAULT_packed_git_window_size;\n size_t packed_git_limit = 256 * 1024 * 1024;\n int pager_in_use;\n int pager_use_color = 1;\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 5d9eb26..e056339 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -87,6 +87,7 @@ extern void set_warn_routine(void (*routine)(const char *warn, va_list params));\n #define MAP_FAILED ((void*)-1)\n #endif\n \n+#define DEFAULT_packed_git_window_size (1 * 1024 * 1024)\n #define mmap git_mmap\n #define munmap git_munmap\n extern void *git_mmap(void *start, size_t length, int prot, int flags, int fd, off_t offset);\n@@ -95,6 +96,7 @@ extern int git_munmap(void *start, size_t length);\n #else /* NO_MMAP */\n \n #include <sys/mman.h>\n+#define DEFAULT_packed_git_window_size (32 * 1024 * 1024)\n \n #endif /* NO_MMAP */\n \n-- \n1.4.4.3.g2e63\n"},{"id":"30224","messageId":"20061224054627.GE8146@spearce.org","threadId":"6094","inReplyTo":"487c7d0ea81f2f82f330e277e0aea38a66ca7cfe.1166939109.git.spearce@spearce.org","subject":"[PATCH 5/7] Don't exit successfully on EPIPE in read_or_die.","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2006-12-24T05:46:27Z","receivedAt":"2006-12-24T05:46:27Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Junio pointed out to me that read() won't return EPIPE like write()\ndoes, so there is no reason for us to exit successfully if we get\nan EPIPE error while in read_or_die.\n\nInstead we should just die if we get any error from read() while\nin read_or_die as there is no reasonable recovery.\n\nSigned-off-by: Shawn O. Pearce <spearce@spearce.org>\n---\n write_or_die.c |    5 +----\n 1 files changed, 1 insertions(+), 4 deletions(-)\n\ndiff --git a/write_or_die.c b/write_or_die.c\nindex a56d992..8cf6486 100644\n--- a/write_or_die.c\n+++ b/write_or_die.c\n@@ -9,11 +9,8 @@ void read_or_die(int fd, void *buf, size_t count)\n \t\tloaded = xread(fd, p, count);\n \t\tif (loaded == 0)\n \t\t\tdie(\"unexpected end of file\");\n-\t\telse if (loaded < 0) {\n-\t\t\tif (errno == EPIPE)\n-\t\t\t\texit(0);\n+\t\telse if (loaded < 0)\n \t\t\tdie(\"read error (%s)\", strerror(errno));\n-\t\t}\n \t\tcount -= loaded;\n \t\tp += loaded;\n \t}\n-- \n1.4.4.3.g2e63\n"},{"id":"30225","messageId":"20061224054719.GF8146@spearce.org","threadId":"6094","inReplyTo":"487c7d0ea81f2f82f330e277e0aea38a66ca7cfe.1166939109.git.spearce@spearce.org","subject":"[PATCH 6/7] Release pack windows before reporting out of memory.","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2006-12-24T05:47:19Z","receivedAt":"2006-12-24T05:47:19Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"If we are about to fail because this process has run out of memory we\nshould first try to automatically control our appetite for address\nspace by releasing enough least-recently-used pack windows to gain\nback enough memory such that we might actually be able to meet the\ncurrent allocation request.\n\nThis should help users who have fairly large repositories but are\nworking on systems with relatively small virtual address space.\nMany times we see reports on the mailing list of these users running\nout of memory during various Git operations.  Dynamically decreasing\nthe amount of pack memory used when the demand for heap memory is\nincreasing is an intelligent solution to this problem.\n\nSigned-off-by: Shawn O. Pearce <spearce@spearce.org>\n---\n git-compat-util.h |   40 ++++++++++++++++++++++++++++++++--------\n sha1_file.c       |    7 +++++++\n 2 files changed, 39 insertions(+), 8 deletions(-)\n\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex e056339..4a417be 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -120,11 +120,17 @@ extern char *gitstrcasestr(const char *haystack, const char *needle);\n extern size_t gitstrlcpy(char *, const char *, size_t);\n #endif\n \n+extern void release_pack_memory(size_t);\n+\n static inline char* xstrdup(const char *str)\n {\n \tchar *ret = strdup(str);\n-\tif (!ret)\n-\t\tdie(\"Out of memory, strdup failed\");\n+\tif (!ret) {\n+\t\trelease_pack_memory(strlen(str) + 1);\n+\t\tret = strdup(str);\n+\t\tif (!ret)\n+\t\t\tdie(\"Out of memory, strdup failed\");\n+\t}\n \treturn ret;\n }\n \n@@ -133,8 +139,14 @@ static inline void *xmalloc(size_t size)\n \tvoid *ret = malloc(size);\n \tif (!ret && !size)\n \t\tret = malloc(1);\n-\tif (!ret)\n-\t\tdie(\"Out of memory, malloc failed\");\n+\tif (!ret) {\n+\t\trelease_pack_memory(size);\n+\t\tret = malloc(size);\n+\t\tif (!ret && !size)\n+\t\t\tret = malloc(1);\n+\t\tif (!ret)\n+\t\t\tdie(\"Out of memory, malloc failed\");\n+\t}\n #ifdef XMALLOC_POISON\n \tmemset(ret, 0xA5, size);\n #endif\n@@ -146,8 +158,14 @@ static inline void *xrealloc(void *ptr, size_t size)\n \tvoid *ret = realloc(ptr, size);\n \tif (!ret && !size)\n \t\tret = realloc(ptr, 1);\n-\tif (!ret)\n-\t\tdie(\"Out of memory, realloc failed\");\n+\tif (!ret) {\n+\t\trelease_pack_memory(size);\n+\t\tret = realloc(ptr, size);\n+\t\tif (!ret && !size)\n+\t\t\tret = realloc(ptr, 1);\n+\t\tif (!ret)\n+\t\t\tdie(\"Out of memory, realloc failed\");\n+\t}\n \treturn ret;\n }\n \n@@ -156,8 +174,14 @@ static inline void *xcalloc(size_t nmemb, size_t size)\n \tvoid *ret = calloc(nmemb, size);\n \tif (!ret && (!nmemb || !size))\n \t\tret = calloc(1, 1);\n-\tif (!ret)\n-\t\tdie(\"Out of memory, calloc failed\");\n+\tif (!ret) {\n+\t\trelease_pack_memory(nmemb * size);\n+\t\tret = calloc(nmemb, size);\n+\t\tif (!ret && (!nmemb || !size))\n+\t\t\tret = calloc(1, 1);\n+\t\tif (!ret)\n+\t\t\tdie(\"Out of memory, calloc failed\");\n+\t}\n \treturn ret;\n }\n \ndiff --git a/sha1_file.c b/sha1_file.c\nindex 8de8ce0..fb1032b 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -522,6 +522,13 @@ static int unuse_one_window(struct packed_git *current)\n \treturn 0;\n }\n \n+void release_pack_memory(size_t need)\n+{\n+\tsize_t cur = pack_mapped;\n+\twhile (need >= (cur - pack_mapped) && unuse_one_window(NULL))\n+\t\t; /* nothing */\n+}\n+\n void unuse_pack(struct pack_window **w_cursor)\n {\n \tstruct pack_window *w = *w_cursor;\n-- \n1.4.4.3.g2e63\n"},{"id":"30226","messageId":"20061224054723.GG8146@spearce.org","threadId":"6094","inReplyTo":"487c7d0ea81f2f82f330e277e0aea38a66ca7cfe.1166939109.git.spearce@spearce.org","subject":"[PATCH 7/7] Replace mmap with xmmap, better handling MAP_FAILED.","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2006-12-24T05:47:23Z","receivedAt":"2006-12-24T05:47:23Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"In some cases we did not even bother to check the return value of\nmmap() and just assume it worked.  This is bad, because if we are\nout of virtual address space the kernel returned MAP_FAILED and we\nwould attempt to dereference that address, segfaulting without any\nreal error output to the user.\n\nWe are replacing all calls to mmap() with xmmap() and moving all\nMAP_FAILED checking into that single location.  If a mmap call\nfails we try to release enough least-recently-used pack windows\nto possibly succeed, then retry the mmap() attempt.  If we cannot\nmmap even after releasing pack memory then we die() as none of our\ncallers have any reasonable recovery strategy for a failed mmap.\n\nSigned-off-by: Shawn O. Pearce <spearce@spearce.org>\n---\n config.c          |    2 +-\n diff.c            |    4 +---\n git-compat-util.h |   13 +++++++++++++\n read-cache.c      |    2 +-\n refs.c            |    2 +-\n sha1_file.c       |   18 +++++-------------\n 6 files changed, 22 insertions(+), 19 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex edc42f4..410d5e9 100644\n--- a/config.c\n+++ b/config.c\n@@ -698,7 +698,7 @@ int git_config_set_multivar(const char* key, const char* value,\n \t\t}\n \n \t\tfstat(in_fd, &st);\n-\t\tcontents = mmap(NULL, st.st_size, PROT_READ,\n+\t\tcontents = xmmap(NULL, st.st_size, PROT_READ,\n \t\t\tMAP_PRIVATE, in_fd, 0);\n \t\tclose(in_fd);\n \ndiff --git a/diff.c b/diff.c\nindex f14288b..244292a 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -1341,10 +1341,8 @@ int diff_populate_filespec(struct diff_filespec *s, int size_only)\n \t\tfd = open(s->path, O_RDONLY);\n \t\tif (fd < 0)\n \t\t\tgoto err_empty;\n-\t\ts->data = mmap(NULL, s->size, PROT_READ, MAP_PRIVATE, fd, 0);\n+\t\ts->data = xmmap(NULL, s->size, PROT_READ, MAP_PRIVATE, fd, 0);\n \t\tclose(fd);\n-\t\tif (s->data == MAP_FAILED)\n-\t\t\tgoto err_empty;\n \t\ts->should_munmap = 1;\n \t}\n \telse {\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 4a417be..51e8d7a 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -185,6 +185,19 @@ static inline void *xcalloc(size_t nmemb, size_t size)\n \treturn ret;\n }\n \n+static inline void *xmmap(void *start, size_t length,\n+\tint prot, int flags, int fd, off_t offset)\n+{\n+\tvoid *ret = mmap(start, length, prot, flags, fd, offset);\n+\tif (ret == MAP_FAILED) {\n+\t\trelease_pack_memory(length);\n+\t\tret = mmap(start, length, prot, flags, fd, offset);\n+\t\tif (ret == MAP_FAILED)\n+\t\t\tdie(\"Out of memory? mmap failed: %s\", strerror(errno));\n+\t}\n+\treturn ret;\n+}\n+\n static inline ssize_t xread(int fd, void *buf, size_t len)\n {\n \tssize_t nr;\ndiff --git a/read-cache.c b/read-cache.c\nindex b8d83cc..ca3efbb 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -798,7 +798,7 @@ int read_cache_from(const char *path)\n \t\tcache_mmap_size = st.st_size;\n \t\terrno = EINVAL;\n \t\tif (cache_mmap_size >= sizeof(struct cache_header) + 20)\n-\t\t\tcache_mmap = mmap(NULL, cache_mmap_size, PROT_READ | PROT_WRITE, MAP_PRIVATE, fd, 0);\n+\t\t\tcache_mmap = xmmap(NULL, cache_mmap_size, PROT_READ | PROT_WRITE, MAP_PRIVATE, fd, 0);\n \t}\n \tclose(fd);\n \tif (cache_mmap == MAP_FAILED)\ndiff --git a/refs.c b/refs.c\nindex a101ff3..286ae45 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1025,7 +1025,7 @@ int read_ref_at(const char *ref, unsigned long at_time, int cnt, unsigned char *\n \tfstat(logfd, &st);\n \tif (!st.st_size)\n \t\tdie(\"Log %s is empty.\", logfile);\n-\tlogdata = mmap(NULL, st.st_size, PROT_READ, MAP_PRIVATE, logfd, 0);\n+\tlogdata = xmmap(NULL, st.st_size, PROT_READ, MAP_PRIVATE, logfd, 0);\n \tclose(logfd);\n \n \tlastrec = NULL;\ndiff --git a/sha1_file.c b/sha1_file.c\nindex fb1032b..84037fe 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -355,10 +355,8 @@ static void read_info_alternates(const char * relative_base, int depth)\n \t\tclose(fd);\n \t\treturn;\n \t}\n-\tmap = mmap(NULL, st.st_size, PROT_READ, MAP_PRIVATE, fd, 0);\n+\tmap = xmmap(NULL, st.st_size, PROT_READ, MAP_PRIVATE, fd, 0);\n \tclose(fd);\n-\tif (map == MAP_FAILED)\n-\t\treturn;\n \n \tlink_alt_odb_entries(map, map + st.st_size, '\\n', relative_base, depth);\n \n@@ -442,10 +440,8 @@ static int check_packed_git_idx(const char *path, unsigned long *idx_size_,\n \t\treturn -1;\n \t}\n \tidx_size = st.st_size;\n-\tidx_map = mmap(NULL, idx_size, PROT_READ, MAP_PRIVATE, fd, 0);\n+\tidx_map = xmmap(NULL, idx_size, PROT_READ, MAP_PRIVATE, fd, 0);\n \tclose(fd);\n-\tif (idx_map == MAP_FAILED)\n-\t\treturn -1;\n \n \tindex = idx_map;\n \t*idx_map_ = idx_map;\n@@ -630,7 +626,7 @@ unsigned char* use_pack(struct packed_git *p,\n \t\t\twhile (packed_git_limit < pack_mapped\n \t\t\t\t&& unuse_one_window(p))\n \t\t\t\t; /* nothing */\n-\t\t\twin->base = mmap(NULL, win->len,\n+\t\t\twin->base = xmmap(NULL, win->len,\n \t\t\t\tPROT_READ, MAP_PRIVATE,\n \t\t\t\tp->pack_fd, win->offset);\n \t\t\tif (win->base == MAP_FAILED)\n@@ -828,10 +824,8 @@ void *map_sha1_file(const unsigned char *sha1, unsigned long *size)\n \t\t */\n \t\tsha1_file_open_flag = 0;\n \t}\n-\tmap = mmap(NULL, st.st_size, PROT_READ, MAP_PRIVATE, fd, 0);\n+\tmap = xmmap(NULL, st.st_size, PROT_READ, MAP_PRIVATE, fd, 0);\n \tclose(fd);\n-\tif (map == MAP_FAILED)\n-\t\treturn NULL;\n \t*size = st.st_size;\n \treturn map;\n }\n@@ -1987,10 +1981,8 @@ int index_fd(unsigned char *sha1, int fd, struct stat *st, int write_object, con\n \n \tbuf = \"\";\n \tif (size)\n-\t\tbuf = mmap(NULL, size, PROT_READ, MAP_PRIVATE, fd, 0);\n+\t\tbuf = xmmap(NULL, size, PROT_READ, MAP_PRIVATE, fd, 0);\n \tclose(fd);\n-\tif (buf == MAP_FAILED)\n-\t\treturn -1;\n \n \tif (!type)\n \t\ttype = blob_type;\n-- \n1.4.4.3.g2e63\n"},{"id":"30237","messageId":"Pine.LNX.4.63.0612241407250.19693@wbgn013.biozentrum.uni-wuerzburg.de","threadId":"6094","inReplyTo":"20061224054547.GB8146@spearce.org","subject":"Re: [PATCH 2/7] Switch git_mmap to use pread.","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2006-12-24T13:09:23Z","receivedAt":"2006-12-24T13:09:23Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sun, 24 Dec 2006, Shawn O. Pearce wrote:\n\n> Now that Git depends on pread in index-pack its safe to say we can\n> also depend on it within the git_mmap emulation we activate when\n> NO_MMAP is set.  On most systems pread should be slightly faster\n> than an lseek/read/lseek sequence as its one system call vs. three\n> system calls.\n\nI don't think it matters much. The _only_ platform we really use NO_MMAP \n(other than for testing) is Windows, and AFAICT it does not have pread(), \nso it is emulated by lseek/read/lseek anyway.\n\nBut it's a cleanup, and it deletes more lines than it adds, so Ack from \nme.\n\nCiao,\nDscho\n"},{"id":"30238","messageId":"Pine.LNX.4.63.0612241409420.19693@wbgn013.biozentrum.uni-wuerzburg.de","threadId":"6094","inReplyTo":"20061224054719.GF8146@spearce.org","subject":"Re: [PATCH 6/7] Release pack windows before reporting out of memory.","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2006-12-24T13:10:32Z","receivedAt":"2006-12-24T13:10:32Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\na very cute idea that having x*alloc() in place you can just pluck in some \ngarbage collection. I like it.\n\nCiao,\nDscho\n"},{"id":"30239","messageId":"Pine.LNX.4.63.0612241410400.19693@wbgn013.biozentrum.uni-wuerzburg.de","threadId":"6094","inReplyTo":"20061224054723.GG8146@spearce.org","subject":"Re: [PATCH 7/7] Replace mmap with xmmap, better handling MAP_FAILED.","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2006-12-24T13:22:48Z","receivedAt":"2006-12-24T13:22:48Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sun, 24 Dec 2006, Shawn O. Pearce wrote:\n\n> diff --git a/diff.c b/diff.c\n> index f14288b..244292a 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -1341,10 +1341,8 @@ int diff_populate_filespec(struct diff_filespec *s, int size_only)\n>  \t\tfd = open(s->path, O_RDONLY);\n>  \t\tif (fd < 0)\n>  \t\t\tgoto err_empty;\n> -\t\ts->data = mmap(NULL, s->size, PROT_READ, MAP_PRIVATE, fd, 0);\n> +\t\ts->data = xmmap(NULL, s->size, PROT_READ, MAP_PRIVATE, fd, 0);\n>  \t\tclose(fd);\n> -\t\tif (s->data == MAP_FAILED)\n> -\t\t\tgoto err_empty;\n\nThe only gripe I have here is that the old code could actually say where \nthe problem occurred (\"cannot read data blob for <blabla>\"), but that \nprobably does not matter so much, now that we can hardly run out of memory \non a decent machine, even using big packfiles.\n\n> diff --git a/read-cache.c b/read-cache.c\n> index b8d83cc..ca3efbb 100644\n> --- a/read-cache.c\n> +++ b/read-cache.c\n> @@ -798,7 +798,7 @@ int read_cache_from(const char *path)\n>  \t\tcache_mmap_size = st.st_size;\n>  \t\terrno = EINVAL;\n>  \t\tif (cache_mmap_size >= sizeof(struct cache_header) + 20)\n> -\t\t\tcache_mmap = mmap(NULL, cache_mmap_size, PROT_READ | PROT_WRITE, MAP_PRIVATE, fd, 0);\n> +\t\t\tcache_mmap = xmmap(NULL, cache_mmap_size, PROT_READ | PROT_WRITE, MAP_PRIVATE, fd, 0);\n>  \t}\n>  \tclose(fd);\n>  \tif (cache_mmap == MAP_FAILED)\n\nThis MAP_FAILED no longer has anything to do with MAP_FAILED, but rather \nwith fstat failed, so you probably want to move that into an else \nconstruct just before \"close(fd);\".\n\nAll in all it is a good change -- for the builtin programs.\n\nBut it is less good for the libification. Maybe it is time for a \ndiscussion about the possible strategies to avoid dying in libgit.a?\n\nCiao,\nDscho\n"},{"id":"30252","messageId":"1166988601.31444.3.camel@bruno.nolaviz.org","threadId":"6094","inReplyTo":"Pine.LNX.4.63.0612241407250.19693@wbgn013.biozentrum.uni-wuerzburg.de","subject":"Re: [PATCH 2/7] Switch git_mmap to use pread.","fromName":"Alon Ziv","fromEmail":"alonz@nolaviz.org","sentAt":"2006-12-24T19:30:01Z","receivedAt":"2006-12-24T19:30:01Z","isPatch":true,"sender":{"key":"alonz@nolaviz.org","avatar":null},"body":"On Sun, 2006-12-24 at 14:09 +0100, Johannes Schindelin wrote:\n> I don't think it matters much. The _only_ platform we really use NO_MMAP \n> (other than for testing) is Windows, and AFAICT it does not have pread(), \n> so it is emulated by lseek/read/lseek anyway.\n> \n\nWell, Win32 does have a pread()-like behavior in its generic ReadFile()\nsystem call (using a curiously-named \"lpOverlapped\" parameter).  I would\nhave expected Cygwin to use it, but unfortunately from the CVS sources\nit appears not to :(\n\n\t-az\n"},{"id":"30253","messageId":"20061224201304.GA631@spearce.org","threadId":"6094","inReplyTo":"Pine.LNX.4.63.0612241407250.19693@wbgn013.biozentrum.uni-wuerzburg.de","subject":"Re: [PATCH 2/7] Switch git_mmap to use pread.","fromName":"Shawn Pearce","fromEmail":"spearce@spearce.org","sentAt":"2006-12-24T20:13:04Z","receivedAt":"2006-12-24T20:13:04Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:\n> On Sun, 24 Dec 2006, Shawn O. Pearce wrote:\n> > Now that Git depends on pread in index-pack its safe to say we can\n> > also depend on it within the git_mmap emulation we activate when\n> > NO_MMAP is set.  On most systems pread should be slightly faster\n> > than an lseek/read/lseek sequence as its one system call vs. three\n> > system calls.\n> \n> I don't think it matters much. The _only_ platform we really use NO_MMAP \n> (other than for testing) is Windows, and AFAICT it does not have pread(), \n> so it is emulated by lseek/read/lseek anyway.\n> \n> But it's a cleanup, and it deletes more lines than it adds, so Ack from \n> me.\n\nRight - that's why I did it.  I was already in there doing other work\nand discovered that we had a lot of lines just to avoid using pread.\nInitially that may have been because we were trying to avoid using\nit, but now that one of our more important tools (index-pack)\ndepends on it... ;-)\n\nI'm actually thinking of doing a NO_PACK_MMAP, especially for systems\nlike Mac OS X where the mmap() of small chunks appears to perform\nso poorly.  Setting NO_PACK_MMAP and just using smaller windows\n(1 MiB) might be worthwhile.  Though in that case we may just want\nto compile with NO_MMAP.\n\n-- \nShawn.\n"},{"id":"30254","messageId":"Pine.LNX.4.64.0612241213320.3671@woody.osdl.org","threadId":"6094","inReplyTo":"Pine.LNX.4.63.0612241407250.19693@wbgn013.biozentrum.uni-wuerzburg.de","subject":"Re: [PATCH 2/7] Switch git_mmap to use pread.","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2006-12-24T20:14:52Z","receivedAt":"2006-12-24T20:14:52Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Sun, 24 Dec 2006, Johannes Schindelin wrote:\n> \n> I don't think it matters much. The _only_ platform we really use NO_MMAP \n> (other than for testing) is Windows, and AFAICT it does not have pread(), \n> so it is emulated by lseek/read/lseek anyway.\n\nWindows definitely has pread(). It is, of course, possible that Cygwin \nemulates it some other way (including doing a \"lseek/read/lseek\" \ncombination), but pread() should still be better - because maybe some \nfuture cygwin release ends up using the native interfaces.\n\n\t\tLinus\n"},{"id":"30255","messageId":"20061224203448.GB631@spearce.org","threadId":"6094","inReplyTo":"Pine.LNX.4.63.0612241410400.19693@wbgn013.biozentrum.uni-wuerzburg.de","subject":"Re: [PATCH 7/7] Replace mmap with xmmap, better handling MAP_FAILED.","fromName":"Shawn Pearce","fromEmail":"spearce@spearce.org","sentAt":"2006-12-24T20:34:48Z","receivedAt":"2006-12-24T20:34:48Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:\n> All in all it is a good change -- for the builtin programs.\n> \n> But it is less good for the libification. Maybe it is time for a \n> discussion about the possible strategies to avoid dying in libgit.a?\n\nWell we have the same problem with xmalloc.  All I've done is move\nthe MAP_FAILED cases which tend to wind up die()'ing later anyway\ninto the same scope of area where the xmalloc issue is.\n\nWe die() all over the place.  ~1312 times according to 'git grep die'.\nGit isn't a program for the living.  :-)\n\nTo properly libify we have a few issues:\n\n  - we cannot just exit this process when we run into an error;\n\n  - routines need to cleanup temporary resources (memory, file\n    descriptors) when returning an error;\n\n  - static variables like environment.c need to be reorganized to\n    support multiple repositories in the same program;\n\n  - objects need to be able to be deallocated, especially\n    if the revision walking machinary has done rewriting\n\nThough the die() is the largest issue.\n\n-- \nShawn.\n"}]}