merge branch profile-stdlib

[Silvius Rus]

On Mon, Jun 15, 2009 at 1:32 PM, Silvius Rus <rus@google.com> wrote:
>
> On Fri, Jun 12, 2009 at 6:20 PM, Benjamin Kosnik <bkoz@redhat.com> wrote:
> >
> > Silvius, I wanted to give you some quick feedback on your branch this
> > week. I haven't been following this branch super closely but have
> > checked it out, and started to read your documentation and build/play
> > with it. However, I will be gone until mid-week and unable to follow-up
> > immediately.
> >
> > So, take these as preliminary notes. Sorry if they are rushed or
> > confused.
> >
> > There are some interesting ideas here. I would like some more time to
> > look at the way this is organized, and think about the way options
> > are presented to users. But in general, I think that there is merit in
> > your approach.
> >
> > > I would like to merge the profile-stdlib branch into trunk.  I made
> > > the changes requested by your previous reviews:
> > > - Gave up on adding a runtime library.  The diagnostic implementation
> > > was moved from profc++/ to include/profile/impl/.
> > > - Reverted driver changes so that the interface is -D_GLIBCXX_DEBUG,
> > > consistent with current extensions.
> >
> > I think you mean _GLIBCXX_PROFILE here.
> >
> > > Before I go with a fine comb, could you please take a look at the
> > > branch and let me know if there are any other major issues?
> >
> > In profile_mode.xml, need to revisit for -fprofile-stdlib status. Also,
> > can replace "stdlib" and "stdlibc++" with libstdc++.
> >
>
> The documentation has not been updated for a while.  I will do that ASAP.
>
> > > The big picture organization is:
> > > - All profile extension headers live in include/profile/.  They
> > > include profile/base.h.
> > > - All diagnostic implementations live in include/profile/impl/.  They
> > > are included by profile/impl/profiler.h.
> > > - profile/base.h includes profile/impl/profiler.h.  This is the only
> > > direct connection between include/profile/ and include/profile/impl/.
> > >
> >
> > Useful, thanks.
> >
> > > - The relation with debug and parallel extensions has not been
> > > defined.
> >
> > I'm currently of the mind that all these extensions should be mutually
> > exclusive. Thoughts?
> >
>
> I agree.  I will add a preprocessor check in config.h.
>
> > > - We are using vector and unordered_map in the implementation, with
> > > default allocators.  This can cause infinite cycles if say the
> > > application code uses libstdc++ containers to gather allocation
> > > statistics.
> >
> > As with Jonathan, this strikes me as something to be fixed sooner
> > rather than later. As you say, the design flaw is understood (custom
> > allocators for internal vector and unordered_map instances.)
> >
>
> OK.  I will fix this on the branch.
>
> > > - The machine-specific performance model component is not on the
> > > branch.  I decided to treat it as a separate component and add it
> > > later.  The decisions are based on generic operation performance
> > > ratios.
> >
> > Can we get some visibility here in terms of a specific instance of a
> > machine-specific model? This seems very hand-wavy. Where are you going
> > with this?
> >
> > Certainly, Cost Model: Math goes here
> >
> > is not sufficient.
> >
>
> It's not clear to me how to integrate the module that creates the cost
> model parameters for a given machine.  The cost model is a database
> with data points such as "operation=map<int,int>::insert
> initial_size=100 average_time=100ns".
>
> The cost model generator is a collection of C++ programs that exercise
> various library operation and record execution times into a database.
> My take at this point is to just define a precise format for this
> database and provide a set of default values in case the database is
> not provided.  It's not clear how to distribute the cost model
> generator.  Do you have any suggestions?
>
> Also, there's another component that's not clear how to distribute.
> We produce a trace that needs to be interpreted at some point.  We can
> do it when the program ends, but this would be useful as a standalone
> tool, so that we could process traces from several executions in order
> to produce smoother diagnostics.
>
> > > - Many diagnostics have not been implemented yet.
> >
> > Is there a list of possible directions for future work? I see a lot
> > already. I'm interested in where you are going with
> > _GLIBCXX_PROFILE_FALSE_SHARING.
> >
>
> We have some future work outlined in http://dx.doi.org/10.1109/CGO.2009.36.
> Yes, there's a lot more that can be done.  For programs that reference
> memory through libstdc++, this is a good handle at the right
> abstraction level, so there are many potential uses.
>
> > Anyway.
> >
> > Stepping back a bit, now that you have this, can you show how it's
> > applied to some kind of C++ source base? How would this be integrated
> > into a user's build system? How are you using it in the real world? Has
> > it been useful? What kind of expectations should we have about execution
> > speed w/ profile-stdlib active, given that you had concerns about debug
> > mode overhead?
> >
> > best,
> > benjamin
>
> I will do it on the SPEC apps that use the standard C++ library, using
> the SPEC harness, and get back with details and overhead measurements.
>
> Our overhead control mechanism is based on:
> - compile time switches (turn on/off each diagnostic or classes of
> diagnostics with, e.g, -D_GLIBCXX_PROFILE_VECTOR_TO_LIST=1)
> - run time switches (turn on/off each diagnostic or classes of
> diagnostics with, e.g, "set GLIBCXX_PROFILE_VECTOR_TO_LIST=1")
> - run time stack trace depth limit.
> There are two reasons why we need overhead control.  First, it is very
> important for continuous testing infrastructures, where (1) you don't
> care where exactly an error occurs, but whether an error was
> introduced and (2) you want robustness and minimal overhead, because
> you're running tests continuously.  Then, there are these applications
> that just behave differently if you introduce too much overhead
> non-uniformly.
>
> All this will keep me busy for a while.  I'll get back when all the
> issues have been addressed.
>
> Thank you!
> Silvius


