{"thread":{"id":"19503","subject":"[PATCH] speed: reuse char instead of recreation in loop","startedAt":"2009-05-25T19:44:10Z","lastAt":"2009-05-26T11:54:03Z","messageCount":7,"participants":["Thomas Spura","Björn Steinbrink","René Scharfe","Daniel Barkalow","Stephen R. van den Berg"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"114696","messageId":"pan.2009.05.25.19.44.10@fedoraproject.org","threadId":"19503","inReplyTo":null,"subject":"[PATCH] speed: reuse char instead of recreation in loop","fromName":"Thomas Spura","fromEmail":"tomspur@fedoraproject.org","sentAt":"2009-05-25T19:44:10Z","receivedAt":"2009-05-25T19:44:10Z","isPatch":true,"sender":{"key":"tomspur@fedoraproject.org","avatar":"https://gravatar.com/avatar/ddf763cae481e089eeffdb3fb1ed26ec2972fcd73703b3a4d2c04e7bce7b362f?d=mp&s=160"},"body":"Move a char and a char * outside of a for loop for speed improvements\n\nSigned-off-by: Thomas Spura <tomspur@fedoraproject.org>\n---\nComments?\n\n transport.c |    7 +++----\n 1 files changed, 3 insertions(+), 4 deletions(-)\n\ndiff --git a/transport.c b/transport.c\nindex 17891d5..e350937 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -263,11 +263,10 @@ static int write_refs_to_temp_dir(struct strbuf \n*temp_dir,\n \t\tint refspec_nr, const char **refspec)\n {\n \tint i;\n+\tunsigned char sha1[20];\n+\tchar *ref;\n \n \tfor (i = 0; i < refspec_nr; i++) {\n-\t\tunsigned char sha1[20];\n-\t\tchar *ref;\n-\n \t\tif (dwim_ref(refspec[i], strlen(refspec[i]), sha1, &ref) !\n= 1)\n \t\t\treturn error(\"Could not get ref %s\", refspec[i]);\n \n@@ -275,8 +274,8 @@ static int write_refs_to_temp_dir(struct strbuf \n*temp_dir,\n \t\t\tfree(ref);\n \t\t\treturn -1;\n \t\t}\n-\t\tfree(ref);\n \t}\n+\tfree(ref);\n \treturn 0;\n }\n \n-- \n1.6.0.6\n"},{"id":"114697","messageId":"20090525201602.GA18471@atjola.homenet","threadId":"19503","inReplyTo":"pan.2009.05.25.19.44.10@fedoraproject.org","subject":"Re: [PATCH] speed: reuse char instead of recreation in loop","fromName":"Björn Steinbrink","fromEmail":"b.steinbrink@gmx.de","sentAt":"2009-05-25T20:16:02Z","receivedAt":"2009-05-25T20:16:02Z","isPatch":true,"sender":{"key":"b.steinbrink@gmx.de","avatar":"https://avatars.githubusercontent.com/u/230962?v=4"},"body":"On 2009.05.25 19:44:10 +0000, Thomas Spura wrote:\n> Move a char and a char * outside of a for loop for speed improvements\n> \n> Signed-off-by: Thomas Spura <tomspur@fedoraproject.org>\n> ---\n> Comments?\n> \n>  transport.c |    7 +++----\n>  1 files changed, 3 insertions(+), 4 deletions(-)\n> \n> diff --git a/transport.c b/transport.c\n> index 17891d5..e350937 100644\n> --- a/transport.c\n> +++ b/transport.c\n> @@ -263,11 +263,10 @@ static int write_refs_to_temp_dir(struct strbuf \n> *temp_dir,\n>  \t\tint refspec_nr, const char **refspec)\n>  {\n>  \tint i;\n> +\tunsigned char sha1[20];\n> +\tchar *ref;\n>  \n>  \tfor (i = 0; i < refspec_nr; i++) {\n> -\t\tunsigned char sha1[20];\n> -\t\tchar *ref;\n> -\n\nI doubt that this makes any difference at all.\n\n>  \t\tif (dwim_ref(refspec[i], strlen(refspec[i]), sha1, &ref) !\n> = 1)\n>  \t\t\treturn error(\"Could not get ref %s\", refspec[i]);\n>  \n> @@ -275,8 +274,8 @@ static int write_refs_to_temp_dir(struct strbuf \n> *temp_dir,\n>  \t\t\tfree(ref);\n>  \t\t\treturn -1;\n>  \t\t}\n> -\t\tfree(ref);\n>  \t}\n> +\tfree(ref);\n\nAnd this now leaks memory.\n\nBjörn\n"},{"id":"114698","messageId":"4A1B01E9.30601@lsrfire.ath.cx","threadId":"19503","inReplyTo":"pan.2009.05.25.19.44.10@fedoraproject.org","subject":"Re: [PATCH] speed: reuse char instead of recreation in loop","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2009-05-25T20:39:05Z","receivedAt":"2009-05-25T20:39:05Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Thomas Spura schrieb:\n> Move a char and a char * outside of a for loop for speed improvements\n\nIt's a good idea to include actual timings, to give the reader a better\nidea what operation is sped up and by how much.\n\n> Signed-off-by: Thomas Spura <tomspur@fedoraproject.org>\n> ---\n> Comments?\n> \n>  transport.c |    7 +++----\n>  1 files changed, 3 insertions(+), 4 deletions(-)\n> \n> diff --git a/transport.c b/transport.c\n> index 17891d5..e350937 100644\n> --- a/transport.c\n> +++ b/transport.c\n> @@ -263,11 +263,10 @@ static int write_refs_to_temp_dir(struct strbuf \n> *temp_dir,\n\nPlease turn off automatic line wrapping in your email program when\nsending patches (at least for the patch part).\n\n>  \t\tint refspec_nr, const char **refspec)\n>  {\n>  \tint i;\n> +\tunsigned char sha1[20];\n> +\tchar *ref;\n>  \n>  \tfor (i = 0; i < refspec_nr; i++) {\n> -\t\tunsigned char sha1[20];\n> -\t\tchar *ref;\n> -\n\nI wouldn't expect this to significantly change the object code.\nDeclaring variables in as narrow a scope as possible often helps to make\nthe code more readable, though.  write_refs_to_temp_dir() is short\nenough, so it doesn't matter in this case, though.\n\n>  \t\tif (dwim_ref(refspec[i], strlen(refspec[i]), sha1, &ref) !\n> = 1)\n>  \t\t\treturn error(\"Could not get ref %s\", refspec[i]);\n>  \n> @@ -275,8 +274,8 @@ static int write_refs_to_temp_dir(struct strbuf \n> *temp_dir,\n>  \t\t\tfree(ref);\n>  \t\t\treturn -1;\n>  \t\t}\n> -\t\tfree(ref);\n>  \t}\n> +\tfree(ref);\n>  \treturn 0;\n>  }\n\nThis introduces a memory leak.  The string pointed to by ref is\nallocated by dwim_ref() and needs to be free()'d after use, and -- more\nimportantly -- before ref is assigned its next value by dwim_ref().\n\nRené\n"},{"id":"114699","messageId":"pan.2009.05.25.20.40.20@fedoraproject.org","threadId":"19503","inReplyTo":"20090525201602.GA18471@atjola.homenet","subject":"Re: [PATCH] speed: reuse char instead of recreation in loop","fromName":"Thomas Spura","fromEmail":"tomspur@fedoraproject.org","sentAt":"2009-05-25T20:40:27Z","receivedAt":"2009-05-25T20:40:27Z","isPatch":true,"sender":{"key":"tomspur@fedoraproject.org","avatar":"https://gravatar.com/avatar/ddf763cae481e089eeffdb3fb1ed26ec2972fcd73703b3a4d2c04e7bce7b362f?d=mp&s=160"},"body":"Am Mon, 25 May 2009 22:16:02 +0200 schrieb Björn Steinbrink:\n\n> On 2009.05.25 19:44:10 +0000, Thomas Spura wrote:\n>> Move a char and a char * outside of a for loop for speed improvements\n>> \n>> Signed-off-by: Thomas Spura <tomspur@fedoraproject.org> ---\n>> Comments?\n>> \n>>  transport.c |    7 +++----\n>>  1 files changed, 3 insertions(+), 4 deletions(-)\n>> \n>> diff --git a/transport.c b/transport.c index 17891d5..e350937 100644\n>> --- a/transport.c\n>> +++ b/transport.c\n>> @@ -263,11 +263,10 @@ static int write_refs_to_temp_dir(struct strbuf\n>> *temp_dir,\n>>  \t\tint refspec_nr, const char **refspec)\n>>  {\n>>  \tint i;\n>> +\tunsigned char sha1[20];\n>> +\tchar *ref;\n>>  \n>>  \tfor (i = 0; i < refspec_nr; i++) {\n>> -\t\tunsigned char sha1[20];\n>> -\t\tchar *ref;\n>> -\n> \n> I doubt that this makes any difference at all.\n\nWith ints, the loop costs about 40% of speed. Without recreation, it \nshould be always faster.\n\n> \n>>  \t\tif (dwim_ref(refspec[i], strlen(refspec[i]), sha1, &ref) !\n>> = 1)\n>>  \t\t\treturn error(\"Could not get ref %s\", refspec[i]);\n>>  \n>> @@ -275,8 +274,8 @@ static int write_refs_to_temp_dir(struct strbuf\n>> *temp_dir,\n>>  \t\t\tfree(ref);\n>>  \t\t\treturn -1;\n>>  \t\t}\n>> -\t\tfree(ref);\n>>  \t}\n>> +\tfree(ref);\n> \n> And this now leaks memory.\n\nHmm, I don't see it atm. Where do you mean?\n"},{"id":"114700","messageId":"pan.2009.05.25.20.44.20@fedoraproject.org","threadId":"19503","inReplyTo":"4A1B01E9.30601@lsrfire.ath.cx","subject":"Re: [PATCH] speed: reuse char instead of recreation in loop","fromName":"Thomas Spura","fromEmail":"tomspur@fedoraproject.org","sentAt":"2009-05-25T20:44:20Z","receivedAt":"2009-05-25T20:44:20Z","isPatch":true,"sender":{"key":"tomspur@fedoraproject.org","avatar":"https://gravatar.com/avatar/ddf763cae481e089eeffdb3fb1ed26ec2972fcd73703b3a4d2c04e7bce7b362f?d=mp&s=160"},"body":"Am Mon, 25 May 2009 22:39:05 +0200 schrieb René Scharfe:\n> This introduces a memory leak.  The string pointed to by ref is\n> allocated by dwim_ref() and needs to be free()'d after use, and -- more\n> importantly -- before ref is assigned its next value by dwim_ref().\n\nThanks, sorry for the noise...\n\n\tThomas\n"},{"id":"114705","messageId":"alpine.LNX.2.00.0905251733240.2147@iabervon.org","threadId":"19503","inReplyTo":"pan.2009.05.25.20.40.20@fedoraproject.org","subject":"Re: [PATCH] speed: reuse char instead of recreation in loop","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2009-05-25T22:08:55Z","receivedAt":"2009-05-25T22:08:55Z","isPatch":true,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Mon, 25 May 2009, Thomas Spura wrote:\n\n> Am Mon, 25 May 2009 22:16:02 +0200 schrieb Björn Steinbrink:\n> \n> > On 2009.05.25 19:44:10 +0000, Thomas Spura wrote:\n> >> Move a char and a char * outside of a for loop for speed improvements\n> >> \n> >> Signed-off-by: Thomas Spura <tomspur@fedoraproject.org> ---\n> >> Comments?\n> >> \n> >>  transport.c |    7 +++----\n> >>  1 files changed, 3 insertions(+), 4 deletions(-)\n> >> \n> >> diff --git a/transport.c b/transport.c index 17891d5..e350937 100644\n> >> --- a/transport.c\n> >> +++ b/transport.c\n> >> @@ -263,11 +263,10 @@ static int write_refs_to_temp_dir(struct strbuf\n> >> *temp_dir,\n> >>  \t\tint refspec_nr, const char **refspec)\n> >>  {\n> >>  \tint i;\n> >> +\tunsigned char sha1[20];\n> >> +\tchar *ref;\n> >>  \n> >>  \tfor (i = 0; i < refspec_nr; i++) {\n> >> -\t\tunsigned char sha1[20];\n> >> -\t\tchar *ref;\n> >> -\n> > \n> > I doubt that this makes any difference at all.\n> \n> With ints, the loop costs about 40% of speed. Without recreation, it \n> should be always faster.\n\nActually, having the variables go out of scope should be at least as fast. \nThe compiler doesn't actually do anything to make the old variable\ninaccessible and get a new variable; with the variable uninitialized, it's \nlegitimate for the compiler to simply reuse the same storage for all \niterations. Futhermore, with the variables declared inside the loop, the \ncompiler is allowed to make optimizations that would fail to preserve \nthose variables between iterations. There are probably no such \noptimizations in this code for it to make, but, in general, letting \nvariables in loops go out of scope (in C) only improves optimization \npossibilities.\n\n\t-Daniel\n*This .sig left intentionally blank*"},{"id":"114737","messageId":"20090526115403.GA4246@cuci.nl","threadId":"19503","inReplyTo":"alpine.LNX.2.00.0905251733240.2147@iabervon.org","subject":"Re: [PATCH] speed: reuse char instead of recreation in loop","fromName":"Stephen R. van den Berg","fromEmail":"srb@cuci.nl","sentAt":"2009-05-26T11:54:03Z","receivedAt":"2009-05-26T11:54:03Z","isPatch":true,"sender":{"key":"srb@cuci.nl","avatar":"https://gravatar.com/avatar/f75389059e827634d38e9df2a9b6ecbd50028b5a454442efa1c7205b7ff29c6a?d=mp&s=160"},"body":"Daniel Barkalow wrote:\n>On Mon, 25 May 2009, Thomas Spura wrote:\n>> Am Mon, 25 May 2009 22:16:02 +0200 schrieb Bj?rn Steinbrink:\n>> > On 2009.05.25 19:44:10 +0000, Thomas Spura wrote:\n>> >> Move a char and a char * outside of a for loop for speed improvements\n\n>> > I doubt that this makes any difference at all.\n\n>> With ints, the loop costs about 40% of speed. Without recreation, it \n>> should be always faster.\n\n>optimizations in this code for it to make, but, in general, letting \n>variables in loops go out of scope (in C) only improves optimization \n>possibilities.\n\nQuite.  The proposed patch is more likely to slow down the code than to\nspeed it up.\n-- \nSincerely,\n           Stephen R. van den Berg.\n\nE Pluribus Unix.\n"}]}