diff mbox series

[3/3] wrapper: use a loop instead of repetitive statements

Message ID 20190925020158.751492-4-alexhenrie24@gmail.com (mailing list archive)
State New, archived
Headers show
Series scan-build fixes | expand

Commit Message

Alex Henrie Sept. 25, 2019, 2:01 a.m. UTC
Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>
---
 wrapper.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

Comments

Derrick Stolee Sept. 26, 2019, 1:13 p.m. UTC | #1
On 9/24/2019 10:01 PM, Alex Henrie wrote:
> Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>
> ---
>  wrapper.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/wrapper.c b/wrapper.c
> index c55d7722d7..c23ac6adcd 100644
> --- a/wrapper.c
> +++ b/wrapper.c
> @@ -469,13 +469,12 @@ int git_mkstemps_mode(char *pattern, int suffix_len, int mode)
>  	filename_template = &pattern[len - 6 - suffix_len];
>  	for (count = 0; count < TMP_MAX; ++count) {
>  		uint64_t v = value;
> +		int i;
>  		/* Fill in the random bits. */
> -		filename_template[0] = letters[v % num_letters]; v /= num_letters;
> -		filename_template[1] = letters[v % num_letters]; v /= num_letters;
> -		filename_template[2] = letters[v % num_letters]; v /= num_letters;
> -		filename_template[3] = letters[v % num_letters]; v /= num_letters;
> -		filename_template[4] = letters[v % num_letters]; v /= num_letters;
> -		filename_template[5] = letters[v % num_letters]; v /= num_letters;
> +		for (i = 0; i < 6; i++) {
> +			filename_template[i] = letters[v % num_letters];
> +			v /= num_letters;
> +		}
>  
>  		fd = open(pattern, O_CREAT | O_EXCL | O_RDWR, mode);
>  		if (fd >= 0)
> 

This change is clear.

Thanks,
-Stolee
Johannes Schindelin Sept. 26, 2019, 8:14 p.m. UTC | #2
Hi,

On Thu, 26 Sep 2019, Derrick Stolee wrote:

> On 9/24/2019 10:01 PM, Alex Henrie wrote:
> > Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>
> > ---
> >  wrapper.c | 11 +++++------
> >  1 file changed, 5 insertions(+), 6 deletions(-)
> >
> > diff --git a/wrapper.c b/wrapper.c
> > index c55d7722d7..c23ac6adcd 100644
> > --- a/wrapper.c
> > +++ b/wrapper.c
> > @@ -469,13 +469,12 @@ int git_mkstemps_mode(char *pattern, int suffix_len, int mode)
> >  	filename_template = &pattern[len - 6 - suffix_len];
> >  	for (count = 0; count < TMP_MAX; ++count) {
> >  		uint64_t v = value;
> > +		int i;
> >  		/* Fill in the random bits. */
> > -		filename_template[0] = letters[v % num_letters]; v /= num_letters;
> > -		filename_template[1] = letters[v % num_letters]; v /= num_letters;
> > -		filename_template[2] = letters[v % num_letters]; v /= num_letters;
> > -		filename_template[3] = letters[v % num_letters]; v /= num_letters;
> > -		filename_template[4] = letters[v % num_letters]; v /= num_letters;
> > -		filename_template[5] = letters[v % num_letters]; v /= num_letters;
> > +		for (i = 0; i < 6; i++) {
> > +			filename_template[i] = letters[v % num_letters];
> > +			v /= num_letters;
> > +		}
> >
> >  		fd = open(pattern, O_CREAT | O_EXCL | O_RDWR, mode);
> >  		if (fd >= 0)
> >
>
> This change is clear.

Not so fast.

This looks like it was intended to help compilers that cannot unroll
loops all that easily, and just because current clang can does not mean
that we should put people at a deliberate disadvantage when they are
stuck with a C compiler that cannot optimize the post-image of this
diff.

Let's first see whether my gut feeling has any merit.

This part of the code entered Git's tree in 0620b39b3b7 (compat: add a
mkstemps() compatibility function, 2009-05-31), and it was clearly
copy-edited from libiberty.

It entered libiberty in
https://github.com/gcc-mirror/gcc/commit/119735e34916. That commit
claims that it was copy-edited from glibc, and edited it was, as it
inlined the `__gen_tempname()` function.

Sadly, the commit message of the patch that introduced this pattern into
glibc is pretty much not helpful at all here:
https://sourceware.org/git/?p=glibc.git;a=commitdiff;h=a7ab2023fcdd5c90c9f664cbaed8ef90dd38e818
(it only talks about using a random-name generator that is already used
elsewhere.)

In short: sadly, this history excursion did not reveal anything to back
up my intuition that your change would revert a change that might be
crucial on older platforms.

However, I think that this patch should at least be accompanied by a
commit message that suggests that some thought was put into it, and that
concerns like mine were considered carefully.

I mean, if there is _any_ performance-critical code path hitting this
unrolled loop, we may want to keep it unrolled.

Ciao,
Johannes
Jeff King Sept. 27, 2019, 2:45 a.m. UTC | #3
On Tue, Sep 24, 2019 at 08:01:58PM -0600, Alex Henrie wrote:

> diff --git a/wrapper.c b/wrapper.c
> index c55d7722d7..c23ac6adcd 100644
> --- a/wrapper.c
> +++ b/wrapper.c
> @@ -469,13 +469,12 @@ int git_mkstemps_mode(char *pattern, int suffix_len, int mode)
>  	filename_template = &pattern[len - 6 - suffix_len];
>  	for (count = 0; count < TMP_MAX; ++count) {
>  		uint64_t v = value;
> +		int i;
>  		/* Fill in the random bits. */
> -		filename_template[0] = letters[v % num_letters]; v /= num_letters;
> -		filename_template[1] = letters[v % num_letters]; v /= num_letters;
> -		filename_template[2] = letters[v % num_letters]; v /= num_letters;
> -		filename_template[3] = letters[v % num_letters]; v /= num_letters;
> -		filename_template[4] = letters[v % num_letters]; v /= num_letters;
> -		filename_template[5] = letters[v % num_letters]; v /= num_letters;
> +		for (i = 0; i < 6; i++) {
> +			filename_template[i] = letters[v % num_letters];
> +			v /= num_letters;
> +		}

I'm not sure the readability is changed much either way. But it does
enable this additional cleanup on top:

-- >8 --
Subject: git_mkstemps_mode(): replace magic numbers with computed value

The magic number "6" appears several times in the function, and is
related to the size of the "XXXXXX" string we expect to find in the
template. Let's pull that "XXXXXX" into a constant array, whose size we
can get at compile time with ARRAY_SIZE().

Note that we probably can't just change this value, since callers will
be feeding us a certain number of X's, but it hopefully makes the
function itself easier to follow.

While we're here, let's do the same with the "letters" array (which we
_could_ modify if we wanted to include more characters).

Signed-off-by: Jeff King <peff@peff.net>
---
 wrapper.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/wrapper.c b/wrapper.c
index c23ac6adcd..e1eaef2e16 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -441,7 +441,9 @@ int git_mkstemps_mode(char *pattern, int suffix_len, int mode)
 		"abcdefghijklmnopqrstuvwxyz"
 		"ABCDEFGHIJKLMNOPQRSTUVWXYZ"
 		"0123456789";
-	static const int num_letters = 62;
+	static const int num_letters = ARRAY_SIZE(letters) - 1;
+	static const char x_pattern[] = "XXXXXX";
+	static const int num_x = ARRAY_SIZE(x_pattern) - 1;
 	uint64_t value;
 	struct timeval tv;
 	char *filename_template;
@@ -450,12 +452,12 @@ int git_mkstemps_mode(char *pattern, int suffix_len, int mode)
 
 	len = strlen(pattern);
 
-	if (len < 6 + suffix_len) {
+	if (len < num_x + suffix_len) {
 		errno = EINVAL;
 		return -1;
 	}
 
-	if (strncmp(&pattern[len - 6 - suffix_len], "XXXXXX", 6)) {
+	if (strncmp(&pattern[len - num_x - suffix_len], x_pattern, num_x)) {
 		errno = EINVAL;
 		return -1;
 	}
@@ -466,12 +468,12 @@ int git_mkstemps_mode(char *pattern, int suffix_len, int mode)
 	 */
 	gettimeofday(&tv, NULL);
 	value = ((uint64_t)tv.tv_usec << 16) ^ tv.tv_sec ^ getpid();
-	filename_template = &pattern[len - 6 - suffix_len];
+	filename_template = &pattern[len - num_x - suffix_len];
 	for (count = 0; count < TMP_MAX; ++count) {
 		uint64_t v = value;
 		int i;
 		/* Fill in the random bits. */
-		for (i = 0; i < 6; i++) {
+		for (i = 0; i < num_x; i++) {
 			filename_template[i] = letters[v % num_letters];
 			v /= num_letters;
 		}
Jeff King Sept. 27, 2019, 2:50 a.m. UTC | #4
On Thu, Sep 26, 2019 at 10:14:17PM +0200, Johannes Schindelin wrote:

> I mean, if there is _any_ performance-critical code path hitting this
> unrolled loop, we may want to keep it unrolled.

The loop in question is maybe a few dozen instructions, and then it
immediately makes an open() syscall, which is probably multiple orders
of magnitude more expensive. So I'd be very surprised if it was a
problem no matter what the generated code looks like.

But...

> However, I think that this patch should at least be accompanied by a
> commit message that suggests that some thought was put into it, and that
> concerns like mine were considered carefully.

...this I would definitely agree with.

-Peff
Derrick Stolee Sept. 27, 2019, 11:48 a.m. UTC | #5
On 9/26/2019 10:45 PM, Jeff King wrote:
> On Tue, Sep 24, 2019 at 08:01:58PM -0600, Alex Henrie wrote:
> 
>> diff --git a/wrapper.c b/wrapper.c
>> index c55d7722d7..c23ac6adcd 100644
>> --- a/wrapper.c
>> +++ b/wrapper.c
>> @@ -469,13 +469,12 @@ int git_mkstemps_mode(char *pattern, int suffix_len, int mode)
>>  	filename_template = &pattern[len - 6 - suffix_len];
>>  	for (count = 0; count < TMP_MAX; ++count) {
>>  		uint64_t v = value;
>> +		int i;
>>  		/* Fill in the random bits. */
>> -		filename_template[0] = letters[v % num_letters]; v /= num_letters;
>> -		filename_template[1] = letters[v % num_letters]; v /= num_letters;
>> -		filename_template[2] = letters[v % num_letters]; v /= num_letters;
>> -		filename_template[3] = letters[v % num_letters]; v /= num_letters;
>> -		filename_template[4] = letters[v % num_letters]; v /= num_letters;
>> -		filename_template[5] = letters[v % num_letters]; v /= num_letters;
>> +		for (i = 0; i < 6; i++) {
>> +			filename_template[i] = letters[v % num_letters];
>> +			v /= num_letters;
>> +		}
> 
> I'm not sure the readability is changed much either way. But it does
> enable this additional cleanup on top:
> 
> -- >8 --
> Subject: git_mkstemps_mode(): replace magic numbers with computed value
> 
> The magic number "6" appears several times in the function, and is
> related to the size of the "XXXXXX" string we expect to find in the
> template. Let's pull that "XXXXXX" into a constant array, whose size we
> can get at compile time with ARRAY_SIZE().

Removing magic numbers is always a good change. Thanks!

> Note that we probably can't just change this value, since callers will
> be feeding us a certain number of X's, but it hopefully makes the
> function itself easier to follow.
> 
> While we're here, let's do the same with the "letters" array (which we
> _could_ modify if we wanted to include more characters).
> 
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  wrapper.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/wrapper.c b/wrapper.c
> index c23ac6adcd..e1eaef2e16 100644
> --- a/wrapper.c
> +++ b/wrapper.c
> @@ -441,7 +441,9 @@ int git_mkstemps_mode(char *pattern, int suffix_len, int mode)
>  		"abcdefghijklmnopqrstuvwxyz"
>  		"ABCDEFGHIJKLMNOPQRSTUVWXYZ"
>  		"0123456789";
> -	static const int num_letters = 62;
> +	static const int num_letters = ARRAY_SIZE(letters) - 1;
> +	static const char x_pattern[] = "XXXXXX";
> +	static const int num_x = ARRAY_SIZE(x_pattern) - 1;
>  	uint64_t value;
>  	struct timeval tv;
>  	char *filename_template;
> @@ -450,12 +452,12 @@ int git_mkstemps_mode(char *pattern, int suffix_len, int mode)
>  
>  	len = strlen(pattern);
>  
> -	if (len < 6 + suffix_len) {
> +	if (len < num_x + suffix_len) {
>  		errno = EINVAL;
>  		return -1;
>  	}
>  
> -	if (strncmp(&pattern[len - 6 - suffix_len], "XXXXXX", 6)) {
> +	if (strncmp(&pattern[len - num_x - suffix_len], x_pattern, num_x)) {
>  		errno = EINVAL;
>  		return -1;
>  	}
> @@ -466,12 +468,12 @@ int git_mkstemps_mode(char *pattern, int suffix_len, int mode)
>  	 */
>  	gettimeofday(&tv, NULL);
>  	value = ((uint64_t)tv.tv_usec << 16) ^ tv.tv_sec ^ getpid();
> -	filename_template = &pattern[len - 6 - suffix_len];
> +	filename_template = &pattern[len - num_x - suffix_len];
>  	for (count = 0; count < TMP_MAX; ++count) {
>  		uint64_t v = value;
>  		int i;
>  		/* Fill in the random bits. */
> -		for (i = 0; i < 6; i++) {
> +		for (i = 0; i < num_x; i++) {
>  			filename_template[i] = letters[v % num_letters];
>  			v /= num_letters;
>  		}
>
Alex Henrie Sept. 29, 2019, 12:51 a.m. UTC | #6
On Thu, Sep 26, 2019 at 8:50 PM Jeff King <peff@peff.net> wrote:
>
> On Thu, Sep 26, 2019 at 10:14:17PM +0200, Johannes Schindelin wrote:
>
> > However, I think that this patch should at least be accompanied by a
> > commit message that suggests that some thought was put into it, and that
> > concerns like mine were considered carefully.
>
> ...this I would definitely agree with.

Thanks for the feedback everyone. I am not used to projects requiring
detailed commit messages for small changes. I will send a v3 of the
three patches with the additional explanations you requested.

-Alex
diff mbox series

Patch

diff --git a/wrapper.c b/wrapper.c
index c55d7722d7..c23ac6adcd 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -469,13 +469,12 @@  int git_mkstemps_mode(char *pattern, int suffix_len, int mode)
 	filename_template = &pattern[len - 6 - suffix_len];
 	for (count = 0; count < TMP_MAX; ++count) {
 		uint64_t v = value;
+		int i;
 		/* Fill in the random bits. */
-		filename_template[0] = letters[v % num_letters]; v /= num_letters;
-		filename_template[1] = letters[v % num_letters]; v /= num_letters;
-		filename_template[2] = letters[v % num_letters]; v /= num_letters;
-		filename_template[3] = letters[v % num_letters]; v /= num_letters;
-		filename_template[4] = letters[v % num_letters]; v /= num_letters;
-		filename_template[5] = letters[v % num_letters]; v /= num_letters;
+		for (i = 0; i < 6; i++) {
+			filename_template[i] = letters[v % num_letters];
+			v /= num_letters;
+		}
 
 		fd = open(pattern, O_CREAT | O_EXCL | O_RDWR, mode);
 		if (fd >= 0)