threads / patch / 14029

v2Make git_dir a path relative to work_tree in setup_work_tree()

Subject: [PATCH v2] Make git_dir a path relative to work_tree in setup_work_tree()

## tl;dr

7 messages between Jun 19, 2008 and Jun 19, 2008. Diffs are folded; open one to read it.

replies: 6people: 4as markdown or json

Daniel Barkalow· Jun 19, 2008, 03:28 UTC · lore

Once we find the absolute paths for git_dir and work_tree, we can make git_dir a relative path since we know pwd will be work_tree. This should save the kernel some time traversing the path to work_tree all the time if git_dir is inside work_tree.

Signed-off-by: Daniel Barkalow <barkalow@iabervon.org>
---
Not only was work_tree potentially NULL, it was also already a copied 
canonical path. So this simpler patch should be better.
 cache.h |    1 +
 path.c  |   17 +++++++++++++++++
 setup.c |    3 ++-
 3 files changed, 20 insertions(+), 1 deletions(-)
Show changes to 3 files +20 −1

cache.h, path.c, setup.c

diff --git a/cache.h b/cache.h
index eab1a17..3465c94 100644
--- a/cache.h
+++ b/cache.h
@@ -524,6 +524,7 @@ static inline int is_absolute_path(const char *path)
 	return path[0] == '/';
 }
 const char *make_absolute_path(const char *path);
+const char *make_relative_path(const char *abs, const char *base);
 
 /* Read and unpack a sha1 file into memory, write memory to a sha1 file */
 extern int sha1_object_info(const unsigned char *, unsigned long *);
diff --git a/path.c b/path.c
index b7c24a2..790d8d4 100644
--- a/path.c
+++ b/path.c
@@ -294,6 +294,23 @@ int adjust_shared_perm(const char *path)
 /* We allow "recursive" symbolic links. Only within reason, though. */
 #define MAXDEPTH 5
 
