[Bug c++/52241] New: Performance degradation of 447.dealII on corei7 at spec2006_base32.

[Bug c++/52241] New: Performance degradation of 447.dealII on corei7 at spec2006_base32.

[vbyakovl23 at gmail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52241

             Bug #: 52241
           Summary: Performance degradation of 447.dealII on corei7 at
                    spec2006_base32.
    Classification: Unclassified
           Product: gcc
           Version: unknown
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c++
        AssignedTo: unassigned@gcc.gnu.org
        ReportedBy: vbyakovl23@gmail.com


Guilty commit

c1e8b3edf7b5038f070c7a9732e58d066081a636 is the first bad commit
commit c1e8b3edf7b5038f070c7a9732e58d066081a636
Author: bkoz <bkoz@138bc75d-0d04-0410-961f-82ee72b054a4>
Date: Mon Jan 23 23:12:01 2012 +0000

caused performance degradation of 447.dealII benchspec 2006.It was happened
because there are no inlining a library routine '_Rb_tree_node_base' from
libstdc++-v3/src/tree.cc

const _Rb_tree_node_base*  _Rb_tree_increment(const _Rb_tree_node_base* __x)
throw ()
{    
  return Rb_tree_increment(const_cast&lt;_Rb_tree_node_base*&gt;(_x));
}

I found out that the degradation is caused by absence of -fPIC flag in
compilation command line. If I add the flag to command line the inlining is
happened.

[Bug c++/52241] Performance degradation of 447.dealII on corei7 at spec2006_base32.

[izamyatin at gmail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52241

--- Comment #1 from Igor Zamyatin <izamyatin at gmail dot com> 2012-02-15 09:06:49 UTC ---
BTW, this is a 4.7 regression

[Bug c++/52241] Performance degradation of 447.dealII on corei7 at spec2006_base32.

[paolo.carlini at oracle dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52241

--- Comment #2 from Paolo Carlini <paolo.carlini at oracle dot com> 2012-02-15 16:39:00 UTC ---
I don't understand what you mean by "inlining", since '_Rb_tree_node_base' is a
*type* not a function.

Anyway, I don't see how Benjamin's split could have caused inlining issues.

[Bug c++/52241] Performance degradation of 447.dealII on corei7 at spec2006_base32.

[vbyakovl23 at gmail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52241

--- Comment #3 from Vladimir Yakovlev <vbyakovl23 at gmail dot com> 2012-02-16 08:58:45 UTC ---
Created attachment 26674
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=26674
Dump of bad case (with -fPIC -DPIC)

[Bug c++/52241] Performance degradation of 447.dealII on corei7 at spec2006_base32.

[vbyakovl23 at gmail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52241

--- Comment #4 from Vladimir Yakovlev <vbyakovl23 at gmail dot com> 2012-02-16 09:00:32 UTC ---
Created attachment 26675
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=26675
Dump of good case (without -fPIC -DPIC)

[Bug c++/52241] Performance degradation of 447.dealII on corei7 at spec2006_base32.

[vbyakovl23 at gmail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52241

--- Comment #5 from Vladimir Yakovlev <vbyakovl23 at gmail dot com> 2012-02-16 09:05:49 UTC ---
(In reply to comment #2)
> I don't understand what you mean by "inlining", since '_Rb_tree_node_base' is a
> *type* not a function.

This is a constructor

> Anyway, I don't see how Benjamin's split could have caused inlining issues.

I don't know also but compare inline dumps with and witout -fPIC -DPIC
(attached).

I used command line from log of compiler build

/export/users/vbyakovl/workspaces/581/build-bad/./gcc/xgcc -shared-libgcc
-B/export/users/vbyakovl/workspaces/581/build-bad/./gcc -nostdinc++
-L/export/users/vbyakovl/workspaces/581/build-bad/x86_64-unknown-linux-gnu/libstdc++-v3/src
-L/export/users/vbyakovl/workspaces/581/build-bad/x86_64-unknown-linux-gnu/libstdc++-v3/src/.libs
-B/export/users/vbyakovl/workspaces/581/install-bad/x86_64-unknown-linux-gnu/bin/
-B/export/users/vbyakovl/workspaces/581/install-bad/x86_64-unknown-linux-gnu/lib/
-isystem
/export/users/vbyakovl/workspaces/581/install-bad/x86_64-unknown-linux-gnu/include
-isystem
/export/users/vbyakovl/workspaces/581/install-bad/x86_64-unknown-linux-gnu/sys-include
-I/export/users/vbyakovl/workspaces/581/gcc/libstdc++-v3/../libgcc
-I/export/users/vbyakovl/workspaces/581/build-bad/x86_64-unknown-linux-gnu/libstdc++-v3/include/x86_64-unknown-linux-gnu
-I/export/users/vbyakovl/workspaces/581/build-bad/x86_64-unknown-linux-gnu/libstdc++-v3/include
-I/export/users/vbyakovl/workspaces/581/gcc/libstdc++-v3/libsupc++
-fno-implicit-templates -Wall -Wextra -Wwrite-strings -Wcast-qual
-fdiagnostics-show-location=once -Wabi -ffunction-sections -fdata-sections
-frandom-seed=tree.lo -g -O2 -D_GNU_SOURCE -c
../../../../../gcc/libstdc++-v3/src/c++98/tree.cc  -fPIC -DPIC -o tree.o
-fdump-ipa-inlin

[Bug c++/52241] Performance degradation of 447.dealII on corei7 at spec2006_base32.

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52241

--- Comment #6 from Jakub Jelinek <jakub at gcc dot gnu.org> 2012-02-16 09:26:58 UTC ---
The fact that -fPIC code is often slower than -fno-pic code on many targets
isn't that surprising.  But libstdc++.so.6's tree.cc has been compiled with
-fPIC -DPIC before Benjamin's change and is compiled with those flags after
those changes as well (on x86_64 you would likely not being able to link
libstdc++.so.6 otherwise).
So, are you linking libstdc++ statically into the benchmark?  Why?  That isn't
what people normally do, nor they should be doing.  It is true that libstdc++.a
now contains PIC code after the move to the convenience libraries, while
previously it contained non-PIC code (with the exception of libsupc++ stuff
that was PIC already before those changes).

[Bug c++/52241] Performance degradation of 447.dealII on corei7 at spec2006_base32.

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52241

--- Comment #7 from Richard Guenther <rguenth at gcc dot gnu.org> 2012-02-16 10:37:34 UTC ---
It is desirable that libstdc++.a contains PIC code, otherwise you cannot
link it statically into shared libraries or into position independent
executables.

[Bug c++/52241] Performance degradation of 447.dealII on corei7 at spec2006_base32.

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52241

--- Comment #8 from Jakub Jelinek <jakub at gcc dot gnu.org> 2012-02-16 10:57:19 UTC ---
Note that if more inlining in tree.cc even in -fPIC code helps more than just a
single benchmark, we could consider compiling that file with -O3, or use some
attributes/pragmas to ensure it is inlined.  So perhaps start with compiling
tree.cc with -O3 instead of -O2 (in both cases with -fPIC -DPIC) and see if it
helps and how much.

[Bug c++/52241] Performance degradation of 447.dealII on corei7 at spec2006_base32.

[paolo.carlini at oracle dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52241

--- Comment #9 from Paolo Carlini <paolo.carlini at oracle dot com> 2012-02-16 13:39:21 UTC ---
(In reply to comment #5)
> (In reply to comment #2)
> > I don't understand what you mean by "inlining", since '_Rb_tree_node_base' is a
> > *type* not a function.
> 
> This is a constructor

What? I see only free functions, like _Rb_tree_increment, and pointers to types
like _Rb_tree_node_base. But I see that probably you just had a typo in your
original message, you meant that the _Rb_tree_increment(_Rb_tree_node_base*)
call is not inlined in _Rb_tree_increment(const _Rb_tree_node_base*).

Still, I don't see *what* really changed post-Benjamin split and *why*. Jakub
may have already figured out that, I don't.

[Bug c++/52241] Performance degradation of 447.dealII on corei7 at spec2006_base32.

[paolo.carlini at oracle dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52241

--- Comment #10 from Paolo Carlini <paolo.carlini at oracle dot com> 2012-02-16 13:45:53 UTC ---
By the way, I don't see anything wrong with explicitly marking
_Rb_tree_increment(_Rb_tree_node_base*) inline if that helps in some special
circumstances.

[Bug c++/52241] Performance degradation of 447.dealII on corei7 at spec2006_base32.

[paolo.carlini at oracle dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52241

--- Comment #13 from Paolo Carlini <paolo.carlini at oracle dot com> 2012-02-16 13:52:38 UTC ---
Thanks Jakub that's definitely possible, and my knowledge of libtool is very
weak anyway. My operative suggestion stands, I guess.

[Bug c++/52241] Performance degradation of 447.dealII on corei7 at spec2006_base32.

[paolo.carlini at oracle dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52241

--- Comment #12 from Paolo Carlini <paolo.carlini at oracle dot com> 2012-02-16 13:50:46 UTC ---
... besides the trivial fact that then isn't exported anymore. For that we can
simply refactor the actual code and call it as-is or via the const_cast from
the two exported _Rb_tree_increment.

[Bug c++/52241] Performance degradation of 447.dealII on corei7 at spec2006_base32.

[paolo.carlini at oracle dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52241

--- Comment #14 from Paolo Carlini <paolo.carlini at oracle dot com> 2012-02-16 13:59:20 UTC ---
Created attachment 26676
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=26676
Draft

Something like this. I suppose either static or inline should do the trick.
Maybe submitter can test both variants.

[Bug c++/52241] Performance degradation of 447.dealII on corei7 at spec2006_base32.

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52241

--- Comment #11 from Jakub Jelinek <jakub at gcc dot gnu.org> 2012-02-16 13:48:59 UTC ---
libtool apparently creates convenience libraries only with -fPIC stuff in it,
instead of having two sets of objects for each source, one -fPIC and one
non-fPIC.
Not sure if it is possible to override that behavior somehow.
So, when in 4.6 we had just libsupc++_convenience.la linked into libstdc++.la,
only the libsupc++ objects were -fPIC in libstdc++.a and the rest, not coming
from convenience library, was -fno-pic.  Now that everything else comes also
from convenience libraries, everything is -fPIC.  Not sure what is desirable...

[Bug c++/52241] Performance degradation of 447.dealII on corei7 at spec2006_base32.

[paolo.carlini at oracle dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52241

--- Comment #15 from Paolo Carlini <paolo.carlini at oracle dot com> 2012-02-16 15:04:42 UTC ---
Note, if we decide to do this at the level of tree.cc, we should consistently
change _Rb_tree_decrement too.

[Bug c++/52241] Performance degradation of 447.dealII on corei7 at spec2006_base32.

[izamyatin at gmail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52241

--- Comment #16 from Igor Zamyatin <izamyatin at gmail dot com> 2012-02-19 18:58:41 UTC ---
Jakub, could you please clarify your statement - "But libstdc++.so.6's tree.cc
has been compiled with
-fPIC -DPIC before Benjamin's change and is compiled with those flags after
those changes as well"

Compiler logs show somwhat opposite to this statement:

Before Benjamin's commit libtool is called without -prefer-pic:

/bin/sh ../libtool --tag CXX   --mode=compile .... -fno-implicit-templates
-Wall -Wextra -Wwrite-strings -Wcast-qual  -fdiagnostics-show-location=once
-Wabi  -ffunction-sections -fdata-sections  -frandom-seed=tree.lo -g -O2
-D_GNU_SOURCE -c -o tree.lo ../../../../libstdc++-v3/src/tree.cc

and then 

libtool: compile:  /export/users/izamyati/....././gcc/xgcc .... 
-fno-implicit-templates -Wall -Wextra -Wwrite-strings -Wcast-qual
-fdiagnostics-show-location=once -Wabi -ffunction-sections -fdata-sections
-frandom-seed=tree.lo -g -O2 -D_GNU_SOURCE -c
../../../../libstdc++-v3/src/tree.cc -o tree.o >/dev/null 2>&1.

But in the same time there is the following

libtool: compile:  /export/users/izamyati/....././gcc/xgcc ....
-fno-implicit-templates -Wall -Wextra -Wwrite-strings -Wcast-qual
-fdiagnostics-show-location=once -Wabi -ffunction-sections -fdata-sections
-frandom-seed=tree.lo -g -O2 -D_GNU_SOURCE -c
../../../../libstdc++-v3/src/tree.cc  -fPIC -DPIC -o .libs/tree.o   which is
absent after Benjamin's commit.

Does all this mean that actually tree.cc compiled with fPIC?

[Bug c++/52241] Performance degradation of 447.dealII on corei7 at spec2006_base32.

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52241

--- Comment #17 from Jakub Jelinek <jakub at gcc dot gnu.org> 2012-02-19 19:24:05 UTC ---
libstdc++.so.6 obviously contained -fPIC code before and does now as well, and
people really should be using the shared library (almost) always.
See http://www.akkadia.org/drepper/no_static_linking.html for some details.
So, if you are doing SPEC testing with -static, it probably just doesn't test
what people normally use or should use.
Can you please test Paolo's patch, if the slowdown you see is inherent in the
PIC code, or if it is about the different inlining decision?  Thanks.

[Bug c++/52241] Performance degradation of 447.dealII on corei7 at spec2006_base32.

[vbyakovl23 at gmail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52241

--- Comment #18 from Vladimir Yakovlev <vbyakovl23 at gmail dot com> 2012-02-20 05:37:32 UTC ---
I tested Paolo's patch and got acceleration on 447.dial

    base: +7.36%
    peak: +5.97%

Also I looked  dumps: the new routine 'local_Rb_tree_increment' in inlined now
in both uses.

[Bug c++/52241] Performance degradation of 447.dealII on corei7 at spec2006_base32.

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52241

--- Comment #19 from Jakub Jelinek <jakub at gcc dot gnu.org> 2012-02-20 08:37:09 UTC ---
Nice, so we want Paolo's patch.  Out of interest, what are the 447.deal numbers
when comparing linking against old (pre-Benjamin's commit) libstdc++.a and
current libstdc++.a with Paolo's patch (or libstdc++.so.6, i.e. without
-static)?

[Bug c++/52241] Performance degradation of 447.dealII on corei7 at spec2006_base32.

[paolo.carlini at oracle dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52241

Paolo Carlini <paolo.carlini at oracle dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |ASSIGNED
   Last reconfirmed|                            |2012-02-20
         AssignedTo|unassigned at gcc dot       |paolo.carlini at oracle dot
                   |gnu.org                     |com
   Target Milestone|---                         |4.7.0
     Ever Confirmed|0                           |1

[Bug libstdc++/52241] Performance degradation of 447.dealII on corei7 at spec2006_base32.

[vbyakovl23 at gmail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52241

--- Comment #20 from Vladimir Yakovlev <vbyakovl23 at gmail dot com> 2012-02-20 18:04:45 UTC ---
(In reply to comment #19)
> Nice, so we want Paolo's patch.  Out of interest, what are the 447.deal numbers
> when comparing linking against old (pre-Benjamin's commit) libstdc++.a and
> current libstdc++.a with Paolo's patch (or libstdc++.so.6, i.e. without
> -static)?

Runspec numbers (as runspec prints) for both static and dynamic

Static
Base:
    Old: 447.dealII      11440        324       35.3 * 
    New: 447.dealII      11440        302       37.9 * 

Peak:
    Old: 447.dealII      11440        285       40.2 *
    New: 447.dealII      11440        268       42.6 *  

Dynamic
Base: 
    Old: 447.dealII      11440        327       34.9 * 
    New: 447.dealII      11440        327       35.0 * 

Peak:
    Old: 447.dealII      11440        287       39.9 S
    New: 447.dealII      11440        288       39.7 *

So, no effect in dynamic case. Is it right?

[Bug libstdc++/52241] Performance degradation of 447.dealII on corei7 at spec2006_base32.

[paolo at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52241

--- Comment #21 from paolo at gcc dot gnu.org <paolo at gcc dot gnu.org> 2012-02-20 21:08:55 UTC ---
Author: paolo
Date: Mon Feb 20 21:08:48 2012
New Revision: 184404

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=184404
Log:
2012-02-20  Paolo Carlini  <paolo.carlini@oracle.com>

    PR libstdc++/52241
    * src/c++98/tree.cc (local_Rb_tree_increment,
    local_Rb_tree_decrement): Add.
    (_Rb_tree_increment(const _Rb_tree_node_base*),
    _Rb_tree_decrement(const _Rb_tree_node_base*)): Use the latter.
    (_Rb_tree_increment(_Rb_tree_node_base*),
    _Rb_tree_decrement(_Rb_tree_node_base*)): New.

Modified:
    trunk/libstdc++-v3/ChangeLog
    trunk/libstdc++-v3/src/c++98/tree.cc

[Bug libstdc++/52241] Performance degradation of 447.dealII on corei7 at spec2006_base32.

[paolo.carlini at oracle dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52241

Paolo Carlini <paolo.carlini at oracle dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|                            |FIXED

--- Comment #22 from Paolo Carlini <paolo.carlini at oracle dot com> 2012-02-20 21:10:50 UTC ---
Fixed for 4.7.0.

[Bug libstdc++/52241] Performance degradation of 447.dealII on corei7 at spec2006_base32.

[paolo.carlini at oracle dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52241

--- Comment #23 from Paolo Carlini <paolo.carlini at oracle dot com> 2012-02-20 21:16:41 UTC ---
PS: Vladimir, thanks a lot for all the good testing!