Re: [PATCH 3/3] wrapper: use a loop instead of repetitive statements
- From
Johannes Schindelin <johannes.schindelin@gmx.de>
- Date
- Sep 26, 2019, 20:14 UTC
- Message-ID
- <nycvar.QRO.7.76.6.1909262155020.15067@tvgsbejvaqbjf.bet>
- In-Reply-To
- <03e509db-942e-4538-4729-4e345df82a7e@gmail.com>
Hi,
On Thu, 26 Sep 2019, Derrick Stolee wrote:
Show 32 quoted lines
> 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