+const char *make_relative_path(const char *abs, const char *base)
+{
+	static char buf[PATH_MAX + 1];
+	int baselen;
+	if (!base)
+		return abs;
+	baselen = strlen(base);
+	if (prefixcmp(abs, base))
+		return abs;
+	if (abs[baselen] == '/')
+		baselen++;
+	else if (base[baselen - 1] != '/')
+		return abs;
+	strcpy(buf, abs + baselen);
+	return buf;
+}
+
 const char *make_absolute_path(const char *path)
 {
 	static char bufs[2][PATH_MAX + 1], *buf = bufs[0], *next_buf = bufs[1];
diff --git a/setup.c b/setup.c
index d630e37..1643ee4 100644
--- a/setup.c
+++ b/setup.c
@@ -292,7 +292,8 @@ void setup_work_tree(void)
 	work_tree = get_git_work_tree();
 	git_dir = get_git_dir();
 	if (!is_absolute_path(git_dir))
-		set_git_dir(make_absolute_path(git_dir));
+		set_git_dir(make_relative_path(make_absolute_path(git_dir),
+					       work_tree));
 	if (!work_tree || chdir(work_tree))
 		die("This operation must be run in a work tree");
 	initialized = 1;

-- 
1.5.4.5
Johannes Schindelin· Jun 19, 2008, 14:11 UTC · re: Daniel Barkalow · lore

Re: [PATCH v2] Make git_dir a path relative to work_tree in setup_work_tree()

Hi,
On Wed, 18 Jun 2008, Daniel Barkalow wrote:
Show 8 quoted lines
> diff --git a/setup.c b/setup.c
> index d630e37..1643ee4 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -292,7 +292,8 @@ void setup_work_tree(void)
>  	work_tree = get_git_work_tree();
>  	git_dir = get_git_dir();
>  	if (!is_absolute_path(git_dir))
I suspect it needs "work_tree &&" here.
Show 6 quoted lines
> -		set_git_dir(make_absolute_path(git_dir));
> +		set_git_dir(make_relative_path(make_absolute_path(git_dir),
> +					       work_tree));
>  	if (!work_tree || chdir(work_tree))
>  		die("This operation must be run in a work tree");
>  	initialized = 1;

All in all I am pretty surprised how easy it was. I tried yesterday, for half an hour, to come up with something sensible, and failed.

Thanks, Dscho

Daniel Barkalow· Jun 19, 2008, 16:12 UTC · re: Johannes Schindelin · lore

Re: [PATCH v2] Make git_dir a path relative to work_tree in setup_work_tree()

On Thu, 19 Jun 2008, Johannes Schindelin wrote:
Show 14 quoted lines
> Hi,
> 
> On Wed, 18 Jun 2008, Daniel Barkalow wrote:
> 
> > diff --git a/setup.c b/setup.c
> > index d630e37..1643ee4 100644
> > --- a/setup.c
> > +++ b/setup.c
> > @@ -292,7 +292,8 @@ void setup_work_tree(void)
> >  	work_tree = get_git_work_tree();
> >  	git_dir = get_git_dir();
> >  	if (!is_absolute_path(git_dir))
> 
> I suspect it needs "work_tree &&" here.

I'm not clear on the semantics of !get_git_work_tree(); is a non-absolute path for git_dir right then?

Show 9 quoted lines
> > -		set_git_dir(make_absolute_path(git_dir));
> > +		set_git_dir(make_relative_path(make_absolute_path(git_dir),
> > +					       work_tree));
> >  	if (!work_tree || chdir(work_tree))
> >  		die("This operation must be run in a work tree");
> >  	initialized = 1;
> 
> All in all I am pretty surprised how easy it was.  I tried yesterday, for 
> half an hour, to come up with something sensible, and failed.

I was sure you'd come up with just this solution, because you'd just recently explained that make_absolute_path() means you can find when one path is in another path with a simple string compare. And, since we know what pwd is going to be...

	-Daniel
*This .sig left intentionally blank*
Johannes Schindelin· Jun 19, 2008, 17:09 UTC · re: Daniel Barkalow · lore

Re: [PATCH v2] Make git_dir a path relative to work_tree in setup_work_tree()

Hi,
On Thu, 19 Jun 2008, Daniel Barkalow wrote:
Show 17 quoted lines
> On Thu, 19 Jun 2008, Johannes Schindelin wrote:
> 
> > On Wed, 18 Jun 2008, Daniel Barkalow wrote:
> > 
> > > diff --git a/setup.c b/setup.c
> > > index d630e37..1643ee4 100644
> > > --- a/setup.c
> > > +++ b/setup.c
> > > @@ -292,7 +292,8 @@ void setup_work_tree(void)
> > >  	work_tree = get_git_work_tree();
> > >  	git_dir = get_git_dir();
> > >  	if (!is_absolute_path(git_dir))
> > 
> > I suspect it needs "work_tree &&" here.
> 
> I'm not clear on the semantics of !get_git_work_tree(); is a non-absolute 
> path for git_dir right then?

My reading was: if there is no work_tree, then a relative git_dir is just fine, since we are quite unlikely to jump around in the file system.

And your implementation of make_relative_path() is nice enough to a (work_tree ==) base == NULL, but would return the absolute path in that case.

Haven't had time to test anything, though.

Ciao, Dscho

Junio C Hamano· Jun 19, 2008, 18:24 UTC · re: Daniel Barkalow · lore

Re: [PATCH v2] Make git_dir a path relative to work_tree in setup_work_tree()

Daniel Barkalow <barkalow@iabervon.org> writes:
Show 14 quoted lines
> diff --git a/path.c b/path.c
> index b7c24a2..790d8d4 100644
> --- a/path.c
> +++ b/path.c
> @@ -294,6 +294,23 @@ int adjust_shared_perm(const char *path)
>  /* We allow "recursive" symbolic links. Only within reason, though. */
>  #define MAXDEPTH 5
>  
> +const char *make_relative_path(const char *abs, const char *base)
> +{
> +	static char buf[PATH_MAX + 1];
> +	int baselen;
> +	if (!base)
> +		return abs;

This special case may help the specific caller you have below, but doesn't it make the function do more than it advertises with its name?

Other than that, I think the change is Ok, but as a "performance tweak", it should be backed by some numbers, please?

Show 30 quoted lines
> +	baselen = strlen(base);
> +	if (prefixcmp(abs, base))
> +		return abs;
> +	if (abs[baselen] == '/')
> +		baselen++;
> +	else if (base[baselen - 1] != '/')
> +		return abs;
> +	strcpy(buf, abs + baselen);
> +	return buf;
> +}
> +
>  const char *make_absolute_path(const char *path)
>  {
>  	static char bufs[2][PATH_MAX + 1], *buf = bufs[0], *next_buf = bufs[1];
> diff --git a/setup.c b/setup.c
> index d630e37..1643ee4 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -292,7 +292,8 @@ void setup_work_tree(void)
>  	work_tree = get_git_work_tree();
>  	git_dir = get_git_dir();
>  	if (!is_absolute_path(git_dir))
> -		set_git_dir(make_absolute_path(git_dir));
> +		set_git_dir(make_relative_path(make_absolute_path(git_dir),
> +					       work_tree));
>  	if (!work_tree || chdir(work_tree))
>  		die("This operation must be run in a work tree");
>  	initialized = 1;
> -- 
> 1.5.4.5
Daniel Barkalow· Jun 19, 2008, 18:44 UTC · re: Junio C Hamano · lore

Re: [PATCH v2] Make git_dir a path relative to work_tree in setup_work_tree()

On Thu, 19 Jun 2008, Junio C Hamano wrote:
Show 19 quoted lines
> Daniel Barkalow <barkalow@iabervon.org> writes:
> 
> > diff --git a/path.c b/path.c
> > index b7c24a2..790d8d4 100644
> > --- a/path.c
> > +++ b/path.c
> > @@ -294,6 +294,23 @@ int adjust_shared_perm(const char *path)
> >  /* We allow "recursive" symbolic links. Only within reason, though. */
> >  #define MAXDEPTH 5
> >  
> > +const char *make_relative_path(const char *abs, const char *base)
> > +{
> > +	static char buf[PATH_MAX + 1];
> > +	int baselen;
> > +	if (!base)
> > +		return abs;
> 
> This special case may help the specific caller you have below, but doesn't
> it make the function do more than it advertises with its name?

I don't think so; the best path relative to nothing is the absolute path. The idea is to return the easiest equivalent path if your pwd is known to be "base" and you give it an absolute path. If you don't know what your pwd is, the easiest equivalent path is the absolute path with no symlinks. Similarly, you get an absolute path if the path you give it isn't inside base. Maybe "make_brief_path" would be a better name?

> Other than that, I think the change is Ok, but as a "performance tweak",
> it should be backed by some numbers, please?

I was hoping Linus would provide some, since he'd noticed the slowness in the first place. I'm not sure I have the RAM to have the kernel spend non-trivial time looking up path elements in an all-cached tree. I'll try to replicate Linus's test case when I get a chance if he hasn't said anything.

	-Daniel
*This .sig left intentionally blank*
Linus Torvalds· Jun 19, 2008, 19:34 UTC · re: Junio C Hamano · lore

Re: [PATCH v2] Make git_dir a path relative to work_tree in setup_work_tree()

On Thu, 19 Jun 2008, Junio C Hamano wrote:
> 
> Other than that, I think the change is Ok, but as a "performance tweak",
> it should be backed by some numbers, please?

Daniel's patch didn't apply for me as-is, so I recreated it with some differences (appended), and here are the numbers from ten runs each.

There is some IO for me - probably due to more-or-less random flushing of the journal - so the variation is bigger than I'd like, but whatever:

	Before:
		real    0m8.135s
		real    0m7.933s
		real    0m8.080s
		real    0m7.954s
		real    0m7.949s
		real    0m8.112s
		real    0m7.934s
		real    0m8.059s
		real    0m7.979s
		real    0m8.038s
	After:
		real    0m7.685s
		real    0m7.968s
		real    0m7.703s
		real    0m7.850s
		real    0m7.995s
		real    0m7.817s
		real    0m7.963s
		real    0m7.955s
		real    0m7.848s
		real    0m7.969s

Now, going by "best of ten" (on the assumption that the longer numbers are all due to IO), I'm saying a 7.933s -> 7.685s reduction, and it does seem to be outside of the noise (ie the "after" case never broke 8s, while the "before" case did so half the time).

So looks like about 3% to me.

Doing it for a slightly smaller test-case (just the "arch" subdirectory) gets more stable numbers probably due to not filling the journal with metadata updates, so we have:

	Before:
		real    0m1.633s
		real    0m1.633s
		real    0m1.633s
		real    0m1.632s
		real    0m1.632s
		real    0m1.630s
		real    0m1.634s
		real    0m1.631s
		real    0m1.632s
		real    0m1.632s
	After:
		real    0m1.610s
		real    0m1.609s
		real    0m1.610s
		real    0m1.608s
		real    0m1.607s
		real    0m1.610s
		real    0m1.609s
		real    0m1.611s
		real    0m1.608s
		real    0m1.611s

where I'ld just take the averages and say 1.632 vs 1.610, which is just over 1% peformance improvement.

So it's not in the noise, but it's not as big as I initially thought and measured.

(That said, it obviously depends on how deep the working directory path is too, and whether it is behind NFS or something else that might need to cause more work to look up).

Modified/updated patch appended.
			Linus
----
 cache.h |    1 +
 path.c  |   17 +++++++++++++++++
 setup.c |    3 ++-
 3 files changed, 20 insertions(+), 1 deletions(-)
Show changes to 3 files +20 −1

cache.h, path.c, setup.c

diff --git a/cache.h b/cache.h
index 01c8502..29aae0c 100644
--- a/cache.h
+++ b/cache.h
@@ -526,6 +526,7 @@ static inline int is_absolute_path(const char *path)
 }
 const char *make_absolute_path(const char *path);
 const char *make_nonrelative_path(const char *path);
+const char *make_relative_path(const char *abs, const char *base);
 
 /* Read and unpack a sha1 file into memory, write memory to a sha1 file */
 extern int sha1_object_info(const unsigned char *, unsigned long *);
diff --git a/path.c b/path.c
index 7a35a26..6e3df18 100644
--- a/path.c
+++ b/path.c
@@ -330,6 +330,23 @@ const char *make_nonrelative_path(const char *path)
 /* We allow "recursive" symbolic links. Only within reason, though. */
 #define MAXDEPTH 5
 
+const char *make_relative_path(const char *abs, const char *base)
+{
+	static char buf[PATH_MAX + 1];
+	int baselen;
+	if (!base)
+		return abs;
+	baselen = strlen(base);
+	if (prefixcmp(abs, base))
+		return abs;
+	if (abs[baselen] == '/')
+		baselen++;
+	else if (base[baselen - 1] != '/')
+		return abs;
+	strcpy(buf, abs + baselen);
+	return buf;
+}
+
 const char *make_absolute_path(const char *path)
 {
 	static char bufs[2][PATH_MAX + 1], *buf = bufs[0], *next_buf = bufs[1];
diff --git a/setup.c b/setup.c
index d630e37..3b111ea 100644
--- a/setup.c
+++ b/setup.c
@@ -292,9 +292,10 @@ void setup_work_tree(void)
 	work_tree = get_git_work_tree();
 	git_dir = get_git_dir();
 	if (!is_absolute_path(git_dir))
-		set_git_dir(make_absolute_path(git_dir));
+		git_dir = make_absolute_path(git_dir);
 	if (!work_tree || chdir(work_tree))
 		die("This operation must be run in a work tree");
+	set_git_dir(make_relative_path(git_dir, work_tree));
 	initialized = 1;
 }

← back to recent threads