RE: [Patch] Fix istream::ignore + minor things

["Damour, James A" <James dot Damour at dfa dot state dot ny dot us>]

I beg the indulgence of this list, but I was hoping that Mr. Myers could
clarify whether his remarks and observations modify the previous
position that I summarized as:

| If I understand your position correctly, you recommend against the use

| of the __builtin_expect function except when performance data shows a 
| clear improvement.  Put a different way, you prefer brevity and 
| clarity in the source over some unproven performance improvement.  
| Thus, I believe that your answer to my question is, "unless the 
| performance improvement is demonstrated to be substantial, the more 
| simple construct has greater benefit".

This discussion is about a patch to istream::ignore; my understanding is
that istream::ignore is not "very commonly inlined in inner loops" and
therefore is not worthy of annotation without specific benchmarks
demonstrating the worth of the __builtin_expect function.  Even if my
understanding was incorrect, or if we *were* talking about a function
that was "very commonly inlined in inner loops", wouldn't we want to see
at least *one* benchmark proving that the performance was improved
"substantially"? 

Taking to heart your recommendation not to overgeneralize the results of
testing, wouldn't a "substantial" performance improvement on a
**single** architecture justify the added complexity (and loss of
portability) of using the __builtin_expect function?

I realize that compiler writers aren't willing to implement features
that don't get used, and that ther time is best spent elsewhere, but
then again I don't think that application programmers should use
features that aren't implemented yet.  I think that the only solution to
this Catch-22 is the one that Mr. Carlini will be pursuing: analyse
profile data to identify potential bottlenecks, and collaborate with the
compiler people to address the bottlenecks.  Both sides (app devs and
compiler devs) have to work together to handle these cross-group issues.
Until they do, each group can better spend their time picking the
low-hanging fruit that fall entirely within their own purview.

-----Original Message-----
From: Nathan Myers [mailto:ncm-nospam@cantrip.org] 
Sent: Tuesday, May 25, 2004 3:47 PM
To: libstdc++
Subject: Re: [Patch] Fix istream::ignore + minor things

On Tue, May 25, 2004 at 08:22:22PM +0200, Paolo Carlini wrote:
> Thanks again: we should analyze carefully this data, check whether 
> something has changed in gcc3.4/mainline in the branch prediction area

> (*), and definitely interact with the compiler people.

Two remarks:

First, while P4 is pretty important, lots of other x86s are affected.
Most code is still built for 386, where simple code-motion is all you
can do, even though it gets run mostly on P3s and P4s anyhow.  (To me
that means code built for 386 ought, by default, to be optimized
assuming P4 or K8 machine characteristics, but that's another rant.)
Furthermore, there are lots of other targets, and some of them may be
making much better use of these attributes than the x86 code generator
does.  While P4 and K8 are very good at using runtime discovery to
inform branch prediction, other targets might not be, and would be much
more strongly affected.

This means we must be careful not to overgeneralize results of testing.


Second, compiler core people seem very sensitive to whether a feature is
actually used in the code they encounter (typically in bug reports)
before they put any time into improving their implementation of that
feature.  At the moment, they probably hardly ever see branch prediction
attributes, because its expression isn't portable.  If we start using
them heavily in our library, the compiler people will be more likely to
start paying them some attention.  Once such annotations actually help,
then users will start using them in their own code too, and a some ISO
committee might then feel obliged to make it portable.  

This isn't a call to start annotating all conditionals, wholesale.
Practically, I think this means that for bits of code that we already
*know* are very commonly inlined in inner loops, we shouldn't demand
complete benchmark tests on all architecures before we welcome patches
to annotate those.

Something else recommended by Geert Bosch (in 2004-05/msg01253.html) is
to mark blocks that throw as no-return, because, as he says, "This will
cause GCC to automatically consider [entry] to it unlikely.  The
advantage is that you don't have to litter your source code at every
call site" -- or, presumably, the conditional expressions themselves.

Nathan Myers
ncm-nospam@cantrip.org