Dynamically adjusting packed_git_window_size

Dynamically adjusting packed_git_window_size

[R. Tyler Ballance]

Internally we are using a custom build of Git, and one of the patches
that I apply to newer builds of Git is one to adjust the
DEFAULT_PACKED_GIT_WINDOW_SIZE in git-compat-util.h so Git won't trample
all over our ulimit values on the 64-bit dev machines.

To do away with this, I've got these two (really one) set of patches to
adjust the packed_git_window_size when setup_git_env() is called to a
fraction of the "addressspace" limit (RLIMIT_AS). If the user's
environment defines "ulimit -v" as "unlimited", this code will not take
effect.

It's worth noting that this doesn't force Git to respect these limits,
I'm still tracking down an issue hiding in get_revision() where I'm
experiencing mmap(2) failures executing a `git log` command with
restrictive ulimit settings (Linus, since you were so "pleased" with my
last epic gdb fail, here's today's):

	(gdb)
	open_packed_git (p=0x71f2e0) at sha1_file.c:733
	733             /* We leave these file descriptors open with sliding mmap;
	(gdb)
	735              */
	(gdb)
	741                     return error("cannot set FD_CLOEXEC");
	(gdb)
	746             if (hdr.hdr_signature != htonl(PACK_SIGNATURE))
	(gdb)

	Recursive internal problem.
	[1]    17381 abort      GIT_PAGER= gdb git
	tyler@starfruit:~/source/git/main>

Oi vei.


Cheers,
-R. Tyler Ballance

[PATCH] Add support for changing packed_git_window_size at process start time

[R. Tyler Ballance]

"Works" insofar that it will alter the packed_git_window_size variable in environment.c
when the environment is set up. It /doesn't/ work when commands like git-diff(1) and git-log(1)
call get_revision() which seems to disregard the setting if the packed_window_size is set to something
low (i.e. ulimit -v 32768)

Signed-off-by: R. Tyler Ballance <tyler@slide.com>
---
 environment.c     |   10 ++++++++++
 git-compat-util.h |    4 ++++
 2 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/environment.c b/environment.c
index e278bce..a3b6bab 100644
--- a/environment.c
+++ b/environment.c
@@ -7,6 +7,9 @@
  * even if you might want to know where the git directory etc
  * are.
  */
+#include <sys/time.h>
+#include <sys/resource.h>
+
 #include "cache.h"
 
 char git_default_email[MAX_GITNAME];
@@ -75,6 +78,13 @@ static void setup_git_env(void)
 	git_graft_file = getenv(GRAFT_ENVIRONMENT);
 	if (!git_graft_file)
 		git_graft_file = git_pathdup("info/grafts");
+	
+	if (DYNAMIC_WINDOW_SIZE) {
+		struct rlimit *as = malloc(sizeof(struct rlimit));
+		if ( (getrlimit(RLIMIT_AS, as) == 0) && ((int)(as->rlim_cur) > 0) ) 
+			packed_git_window_size = (unsigned int)(as->rlim_cur * DYNAMIC_WINDOW_SIZE_PERCENTAGE);
+		free(as);
+	}
 }
 
 int is_bare_repository(void)
diff --git a/git-compat-util.h b/git-compat-util.h
index e20b1e8..9603ca6 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -182,6 +182,8 @@ extern int git_munmap(void *start, size_t length);
 
 /* This value must be multiple of (pagesize * 2) */
 #define DEFAULT_PACKED_GIT_WINDOW_SIZE (1 * 1024 * 1024)
+#define DYNAMIC_WINDOW_SIZE 0
+#define DYNAMIC_WINDOW_SIZE_PERCENTAGE 0
 
 #else /* NO_MMAP */
 
@@ -192,6 +194,8 @@ extern int git_munmap(void *start, size_t length);
 	(sizeof(void*) >= 8 \
 		?  1 * 1024 * 1024 * 1024 \
 		: 32 * 1024 * 1024)
+#define DYNAMIC_WINDOW_SIZE 1
+#define DYNAMIC_WINDOW_SIZE_PERCENTAGE 0.85
 
 #endif /* NO_MMAP */
 
-- 

[PATCH] Style changes per suggestions from Junio in #git

[R. Tyler Ballance]

Moving includes into git-compat-util.h, move away from malloc(2)

Signed-off-by: R. Tyler Ballance <tyler@slide.com>
---
 environment.c     |    9 +++------
 git-compat-util.h |    2 ++
 2 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/environment.c b/environment.c
index a3b6bab..aa36360 100644
--- a/environment.c
+++ b/environment.c
@@ -7,8 +7,6 @@
  * even if you might want to know where the git directory etc
  * are.
  */
-#include <sys/time.h>
-#include <sys/resource.h>
 
 #include "cache.h"
 
@@ -80,10 +78,9 @@ static void setup_git_env(void)
 		git_graft_file = git_pathdup("info/grafts");
 	
 	if (DYNAMIC_WINDOW_SIZE) {
-		struct rlimit *as = malloc(sizeof(struct rlimit));
-		if ( (getrlimit(RLIMIT_AS, as) == 0) && ((int)(as->rlim_cur) > 0) ) 
-			packed_git_window_size = (unsigned int)(as->rlim_cur * DYNAMIC_WINDOW_SIZE_PERCENTAGE);
-		free(as);
+		struct rlimit as;
+		if (getrlimit(RLIMIT_AS, &as) == 0 && (int)as.rlim_cur > 0)
+			packed_git_window_size = (unsigned int)(as.rlim_cur * DYNAMIC_WINDOW_SIZE_PERCENTAGE);
 	}
 }
 
diff --git a/git-compat-util.h b/git-compat-util.h
index 9603ca6..dad2dc8 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -188,6 +188,8 @@ extern int git_munmap(void *start, size_t length);
 #else /* NO_MMAP */
 
 #include <sys/mman.h>
