{"thread":{"id":"16822","subject":"[PATCH] Add support for changing packed_git_window_size at process start time","startedAt":"2008-12-21T21:37:32Z","lastAt":"2008-12-26T21:36:04Z","messageCount":8,"participants":["R. Tyler Ballance","Matthieu Moy","Shawn O. Pearce"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"98502","messageId":"1229895454-19498-1-git-send-email-tyler@slide.com","threadId":"16822","inReplyTo":null,"subject":"Dynamically adjusting packed_git_window_size","fromName":"R. Tyler Ballance","fromEmail":"tyler@slide.com","sentAt":"2008-12-21T21:37:32Z","receivedAt":"2008-12-21T21:37:32Z","isPatch":false,"sender":{"key":"tyler@slide.com","avatar":null},"body":"Internally we are using a custom build of Git, and one of the patches\nthat I apply to newer builds of Git is one to adjust the\nDEFAULT_PACKED_GIT_WINDOW_SIZE in git-compat-util.h so Git won't trample\nall over our ulimit values on the 64-bit dev machines.\n\nTo do away with this, I've got these two (really one) set of patches to\nadjust the packed_git_window_size when setup_git_env() is called to a\nfraction of the \"addressspace\" limit (RLIMIT_AS). If the user's\nenvironment defines \"ulimit -v\" as \"unlimited\", this code will not take\neffect.\n\nIt's worth noting that this doesn't force Git to respect these limits,\nI'm still tracking down an issue hiding in get_revision() where I'm\nexperiencing mmap(2) failures executing a `git log` command with\nrestrictive ulimit settings (Linus, since you were so \"pleased\" with my\nlast epic gdb fail, here's today's):\n\n\t(gdb)\n\topen_packed_git (p=0x71f2e0) at sha1_file.c:733\n\t733             /* We leave these file descriptors open with sliding mmap;\n\t(gdb)\n\t735              */\n\t(gdb)\n\t741                     return error(\"cannot set FD_CLOEXEC\");\n\t(gdb)\n\t746             if (hdr.hdr_signature != htonl(PACK_SIGNATURE))\n\t(gdb)\n\n\tRecursive internal problem.\n\t[1]    17381 abort      GIT_PAGER= gdb git\n\ttyler@starfruit:~/source/git/main>\n\nOi vei.\n\n\nCheers,\n-R. Tyler Ballance\n"},{"id":"98501","messageId":"1229895454-19498-2-git-send-email-tyler@slide.com","threadId":"16822","inReplyTo":"1229895454-19498-1-git-send-email-tyler@slide.com","subject":"[PATCH] Add support for changing packed_git_window_size at process start time","fromName":"R. Tyler Ballance","fromEmail":"tyler@slide.com","sentAt":"2008-12-21T21:37:33Z","receivedAt":"2008-12-21T21:37:33Z","isPatch":true,"sender":{"key":"tyler@slide.com","avatar":null},"body":"\"Works\" insofar that it will alter the packed_git_window_size variable in environment.c\nwhen the environment is set up. It /doesn't/ work when commands like git-diff(1) and git-log(1)\ncall get_revision() which seems to disregard the setting if the packed_window_size is set to something\nlow (i.e. ulimit -v 32768)\n\nSigned-off-by: R. Tyler Ballance <tyler@slide.com>\n---\n environment.c     |   10 ++++++++++\n git-compat-util.h |    4 ++++\n 2 files changed, 14 insertions(+), 0 deletions(-)\n\ndiff --git a/environment.c b/environment.c\nindex e278bce..a3b6bab 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -7,6 +7,9 @@\n  * even if you might want to know where the git directory etc\n  * are.\n  */\n+#include <sys/time.h>\n+#include <sys/resource.h>\n+\n #include \"cache.h\"\n \n char git_default_email[MAX_GITNAME];\n@@ -75,6 +78,13 @@ static void setup_git_env(void)\n \tgit_graft_file = getenv(GRAFT_ENVIRONMENT);\n \tif (!git_graft_file)\n \t\tgit_graft_file = git_pathdup(\"info/grafts\");\n+\t\n+\tif (DYNAMIC_WINDOW_SIZE) {\n+\t\tstruct rlimit *as = malloc(sizeof(struct rlimit));\n+\t\tif ( (getrlimit(RLIMIT_AS, as) == 0) && ((int)(as->rlim_cur) > 0) ) \n+\t\t\tpacked_git_window_size = (unsigned int)(as->rlim_cur * DYNAMIC_WINDOW_SIZE_PERCENTAGE);\n+\t\tfree(as);\n+\t}\n }\n \n int is_bare_repository(void)\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex e20b1e8..9603ca6 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -182,6 +182,8 @@ extern int git_munmap(void *start, size_t length);\n \n /* This value must be multiple of (pagesize * 2) */\n #define DEFAULT_PACKED_GIT_WINDOW_SIZE (1 * 1024 * 1024)\n+#define DYNAMIC_WINDOW_SIZE 0\n+#define DYNAMIC_WINDOW_SIZE_PERCENTAGE 0\n \n #else /* NO_MMAP */\n \n@@ -192,6 +194,8 @@ extern int git_munmap(void *start, size_t length);\n \t(sizeof(void*) >= 8 \\\n \t\t?  1 * 1024 * 1024 * 1024 \\\n \t\t: 32 * 1024 * 1024)\n+#define DYNAMIC_WINDOW_SIZE 1\n+#define DYNAMIC_WINDOW_SIZE_PERCENTAGE 0.85\n \n #endif /* NO_MMAP */\n \n-- \n"},{"id":"98503","messageId":"1229895454-19498-3-git-send-email-tyler@slide.com","threadId":"16822","inReplyTo":"1229895454-19498-2-git-send-email-tyler@slide.com","subject":"[PATCH] Style changes per suggestions from Junio in #git","fromName":"R. Tyler Ballance","fromEmail":"tyler@slide.com","sentAt":"2008-12-21T21:37:34Z","receivedAt":"2008-12-21T21:37:34Z","isPatch":true,"sender":{"key":"tyler@slide.com","avatar":null},"body":"Moving includes into git-compat-util.h, move away from malloc(2)\n\nSigned-off-by: R. Tyler Ballance <tyler@slide.com>\n---\n environment.c     |    9 +++------\n git-compat-util.h |    2 ++\n 2 files changed, 5 insertions(+), 6 deletions(-)\n\ndiff --git a/environment.c b/environment.c\nindex a3b6bab..aa36360 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -7,8 +7,6 @@\n  * even if you might want to know where the git directory etc\n  * are.\n  */\n-#include <sys/time.h>\n-#include <sys/resource.h>\n \n #include \"cache.h\"\n \n@@ -80,10 +78,9 @@ static void setup_git_env(void)\n \t\tgit_graft_file = git_pathdup(\"info/grafts\");\n \t\n \tif (DYNAMIC_WINDOW_SIZE) {\n-\t\tstruct rlimit *as = malloc(sizeof(struct rlimit));\n-\t\tif ( (getrlimit(RLIMIT_AS, as) == 0) && ((int)(as->rlim_cur) > 0) ) \n-\t\t\tpacked_git_window_size = (unsigned int)(as->rlim_cur * DYNAMIC_WINDOW_SIZE_PERCENTAGE);\n-\t\tfree(as);\n+\t\tstruct rlimit as;\n+\t\tif (getrlimit(RLIMIT_AS, &as) == 0 && (int)as.rlim_cur > 0)\n+\t\t\tpacked_git_window_size = (unsigned int)(as.rlim_cur * DYNAMIC_WINDOW_SIZE_PERCENTAGE);\n \t}\n }\n \ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 9603ca6..dad2dc8 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -188,6 +188,8 @@ extern int git_munmap(void *start, size_t length);\n #else /* NO_MMAP */\n \n #include <sys/mman.h>\n+#include <sys/time.h>\n+#include <sys/resource.h>\n \n /* This value must be multiple of (pagesize * 2) */\n #define DEFAULT_PACKED_GIT_WINDOW_SIZE \\\n-- \n"},{"id":"98505","messageId":"vpqbpv5mbie.fsf@bauges.imag.fr","threadId":"16822","inReplyTo":"1229895454-19498-3-git-send-email-tyler@slide.com","subject":"Re: [PATCH] Style changes per suggestions from Junio in #git","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@imag.fr","sentAt":"2008-12-21T22:03:37Z","receivedAt":"2008-12-21T22:03:37Z","isPatch":true,"sender":{"key":"git@matthieu-moy.fr","avatar":"https://avatars.githubusercontent.com/u/14709?v=4"},"body":"\"R. Tyler Ballance\" <tyler@slide.com> writes:\n\n> Moving includes into git-compat-util.h, move away from malloc(2)\n\nUsually, those cleanup patches are merged with the actual patch before\ninclusion. This helps review (i.e. let reviewers not have to say or\nthink \"you shouldn't do that - oh, ok, you're actually not doing it\"),\nand avoids having bad commits at all in Junio's repository.\n\n-- \nMatthieu\n"},{"id":"98512","messageId":"20081221222207.GD17355@spearce.org","threadId":"16822","inReplyTo":"1229895454-19498-3-git-send-email-tyler@slide.com","subject":"Re: [PATCH] Style changes per suggestions from Junio in #git","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2008-12-21T22:22:07Z","receivedAt":"2008-12-21T22:22:07Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"\"R. Tyler Ballance\" <tyler@slide.com> wrote:\n> Moving includes into git-compat-util.h, move away from malloc(2)\n\nObviously this should just be squashed into the prior patch.\n \n\n-- \nShawn.\n"},{"id":"98514","messageId":"20081221222848.GE17355@spearce.org","threadId":"16822","inReplyTo":"1229895454-19498-2-git-send-email-tyler@slide.com","subject":"Re: [PATCH] Add support for changing packed_git_window_size at process start time","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2008-12-21T22:28:48Z","receivedAt":"2008-12-21T22:28:48Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"\"R. Tyler Ballance\" <tyler@slide.com> wrote:\n> \"Works\" insofar that it will alter the packed_git_window_size variable in environment.c\n> when the environment is set up. It /doesn't/ work when commands like git-diff(1) and git-log(1)\n> call get_revision() which seems to disregard the setting if the packed_window_size is set to something\n> low (i.e. ulimit -v 32768)\n\nI think you are tweaking the wrong variable here.  Its more than\njust the window size that matters to how much of the ulimit we use.\nIts also packed_git_limit, which on a 64 bit system is 8 GB.\n\nThat's probably why get_revision doesn't seem to honor this as\na setting.  Yea, its down to 0.85% of your ulimit per window,\nbut we'll still try to open a new window because we have space\nleft before the 8 GB limit.\n\nI think this is a good idea, trying to fit within the ulimit\nrather than assuming we can take whatever we please.  But you\nalso need to drop the packed_git_limit down.\n\nMy suggestion is this:\n\n\tpacked_git_limit = as->rlim_cur * 0.85;\n\tpacked_git_window_size = packed_git_limit / 4;\n\nor maybe / 2.  You really want at least 2 windows available within\nyour limit.\n \n> @@ -75,6 +78,13 @@ static void setup_git_env(void)\n>  \tgit_graft_file = getenv(GRAFT_ENVIRONMENT);\n>  \tif (!git_graft_file)\n>  \t\tgit_graft_file = git_pathdup(\"info/grafts\");\n> +\t\n> +\tif (DYNAMIC_WINDOW_SIZE) {\n> +\t\tstruct rlimit *as = malloc(sizeof(struct rlimit));\n> +\t\tif ( (getrlimit(RLIMIT_AS, as) == 0) && ((int)(as->rlim_cur) > 0) ) \n> +\t\t\tpacked_git_window_size = (unsigned int)(as->rlim_cur * DYNAMIC_WINDOW_SIZE_PERCENTAGE);\n> +\t\tfree(as);\n> +\t}\n>  }\n\n-- \nShawn.\n"},{"id":"98535","messageId":"1229927143.14882.17.camel@starfruit","threadId":"16822","inReplyTo":"20081221222848.GE17355@spearce.org","subject":"Re: [PATCH] Add support for changing packed_git_window_size at process start time","fromName":"R. Tyler Ballance","fromEmail":"tyler@slide.com","sentAt":"2008-12-22T06:25:43Z","receivedAt":"2008-12-22T06:25:43Z","isPatch":true,"sender":{"key":"tyler@slide.com","avatar":null},"body":"On Sun, 2008-12-21 at 14:28 -0800, Shawn O. Pearce wrote:\n> \n> I think this is a good idea, trying to fit within the ulimit\n> rather than assuming we can take whatever we please.  But you\n> also need to drop the packed_git_limit down.\n> \n> My suggestion is this:\n> \n> \tpacked_git_limit = as->rlim_cur * 0.85;\n> \tpacked_git_window_size = packed_git_limit / 4;\n> \n> or maybe / 2.  You really want at least 2 windows available within\n> your limit.\n\nAh, gotcha, sounds like a good idea. I went ahead and added the change\nand I'm still getting the memory issues. \n\nI'm not as familiar with using gdb(1), so I'm having trouble tracking\ndown the issue in a limited session, I get loads of issues like the\nfollowing when trying to step through an execution of `git log`\n\n\n        1368            if (diff_setup_done(&revs->diffopt) < 0)\n        (gdb) \n        utils.c:1065: internal-error: virtual memory exhausted: can't\n        allocate 4064 bytes.\n        A problem internal to GDB has been detected,\n        further debugging may prove unreliable.\n        Quit this debugging session? (y or n) n\n        utils.c:1065: internal-error: virtual memory exhausted: can't\n        allocate 4064 bytes.\n        A problem internal to GDB has been detected,\n        further debugging may prove unreliable.\n        Create a core file of GDB? (y or n) y\n        (gdb) q\n        The program is running.  Quit anyway (and kill it)? (y or n) y\n        tyler@starfruit:~/source/git/main>\n\n\nIs there a means in which I can cause a core dump on an ENOMEM error passed back from mmap(2)? That or a way to impose these limits on the gdb git-subprocess but not on the gdb process?\n\nAppreciate the help :)\n\n\nCheers\n-- \n-R. Tyler Ballance\nSlide, Inc.\n"},{"id":"98741","messageId":"20081226213604.GA20356@spearce.org","threadId":"16822","inReplyTo":"1229927143.14882.17.camel@starfruit","subject":"Re: [PATCH] Add support for changing packed_git_window_size at process start time","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2008-12-26T21:36:04Z","receivedAt":"2008-12-26T21:36:04Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"\"R. Tyler Ballance\" <tyler@slide.com> wrote:\n> Ah, gotcha, sounds like a good idea. I went ahead and added the change\n> and I'm still getting the memory issues. \n\n:-|\n \n> I'm not as familiar with using gdb(1), so I'm having trouble tracking\n> down the issue in a limited session, I get loads of issues like the\n> following when trying to step through an execution of `git log`\n> \n> Is there a means in which I can cause a core dump on an ENOMEM error passed back from mmap(2)? That or a way to impose these limits on the gdb git-subprocess but not on the gdb process?\n\nLook at xmmap in git's code.  All of our mmap calls go through that\nfunction and try to release pack windows if we get an error back\nfrom mmap(), then it retries the mmap request.  xmalloc likewise\ndoes the same thing for malloc requests; xcalloc for calloc, xstrdup\nfor strdup.  We have a number of these x variant functions to handle\nmemory allocation.\n\nYou might be be able to put a setrlimit call into main() in git.c to\ndrop the rlimit for Git to a lower limit than it inherited from gdb,\nallowing you to start gdb with a much higher ulimit so it doesn't\nbarf when trying to inspect the git child.\n \n-- \nShawn.\n"}]}