Hello Benjamin,

I just committed revision 149613 to the profile-stdlib branch.  The
following changes were implemented since your first review:
- Make the user interface clear.  All preprocessor defines and
environment variable names are now in profiler.h.
- Add thread-safe and exception-safe reentrance guards for all the
hook implementation calls.
- Change the way warnings are issued to avoid external dependencies.
- Update documentation.
- Fix bugs.

The part that has not been implemented yet is the empirical cost model
database.  The current implementation is functional without it.  It is
only the accuracy of the advice that is affected by the lack of the
cost model database.  The current advice model uses some predefined
ratios of operation costs such as "iterating through a list is N times
slower than in a vector".

Also, below is a sample work flow and the actual warnings issued.

Could you please take another look?


Thank you,
Silvius


---

$ cat test.cc
#include <map>
#include <vector>
#include <unordered_map>

using std::map;
using std::vector;
using std::unordered_map;

struct dumb_hash {
  size_t operator()(int x) const {return 0;}
  size_t operator()(int x, int y) const {return x == y;}
};

int main() {
  map<int, int> m_to_umap;
  vector<int> v_to_list;
  unordered_map<int, int> um_too_small;
  unordered_map<int, int> um_too_large(1000000);
  unordered_map<int, int, dumb_hash, dumb_hash> um_dumb_hash;

  for (int i = 0; i < 10000; ++i) {
    m_to_umap[i] = i;
    v_to_list.insert(v_to_list.begin(), i);
    um_too_small[i] = i;
    um_too_small[i] = i;
    um_dumb_hash[i] = i;
  }
}

$ .../g++ -D_GLIBCXX_PROFILE -DHAVE_EXECINFO_H -std=c++0x -O0 test.cc

$ ./a.out

$ cat libstdcxx-profile.txt
vector-to-list: improvement = 7: call stack = 0x8048688 0x804b055
0x804df43 0x804831e 0xf7d51450 0x80481be : advice = change std::vector
to std::list
hash-distr: improvement = 7: call stack = 0x8048688 0x80498f5
0x804e2d0 0x804842f 0xf7d51450 0x80481be : advice = change hash
function
vector-size: improvement = 4: call stack = 0x8048688 0x804a921
0x804df02 0x804831e 0xf7d51450 0x80481be : advice = change initial
container size from 0 to 16384
hashtable-size: improvement = 4: call stack = 0x8048688 0x804a277
0x804e0e3 0x8048379 0xf7d51450 0x80481be : advice = change initial
container size from 11 to 15173
hashtable-size: improvement = 4: call stack = 0x8048688 0x804a277
0x804e28f 0x804842f 0xf7d51450 0x80481be : advice = change initial
container size from 11 to 15173
hashtable-size: improvement = 0: call stack = 0x8048688 0x804a277
0x804e0e3 0x80483d4 0xf7d51450 0x80481be : advice = change initial
container size from 1056323 to 0
hash-distr: improvement = 0: call stack = 0x8048688 0x80498f5
0x804e124 0x8048379 0xf7d51450 0x80481be : advice = change hash
function
hash-distr: improvement = 0: call stack = 0x8048688 0x80498f5
0x804e124 0x80483d4 0xf7d51450 0x80481be : advice = change hash
function
map-to-unordered-map: improvement = -4: call stack = 0x8048688
0x804a79b 0x804ddc6 0x80482f6 0xf7d51450 0x80481be : advice = change
std::map to std::unordered_map