+#include <sys/time.h>
+#include <sys/resource.h>
 
 /* This value must be multiple of (pagesize * 2) */
 #define DEFAULT_PACKED_GIT_WINDOW_SIZE \
-- 

Re: [PATCH] Style changes per suggestions from Junio in #git

[Matthieu Moy]

"R. Tyler Ballance" <tyler@slide.com> writes:

> Moving includes into git-compat-util.h, move away from malloc(2)

Usually, those cleanup patches are merged with the actual patch before
inclusion. This helps review (i.e. let reviewers not have to say or
think "you shouldn't do that - oh, ok, you're actually not doing it"),
and avoids having bad commits at all in Junio's repository.

-- 
Matthieu

Re: [PATCH] Style changes per suggestions from Junio in #git

[Shawn O. Pearce]

"R. Tyler Ballance" <tyler@slide.com> wrote:
> Moving includes into git-compat-util.h, move away from malloc(2)

Obviously this should just be squashed into the prior patch.
 

-- 
Shawn.

Re: [PATCH] Add support for changing packed_git_window_size at process start time

[Shawn O. Pearce]

"R. Tyler Ballance" <tyler@slide.com> wrote:
> "Works" insofar that it will alter the packed_git_window_size variable in environment.c
> when the environment is set up. It /doesn't/ work when commands like git-diff(1) and git-log(1)
> call get_revision() which seems to disregard the setting if the packed_window_size is set to something
> low (i.e. ulimit -v 32768)

I think you are tweaking the wrong variable here.  Its more than
just the window size that matters to how much of the ulimit we use.
Its also packed_git_limit, which on a 64 bit system is 8 GB.

That's probably why get_revision doesn't seem to honor this as
a setting.  Yea, its down to 0.85% of your ulimit per window,
but we'll still try to open a new window because we have space
left before the 8 GB limit.

I think this is a good idea, trying to fit within the ulimit
rather than assuming we can take whatever we please.  But you
also need to drop the packed_git_limit down.

My suggestion is this:

	packed_git_limit = as->rlim_cur * 0.85;
	packed_git_window_size = packed_git_limit / 4;

or maybe / 2.  You really want at least 2 windows available within
your limit.
 
> @@ -75,6 +78,13 @@ static void setup_git_env(void)
>  	git_graft_file = getenv(GRAFT_ENVIRONMENT);
>  	if (!git_graft_file)
>  		git_graft_file = git_pathdup("info/grafts");
> +	
> +	if (DYNAMIC_WINDOW_SIZE) {
> +		struct rlimit *as = malloc(sizeof(struct rlimit));
> +		if ( (getrlimit(RLIMIT_AS, as) == 0) && ((int)(as->rlim_cur) > 0) ) 
> +			packed_git_window_size = (unsigned int)(as->rlim_cur * DYNAMIC_WINDOW_SIZE_PERCENTAGE);
> +		free(as);
> +	}
>  }

-- 
Shawn.

Re: [PATCH] Add support for changing packed_git_window_size at process start time

[R. Tyler Ballance]

[-- Attachment #1: Type: text/plain, Size: 1772 bytes --]

On Sun, 2008-12-21 at 14:28 -0800, Shawn O. Pearce wrote:
> 
> I think this is a good idea, trying to fit within the ulimit
> rather than assuming we can take whatever we please.  But you
> also need to drop the packed_git_limit down.
> 
> My suggestion is this:
> 
> 	packed_git_limit = as->rlim_cur * 0.85;
> 	packed_git_window_size = packed_git_limit / 4;
> 
> or maybe / 2.  You really want at least 2 windows available within
> your limit.

Ah, gotcha, sounds like a good idea. I went ahead and added the change
and I'm still getting the memory issues. 

I'm not as familiar with using gdb(1), so I'm having trouble tracking
down the issue in a limited session, I get loads of issues like the
following when trying to step through an execution of `git log`


        1368            if (diff_setup_done(&revs->diffopt) < 0)
        (gdb) 
        utils.c:1065: internal-error: virtual memory exhausted: can't
        allocate 4064 bytes.
        A problem internal to GDB has been detected,
        further debugging may prove unreliable.
        Quit this debugging session? (y or n) n
        utils.c:1065: internal-error: virtual memory exhausted: can't
        allocate 4064 bytes.
        A problem internal to GDB has been detected,
        further debugging may prove unreliable.
        Create a core file of GDB? (y or n) y
        (gdb) q
        The program is running.  Quit anyway (and kill it)? (y or n) y
        tyler@starfruit:~/source/git/main>


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?

Appreciate the help :)


Cheers
-- 
-R. Tyler Ballance
Slide, Inc.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] Add support for changing packed_git_window_size at process start time

[Shawn O. Pearce]

"R. Tyler Ballance" <tyler@slide.com> wrote:
> Ah, gotcha, sounds like a good idea. I went ahead and added the change
> and I'm still getting the memory issues. 

:-|
 
> I'm not as familiar with using gdb(1), so I'm having trouble tracking
> down the issue in a limited session, I get loads of issues like the
> following when trying to step through an execution of `git log`
> 
> 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?

Look at xmmap in git's code.  All of our mmap calls go through that
function and try to release pack windows if we get an error back
from mmap(), then it retries the mmap request.  xmalloc likewise
does the same thing for malloc requests; xcalloc for calloc, xstrdup
for strdup.  We have a number of these x variant functions to handle
memory allocation.

You might be be able to put a setrlimit call into main() in git.c to
drop the rlimit for Git to a lower limit than it inherited from gdb,
allowing you to start gdb with a much higher ulimit so it doesn't
barf when trying to inspect the git child.
 
-- 
Shawn.