Pierre HabouzitHere is a smallish series in builtin-rerere.c. Nothing very exciting though.
Pierre Habouzit
Johannes SchindelinRe: [PATCH 2/2] Make builtin-rerere use of strbuf nicer and more efficient.
Hi,
both patches appear to be obviously correct to me. (However, I was lazy enough not to compile and test, but I'd not expect any breakage there, given your previous patch serieses. ;-)
Ciao, Dscho
Pierre HabouzitRe: [PATCH 2/2] Make builtin-rerere use of strbuf nicer and more efficient.
On mer, sep 26, 2007 at 12:31:47 +0000, Junio C Hamano wrote:
> Signoffs?
This is obviously a lapse on my end. You can add:
Signed-off-by: Pierre Habouzit <madcoder@debian.org>
To any patch I send to this list.
-- ·O· Pierre Habouzit ··O madcoder@debian.org OOO http://www.madism.org
Alex Riesen[PATCH] Make strbuf_cmp inline, constify its arguments and optimize it a bit
It is definitely less code (also object code). It is not always measurably faster (but mostly is).
Signed-off-by: Alex Riesen <raa.lkml@gmail.com> --- strbuf.c | 12 ------------ strbuf.h | 9 ++++++++- 2 files changed, 8 insertions(+), 13 deletions(-)
diff --git a/strbuf.c b/strbuf.c index f4201e1..215837b 100644 --- a/strbuf.c +++ b/strbuf.c @@ -58,18 +58,6 @@ void strbuf_rtrim(struct strbuf *sb) sb->buf[sb->len] = '\0'; } -int strbuf_cmp(struct strbuf *a, struct strbuf *b) -{ - int cmp; - if (a->len < b->len) { - cmp = memcmp(a->buf, b->buf, a->len); - return cmp ? cmp : -1; - } else { - cmp = memcmp(a->buf, b->buf, b->len); - return cmp ? cmp : a->len != b->len; - } -} - void strbuf_splice(struct strbuf *sb, size_t pos, size_t len, const void *data, size_t dlen) { diff --git a/strbuf.h b/strbuf.h index 9b9e861..3116387 100644 --- a/strbuf.h +++ b/strbuf.h @@ -78,7 +78,14 @@ static inline void strbuf_setlen(struct strbuf *sb, size_t len) { /*----- content related -----*/ extern void strbuf_rtrim(struct strbuf *); -extern int strbuf_cmp(struct strbuf *, struct strbuf *); +static inline int strbuf_cmp(const struct strbuf *a, const struct strbuf *b) +{ + int len = a->len < b->len ? a->len: b->len; + int cmp = memcmp(a->buf, b->buf, len); + if (cmp) + return cmp; + return a->len < b->len ? -1: a->len != b->len; +} /*----- add data in your buffer -----*/ static inline void strbuf_addch(struct strbuf *sb, int c) {
-- 1.5.3.4.223.g78587
Re: [PATCH] Make strbuf_cmp inline, constify its arguments and optimize it a bit
Alex Riesen <raa.lkml@gmail.com> wrote:
Show 8 quoted lines
> +static inline int strbuf_cmp(const struct strbuf *a, const struct strbuf *b)
> +{
> + int len = a->len < b->len ? a->len: b->len;
> + int cmp = memcmp(a->buf, b->buf, len);
> + if (cmp)
> + return cmp;
> + return a->len < b->len ? -1: a->len != b->len;
> +}strbuf->buf is always non-NULL and NUL-terminated so you could just do
static inline int strbuf_cmp(const struct strbuf *a, const struct strbuf *b)
{
int len = a->len < b->len ? a->len : b->len;
return memcmp(a->buf, b->buf, len + 1);
}Pierre HabouzitRe: [PATCH] Make strbuf_cmp inline, constify its arguments and optimize it a bit
On Sun, Oct 07, 2007 at 02:24:25PM +0000, Timo Hirvonen wrote:
Show 18 quoted lines
> Alex Riesen <raa.lkml@gmail.com> wrote:
>
> > +static inline int strbuf_cmp(const struct strbuf *a, const struct strbuf *b)
> > +{
> > + int len = a->len < b->len ? a->len: b->len;
> > + int cmp = memcmp(a->buf, b->buf, len);
> > + if (cmp)
> > + return cmp;
> > + return a->len < b->len ? -1: a->len != b->len;
> > +}
>
> strbuf->buf is always non-NULL and NUL-terminated so you could just do
>
> static inline int strbuf_cmp(const struct strbuf *a, const struct strbuf *b)
> {
> int len = a->len < b->len ? a->len : b->len;
> return memcmp(a->buf, b->buf, len + 1);
> }doesn't work, because a buffer can have (in some very specific cases) an embeded NUL.
-- ·O· Pierre Habouzit ··O madcoder@debian.org OOO http://www.madism.org
Miles BaderRe: [PATCH] Make strbuf_cmp inline, constify its arguments and optimize it a bit
Pierre Habouzit <madcoder@debian.org> writes:
Show 10 quoted lines
>> strbuf->buf is always non-NULL and NUL-terminated so you could just do
>>
>> static inline int strbuf_cmp(const struct strbuf *a, const struct strbuf *b)
>> {
>> int len = a->len < b->len ? a->len : b->len;
>> return memcmp(a->buf, b->buf, len + 1);
>> }
>
> doesn't work, because a buffer can have (in some very specific cases)
> an embeded NUL.Couldn't you then just do:
int len = a->len < b->len ? a->len : b->len;
int cmp = memcmp(a->buf, b->buf, len);
if (cmp == 0)
cmp = b->len - a->len;
return cmp;[In the case where one string is a prefix of the other, then the longer one is "greater".]
?
-Miles
-- "Suppose He doesn't give a shit? Suppose there is a God but He just doesn't give a shit?" [George Carlin]
David KastrupRe: [PATCH] Make strbuf_cmp inline, constify its arguments and optimize it a bit
Miles Bader <miles@gnu.org> writes:
Show 24 quoted lines
> Pierre Habouzit <madcoder@debian.org> writes:
>>> strbuf->buf is always non-NULL and NUL-terminated so you could just do
>>>
>>> static inline int strbuf_cmp(const struct strbuf *a, const struct strbuf *b)
>>> {
>>> int len = a->len < b->len ? a->len : b->len;
>>> return memcmp(a->buf, b->buf, len + 1);
>>> }
>>
>> doesn't work, because a buffer can have (in some very specific cases)
>> an embeded NUL.
>
> Couldn't you then just do:
>
> int len = a->len < b->len ? a->len : b->len;
> int cmp = memcmp(a->buf, b->buf, len);
> if (cmp == 0)
> cmp = b->len - a->len;
> return cmp;
>
> [In the case where one string is a prefix of the other, then the longer
> one is "greater".]
>
> ?I fail to see where this variant is simpler than what we started the journey of simplification from.
The only change I consider worth checking from the whole series in this thread is making the function inline. All the rest pretty much was worse than what we started from in that it needed to reevaluate more conditions and turned out more complicated and obfuscate even to the human reader.
-- David Kastrup, Kriemhildstr. 15, 44793 Bochum
Alex RiesenRe: [PATCH] Make strbuf_cmp inline, constify its arguments and optimize it a bit
David Kastrup, Sun, Oct 07, 2007 18:07:17 +0200:
Show 32 quoted lines
> Miles Bader <miles@gnu.org> writes:
>
> > Pierre Habouzit <madcoder@debian.org> writes:
> >>> strbuf->buf is always non-NULL and NUL-terminated so you could just do
> >>>
> >>> static inline int strbuf_cmp(const struct strbuf *a, const struct strbuf *b)
> >>> {
> >>> int len = a->len < b->len ? a->len : b->len;
> >>> return memcmp(a->buf, b->buf, len + 1);
> >>> }
> >>
> >> doesn't work, because a buffer can have (in some very specific cases)
> >> an embeded NUL.
> >
> > Couldn't you then just do:
> >
> > int len = a->len < b->len ? a->len : b->len;
> > int cmp = memcmp(a->buf, b->buf, len);
> > if (cmp == 0)
> > cmp = b->len - a->len;
> > return cmp;
> >
> > [In the case where one string is a prefix of the other, then the longer
> > one is "greater".]
> >
> > ?
>
> I fail to see where this variant is simpler than what we started the
> journey of simplification from.
>
> The only change I consider worth checking from the whole series in
> this thread is making the function inline. It also makes arguments const (which admittedly wont make it faster).
> ... All the rest pretty much > was worse than what we started from in that it needed to reevaluate > more conditions and turned out more complicated and obfuscate even to > the human reader.
it _is_ smaller. And it is _measurably_ faster on that thing I have at home (and old p4).
Wincent ColaiutaRe: [PATCH] Make strbuf_cmp inline, constify its arguments and optimize it a bit
El 7/10/2007, a las 23:54, Alex Riesen escribió:
Show 7 quoted lines
>> ... All the rest pretty much >> was worse than what we started from in that it needed to reevaluate >> more conditions and turned out more complicated and obfuscate even to >> the human reader. > > it _is_ smaller. And it is _measurably_ faster on that thing I have at > home (and old p4).
Can we see the numbers and the steps used to obtain them? I'm also a little bit confused about how an inlined function can lead to a smaller executable... or did you just mean lines-of-code?
Cheers, Wincent
Alex RiesenRe: [PATCH] Make strbuf_cmp inline, constify its arguments and optimize it a bit
Wincent Colaiuta, Mon, Oct 08, 2007 00:12:17 +0200:
Show 13 quoted lines
> El 7/10/2007, a las 23:54, Alex Riesen escribió: > > >>... All the rest pretty much > >>was worse than what we started from in that it needed to reevaluate > >>more conditions and turned out more complicated and obfuscate even to > >>the human reader. > > > >it _is_ smaller. And it is _measurably_ faster on that thing I have at > >home (and old p4). > > Can we see the numbers and the steps used to obtain them? I'm also a > little bit confused about how an inlined function can lead to a > smaller executable... or did you just mean lines-of-code?
I did mean the bytes of object code. I never said it produces a smaller executable.
I compiled with gcc -O2 and -O4, gcc 4.1.2 (Ubuntu 4.1.2-0ubuntu4). Cut the functions out into their own files and compile them to get the object code. Compile with -S (assembly) to examine the generated code. Compare.
#include <stdint.h> #include <unistd.h> #include <stdlib.h> #include <sys/time.h> #include <stdio.h> #include <string.h>
struct strbuf {
size_t alloc;
size_t len;
char *buf;
};int strbuf_cmp2(struct strbuf *a, struct strbuf *b)
{
int len = a->len < b->len ? a->len: b->len;
int cmp = memcmp(a->buf, b->buf, len);
if (cmp)
return cmp;
return a->len < b->len ? -1: a->len != b->len;
}int strbuf_cmp1(struct strbuf *a, struct strbuf *b)
{
int cmp;
if (a->len < b->len) {
cmp = memcmp(a->buf, b->buf, a->len);
return cmp ? cmp : -1;
} else {
cmp = memcmp(a->buf, b->buf, b->len);
return cmp ? cmp : a->len != b->len;
}
}int main(int argc, char *argv[], char *envp[])
{
struct strbuf s1 = {
.alloc = 0,
.len = 50,
.buf = "01234567890123456789012345678901234567890123456789",
};
struct strbuf s2 = {
.alloc = 0,
.len = 50,
.buf = "0123456789012345678901234567890123456789",
};
struct strbuf s3 = {
.alloc = 0,
.len = 50,
.buf = "0123456789012345678901234567890123456789012345678x",
};
struct timeval tv1, tv2, diff;
unsigned n;
int result;
#define CYCLES 0xffffffffu strbuf_cmp1(&s1, &s2);
strbuf_cmp1(&s2, &s3);
result = 0;
gettimeofday(&tv1, NULL);
for (n = CYCLES; n--; ) {
result += strbuf_cmp1(&s1, &s2);
result += strbuf_cmp1(&s2, &s3);
result += strbuf_cmp1(&s1, &s3);
result += strbuf_cmp1(&s1, &s1);
result += n;
}
gettimeofday(&tv2, NULL);
timersub(&tv2, &tv1, &diff);
printf("ph=%ld.%ld (%d)\n", diff.tv_sec, diff.tv_usec, result); strbuf_cmp2(&s1, &s2);
strbuf_cmp2(&s2, &s3);
result = 0;
gettimeofday(&tv1, NULL);
for (n = CYCLES; n--; ) {
result += strbuf_cmp2(&s1, &s2);
result += strbuf_cmp2(&s2, &s3);
result += strbuf_cmp2(&s1, &s3);
result += strbuf_cmp2(&s1, &s1);
result += n;
}
gettimeofday(&tv2, NULL);
timersub(&tv2, &tv1, &diff);
printf("ar=%ld.%ld (%d)\n", diff.tv_sec, diff.tv_usec, result);
return 0;
}Miles BaderRe: [PATCH] Make strbuf_cmp inline, constify its arguments and optimize it a bit
Alex Riesen <raa.lkml@gmail.com> writes:
Show 8 quoted lines
> int strbuf_cmp2(struct strbuf *a, struct strbuf *b)
> {
> int len = a->len < b->len ? a->len: b->len;
> int cmp = memcmp(a->buf, b->buf, len);
> if (cmp)
> return cmp;
> return a->len < b->len ? -1: a->len != b->len;
> }BTW, why are you making such effort to return only -1, 0, or 1 in the last line? memcmp/strcmp make no such guarantee; e.g. glibc says:
The `strcmp' function compares the string S1 against S2, returning
a value that has the same sign as the difference between the first
differing pair of characters (interpreted as `unsigned char'
objects, then promoted to `int').If the two strings are equal, `strcmp' returns `0'.
A consequence of the ordering used by `strcmp' is that if S1 is an
initial substring of S2, then S1 is considered to be "less than"
S2.So I think the last line can just be:
return a->len - b->len;
-miles
-- Suburbia: where they tear out the trees and then name streets after them.
Pierre HabouzitRe: [PATCH] Make strbuf_cmp inline, constify its arguments and optimize it a bit
On Mon, Oct 08, 2007 at 01:45:27AM +0000, Miles Bader wrote:
Show 27 quoted lines
> Alex Riesen <raa.lkml@gmail.com> writes:
> > int strbuf_cmp2(struct strbuf *a, struct strbuf *b)
> > {
> > int len = a->len < b->len ? a->len: b->len;
> > int cmp = memcmp(a->buf, b->buf, len);
> > if (cmp)
> > return cmp;
> > return a->len < b->len ? -1: a->len != b->len;
> > }
>
> BTW, why are you making such effort to return only -1, 0, or 1 in the
> last line? memcmp/strcmp make no such guarantee; e.g. glibc says:
>
> The `strcmp' function compares the string S1 against S2, returning
> a value that has the same sign as the difference between the first
> differing pair of characters (interpreted as `unsigned char'
> objects, then promoted to `int').
>
> If the two strings are equal, `strcmp' returns `0'.
>
> A consequence of the ordering used by `strcmp' is that if S1 is an
> initial substring of S2, then S1 is considered to be "less than"
> S2.
>
> So I think the last line can just be:
>
> return a->len - b->len;Won't work because ->len are size_t and return value is int, so on 64 bits platform, this has chances to overflow.
FWIW I believe we are doing micro-benchs in a function that is used in 2 places in git right now.
-- ·O· Pierre Habouzit ··O madcoder@debian.org OOO http://www.madism.org
Re: [PATCH] Make strbuf_cmp inline, constify its arguments and optimize it a bit
* Pierre Habouzit:
Show 6 quoted lines
>> So I think the last line can just be: >> >> return a->len - b->len; > > Won't work because ->len are size_t and return value is int, so on 64 > bits platform, this has chances to overflow.
Nit: It can overflow on 32-bit, too.
And "int len" in the first line of the function body should be "size_t len".
Moving that to a compare_int/compare_size_t function should help; AFAIK there's no short idiom which does the job.
-- Florian Weimer <fweimer@bfk.de> BFK edv-consulting GmbH http://www.bfk.de/ Kriegsstraße 100 tel: +49-721-96201-1 D-76133 Karlsruhe fax: +49-721-96201-99
Alex RiesenRe: [PATCH] Make strbuf_cmp inline, constify its arguments and optimize it a bit
Florian Weimer, Mon, Oct 08, 2007 10:54:32 +0200:
> And "int len" in the first line of the function body should be > "size_t len".
right. Missed that.
Johannes SchindelinRe: [PATCH] Make strbuf_cmp inline, constify its arguments and optimize it a bit
Hi,
On Sun, 7 Oct 2007, Pierre Habouzit wrote:
Show 12 quoted lines
> On Sun, Oct 07, 2007 at 02:24:25PM +0000, Timo Hirvonen wrote:
>
> > strbuf->buf is always non-NULL and NUL-terminated so you could just do
> >
> > static inline int strbuf_cmp(const struct strbuf *a, const struct strbuf *b)
> > {
> > int len = a->len < b->len ? a->len : b->len;
> > return memcmp(a->buf, b->buf, len + 1);
> > }
>
> doesn't work, because a buffer can have (in some very specific cases)
> an embeded NUL.But it should work. The function memcmp() could not care less if there is a NUL or not, it just compares until it finds a difference.
Ciao, Dscho
Re: [PATCH] Make strbuf_cmp inline, constify its arguments and optimize it a bit
Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
Show 19 quoted lines
> Hi,
>
> On Sun, 7 Oct 2007, Pierre Habouzit wrote:
>
> > On Sun, Oct 07, 2007 at 02:24:25PM +0000, Timo Hirvonen wrote:
> >
> > > strbuf->buf is always non-NULL and NUL-terminated so you could just do
> > >
> > > static inline int strbuf_cmp(const struct strbuf *a, const struct strbuf *b)
> > > {
> > > int len = a->len < b->len ? a->len : b->len;
> > > return memcmp(a->buf, b->buf, len + 1);
> > > }
> >
> > doesn't work, because a buffer can have (in some very specific cases)
> > an embeded NUL.
>
> But it should work. The function memcmp() could not care less if there is
> a NUL or not, it just compares until it finds a difference.Almost. If a is "hello\0world" and b is "hello" then it would compare 6 characters from both and think the strings are equal.
Johannes SchindelinRe: [PATCH] Make strbuf_cmp inline, constify its arguments and optimize it a bit
Hi,
On Sun, 7 Oct 2007, Timo Hirvonen wrote:
Show 22 quoted lines
> Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
>
> > On Sun, 7 Oct 2007, Pierre Habouzit wrote:
> >
> > > On Sun, Oct 07, 2007 at 02:24:25PM +0000, Timo Hirvonen wrote:
> > >
> > > > strbuf->buf is always non-NULL and NUL-terminated so you could just do
> > > >
> > > > static inline int strbuf_cmp(const struct strbuf *a, const struct strbuf *b)
> > > > {
> > > > int len = a->len < b->len ? a->len : b->len;
> > > > return memcmp(a->buf, b->buf, len + 1);
> > > > }
> > >
> > > doesn't work, because a buffer can have (in some very specific cases)
> > > an embeded NUL.
> >
> > But it should work. The function memcmp() could not care less if there is
> > a NUL or not, it just compares until it finds a difference.
>
> Almost. If a is "hello\0world" and b is "hello" then it would compare 6
> characters from both and think the strings are equal.Good point.
Ciao, Dscho
Pierre HabouzitRe: [PATCH] Make strbuf_cmp inline, constify its arguments and optimize it a bit
On Sun, Oct 07, 2007 at 04:11:29PM +0000, Johannes Schindelin wrote:
Show 19 quoted lines
> Hi,
>
> On Sun, 7 Oct 2007, Pierre Habouzit wrote:
>
> > On Sun, Oct 07, 2007 at 02:24:25PM +0000, Timo Hirvonen wrote:
> >
> > > strbuf->buf is always non-NULL and NUL-terminated so you could just do
> > >
> > > static inline int strbuf_cmp(const struct strbuf *a, const struct strbuf *b)
> > > {
> > > int len = a->len < b->len ? a->len : b->len;
> > > return memcmp(a->buf, b->buf, len + 1);
> > > }
> >
> > doesn't work, because a buffer can have (in some very specific cases)
> > an embeded NUL.
>
> But it should work. The function memcmp() could not care less if there is
> a NUL or not, it just compares until it finds a difference.not if your one of your strbuf has as prefix, the other followed by '\0', then anything else (including nothing ;p).
Your test would yield equality.
-- ·O· Pierre Habouzit ··O madcoder@debian.org OOO http://www.madism.org
David KastrupRe: [PATCH] Make strbuf_cmp inline, constify its arguments and optimize it a bit
Alex Riesen <raa.lkml@gmail.com> writes:
> It is definitely less code (also object code). It is not always > measurably faster (but mostly is).
Show 12 quoted lines
> -int strbuf_cmp(struct strbuf *a, struct strbuf *b)
> -{
> - int cmp;
> - if (a->len < b->len) {
> - cmp = memcmp(a->buf, b->buf, a->len);
> - return cmp ? cmp : -1;
> - } else {
> - cmp = memcmp(a->buf, b->buf, b->len);
> - return cmp ? cmp : a->len != b->len;
> - }
> -}
> -Show 8 quoted lines
> +static inline int strbuf_cmp(const struct strbuf *a, const struct strbuf *b)
> +{
> + int len = a->len < b->len ? a->len: b->len;
> + int cmp = memcmp(a->buf, b->buf, len);
> + if (cmp)
> + return cmp;
> + return a->len < b->len ? -1: a->len != b->len;
> +}My guess is that you are conflating two issues about speed here: the inlining will like speed the stuff up. But having to evaluate the (a->len < b->len) comparison twice will likely slow it down.
So if you do any profiling, you should do it on both separate angles of this patch.
-- David Kastrup, Kriemhildstr. 15, 44793 Bochum
Alex RiesenRe: [PATCH] Make strbuf_cmp inline, constify its arguments and optimize it a bit
David Kastrup, Sun, Oct 07, 2007 16:24:57 +0200:
Show 30 quoted lines
> Alex Riesen <raa.lkml@gmail.com> writes:
>
> > It is definitely less code (also object code). It is not always
> > measurably faster (but mostly is).
>
> > -int strbuf_cmp(struct strbuf *a, struct strbuf *b)
> > -{
> > - int cmp;
> > - if (a->len < b->len) {
> > - cmp = memcmp(a->buf, b->buf, a->len);
> > - return cmp ? cmp : -1;
> > - } else {
> > - cmp = memcmp(a->buf, b->buf, b->len);
> > - return cmp ? cmp : a->len != b->len;
> > - }
> > -}
> > -
>
> > +static inline int strbuf_cmp(const struct strbuf *a, const struct strbuf *b)
> > +{
> > + int len = a->len < b->len ? a->len: b->len;
> > + int cmp = memcmp(a->buf, b->buf, len);
> > + if (cmp)
> > + return cmp;
> > + return a->len < b->len ? -1: a->len != b->len;
> > +}
>
> My guess is that you are conflating two issues about speed here: the
> inlining will like speed the stuff up. But having to evaluate the
> (a->len < b->len) comparison twice will likely slow it down.Can't the result of the expression be reused in compiled? Isn't it a common expression?
> So if you do any profiling, you should do it on both separate angles > of this patch. >
I compared the inlined versions of both.
David KastrupRe: [PATCH] Make strbuf_cmp inline, constify its arguments and optimize it a bit
Alex Riesen <raa.lkml@gmail.com> writes:
Show 34 quoted lines
> David Kastrup, Sun, Oct 07, 2007 16:24:57 +0200:
>> Alex Riesen <raa.lkml@gmail.com> writes:
>>
>> > It is definitely less code (also object code). It is not always
>> > measurably faster (but mostly is).
>>
>> > -int strbuf_cmp(struct strbuf *a, struct strbuf *b)
>> > -{
>> > - int cmp;
>> > - if (a->len < b->len) {
>> > - cmp = memcmp(a->buf, b->buf, a->len);
>> > - return cmp ? cmp : -1;
>> > - } else {
>> > - cmp = memcmp(a->buf, b->buf, b->len);
>> > - return cmp ? cmp : a->len != b->len;
>> > - }
>> > -}
>> > -
>>
>> > +static inline int strbuf_cmp(const struct strbuf *a, const struct strbuf *b)
>> > +{
>> > + int len = a->len < b->len ? a->len: b->len;
>> > + int cmp = memcmp(a->buf, b->buf, len);
>> > + if (cmp)
>> > + return cmp;
>> > + return a->len < b->len ? -1: a->len != b->len;
>> > +}
>>
>> My guess is that you are conflating two issues about speed here: the
>> inlining will like speed the stuff up. But having to evaluate the
>> (a->len < b->len) comparison twice will likely slow it down.
>
> Can't the result of the expression be reused in compiled?
> Isn't it a common expression?No, since the call to memcmp might change a->len or b->len. A standard-compliant C compiler can't make assumptions about what memcmp might or might not touch unless both a and b can be shown to refer to variables with an address never passed out of the scope of the compilation unit.
>> So if you do any profiling, you should do it on both separate >> angles of this patch. > > I compared the inlined versions of both.
Interesting.
-- David Kastrup, Kriemhildstr. 15, 44793 Bochum
Alex RiesenRe: [PATCH] Make strbuf_cmp inline, constify its arguments and optimize it a bit
David Kastrup, Sun, Oct 07, 2007 18:27:15 +0200:
Show 18 quoted lines
> Alex Riesen <raa.lkml@gmail.com> writes:
> >> > +static inline int strbuf_cmp(const struct strbuf *a, const struct strbuf *b)
> >> > +{
> >> > + int len = a->len < b->len ? a->len: b->len;
> >> > + int cmp = memcmp(a->buf, b->buf, len);
> >> > + if (cmp)
> >> > + return cmp;
> >> > + return a->len < b->len ? -1: a->len != b->len;
> >> > +}
> >>
> >> My guess is that you are conflating two issues about speed here: the
> >> inlining will like speed the stuff up. But having to evaluate the
> >> (a->len < b->len) comparison twice will likely slow it down.
> >
> > Can't the result of the expression be reused in compiled?
> > Isn't it a common expression?
>
> No, since the call to memcmp might change a->len or b->len. AHuh?! How's that? It is not even given them!
Jeff KingRe: [PATCH] Make strbuf_cmp inline, constify its arguments and optimize it a bit
On Sun, Oct 07, 2007 at 11:57:49PM +0200, Alex Riesen wrote:
Show 6 quoted lines
> > > Can't the result of the expression be reused in compiled? > > > Isn't it a common expression? > > > > No, since the call to memcmp might change a->len or b->len. A > > Huh?! How's that? It is not even given them!
But they are non-local variables (they are part of structs passed in as pointers), so that translation unit has no idea how they are allocated. They could be globals that memcmp mucks with as a side effect.
That being said, standards-conforming compilers _can_ realize that memcmp is a special, standards-defined function with no side effects and act accordingly. gcc provides the 'pure' function attribute for this purpose, which is used by glibc.
-Peff