[PATCH] speed: reuse char instead of recreation in loop

Move a char and a char * outside of a for loop for speed improvements

Signed-off-by: Thomas Spura <redacted>
---
Comments?

 transport.c |    7 +++----
 1 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/transport.c b/transport.c
index 17891d5..e350937 100644
--- a/transport.c
+++ b/transport.c

@@ -263,11 +263,10 @@ static int write_refs_to_temp_dir(struct strbuf 

*temp_dir,
 		int refspec_nr, const char **refspec)
 {
 	int i;
+	unsigned char sha1[20];
+	char *ref;
 
 	for (i = 0; i < refspec_nr; i++) {
-		unsigned char sha1[20];
-		char *ref;
-
 		if (dwim_ref(refspec[i], strlen(refspec[i]), sha1, &ref) !
= 1)
 			return error("Could not get ref %s", refspec[i]);
 

@@ -275,8 +274,8 @@ static int write_refs_to_temp_dir(struct strbuf 

*temp_dir,
 			free(ref);
 			return -1;
 		}
-		free(ref);
 	}
+	free(ref);
 	return 0;
 }
 
-- 
1.6.0.6

On 2009.05.25 19:44:10 +0000, Thomas Spura wrote:

I doubt that this makes any difference at all.

And this now leaks memory.

Björn

Thomas Spura schrieb:

Move a char and a char * outside of a for loop for speed improvements

It's a good idea to include actual timings, to give the reader a better
idea what operation is sped up and by how much.

Please turn off automatic line wrapping in your email program when
sending patches (at least for the patch part).

 		int refspec_nr, const char **refspec)
 {
 	int i;
+	unsigned char sha1[20];
+	char *ref;
 
 	for (i = 0; i < refspec_nr; i++) {
-		unsigned char sha1[20];
-		char *ref;
-

I wouldn't expect this to significantly change the object code.
Declaring variables in as narrow a scope as possible often helps to make
the code more readable, though.  write_refs_to_temp_dir() is short
enough, so it doesn't matter in this case, though.

This introduces a memory leak.  The string pointed to by ref is
allocated by dwim_ref() and needs to be free()'d after use, and -- more
importantly -- before ref is assigned its next value by dwim_ref().

René

Am Mon, 25 May 2009 22:16:02 +0200 schrieb Björn Steinbrink:

On 2009.05.25 19:44:10 +0000, Thomas Spura wrote:

quoted

Move a char and a char * outside of a for loop for speed improvements

Signed-off-by: Thomas Spura <redacted> ---
Comments?

 transport.c |    7 +++----
 1 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/transport.c b/transport.c index 17891d5..e350937 100644
--- a/transport.c
+++ b/transport.c

@@ -263,11 +263,10 @@ static int write_refs_to_temp_dir(struct strbuf

*temp_dir,
 		int refspec_nr, const char **refspec)
 {
 	int i;
+	unsigned char sha1[20];
+	char *ref;
 
 	for (i = 0; i < refspec_nr; i++) {
-		unsigned char sha1[20];
-		char *ref;
-

I doubt that this makes any difference at all.

With ints, the loop costs about 40% of speed. Without recreation, it 
should be always faster.

quoted

 		if (dwim_ref(refspec[i], strlen(refspec[i]), sha1, &ref) !
= 1)
 			return error("Could not get ref %s", refspec[i]);
 

@@ -275,8 +274,8 @@ static int write_refs_to_temp_dir(struct strbuf

*temp_dir,
 			free(ref);
 			return -1;
 		}
-		free(ref);
 	}
+	free(ref);

And this now leaks memory.

Hmm, I don't see it atm. Where do you mean?

Am Mon, 25 May 2009 22:39:05 +0200 schrieb René Scharfe:

This introduces a memory leak.  The string pointed to by ref is
allocated by dwim_ref() and needs to be free()'d after use, and -- more
importantly -- before ref is assigned its next value by dwim_ref().

Thanks, sorry for the noise...

	Thomas

On Mon, 25 May 2009, Thomas Spura wrote:

Am Mon, 25 May 2009 22:16:02 +0200 schrieb Björn Steinbrink:

quoted

On 2009.05.25 19:44:10 +0000, Thomas Spura wrote:

quoted

Move a char and a char * outside of a for loop for speed improvements

Signed-off-by: Thomas Spura <redacted> ---
Comments?

 transport.c |    7 +++----
 1 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/transport.c b/transport.c index 17891d5..e350937 100644
--- a/transport.c
+++ b/transport.c

@@ -263,11 +263,10 @@ static int write_refs_to_temp_dir(struct strbuf

*temp_dir,
 		int refspec_nr, const char **refspec)
 {
 	int i;
+	unsigned char sha1[20];
+	char *ref;
 
 	for (i = 0; i < refspec_nr; i++) {
-		unsigned char sha1[20];
-		char *ref;
-

I doubt that this makes any difference at all.

With ints, the loop costs about 40% of speed. Without recreation, it 
should be always faster.

Actually, having the variables go out of scope should be at least as fast. 
The compiler doesn't actually do anything to make the old variable
inaccessible and get a new variable; with the variable uninitialized, it's 
legitimate for the compiler to simply reuse the same storage for all 
iterations. Futhermore, with the variables declared inside the loop, the 
compiler is allowed to make optimizations that would fail to preserve 
those variables between iterations. There are probably no such 
optimizations in this code for it to make, but, in general, letting 
variables in loops go out of scope (in C) only improves optimization 
possibilities.

	-Daniel
*This .sig left intentionally blank*

Daniel Barkalow wrote:

On Mon, 25 May 2009, Thomas Spura wrote:

quoted

Am Mon, 25 May 2009 22:16:02 +0200 schrieb Bj?rn Steinbrink:

quoted

On 2009.05.25 19:44:10 +0000, Thomas Spura wrote:

quoted

Move a char and a char * outside of a for loop for speed improvements

quoted

quoted

I doubt that this makes any difference at all.

quoted

With ints, the loop costs about 40% of speed. Without recreation, it 
should be always faster.

optimizations in this code for it to make, but, in general, letting 
variables in loops go out of scope (in C) only improves optimization 
possibilities.

Quite.  The proposed patch is more likely to slow down the code than to
speed it up.
-- 
Sincerely,
           Stephen R. van den Berg.

E Pluribus Unix.