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@xxxxxxxxx>
> > ---
> > 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
