svn commit: r227812 - head/lib/libc/string

[Eitan Adler]

Meta comment:
I should have sent this patch to -hackers prior to seeking approval to
commit. I learned my lesson and will seek wider review before making
such changes in the future.
On Tue, Nov 22, 2011 at 6:21 AM, Bruce Evans <brde at optusnet.com.au> wrote:
I saw your email about the style changes. I'd like to flesh out what
changes should be made first before making another commit>> I guess
I'm a little confused.  Do we really have profiling>> information at
this level that suggests the overhead of the branch is>> significant?
I thought most hardware had pretty good>> branch-prediction,
particularly with speculative execution.
I made this change at the direction of theraven at . I assume he knows a
little bit more about compilers than I do. ;)It seems from the rest of
this thread that even if this were an optimization it still shouldn't
be done at source code code level.
> Every time I have profiled the string functions, changes like this always> have an insignificant effect, even in benchmarks.  Normal code spends> perhaps 0.01% of its time calling string functions, so the improvements> from optimizing string functions are 0.01% of an insignificant amount.
The problem with profiling this type of change is that it is hard to
find a good representative benchmark. I could easily write code that
will show you that adding the equality check is a good idea or that it
is a horrible idea. IMHO it saves enough time when they are equal, but
loses almost no time when the strings are not equal.
>> Wouldn't something like __predict_false() have more value for>> performance,
It seems that at most this couldn't hurt.
On Tue, Nov 22, 2011 at 10:33 AM, David Schultz <das at freebsd.org> wrote:
> Third, it's not clear that checking whether s1 == s2 is even an
> optimization.  Most programs simply aren't going to pass identical
> pointers as arguments to strcmp(), so for the overwhelming
> majority of cases, this is just a wasted test and a wasted slot in
> a branch predict table.  (FWIW, I doubt that a realistic benchmark
> would demonstrate any measurable difference.)

It is fairly common for programs to compare a value passed to a
function to a array of strings. Also note that for the strn*cmp
functions we already have a branch so this isn't always a wasted slot.

Here is what I'd like to do next:

- fix bde@'s style nits
- change the | to a || and remove the comment
- but leave the equality check as is.
- find a src committer to approve the patch
- go back to working on ports for a while ;)

Is this the right course of action? Or should I just revert both
commits entirely?


-- 
Eitan Adler
Ports committer
X11, Bugbusting teams