Sorry for disturbing you again.
Previously attached profiledata.cc.patch has some flaws.
Updated patch attached.

Thanks for the patch!  I don't know anything about PAPI, so I'm wondering: what information
does this give us that we don't get already?  Is it a replacement for existing cpu-usage
information, or an addition?  Are you collecting all the useful information that PAPI
provides for cpu profiling, or is there more it might make sense to add later?

I like that the patch is limited to only a few files, but it seems pretty invasive
within those files.  In general, I like to minimize the number of #ifdefs in code.
 I wonder if it's possible to keep the papi-specific stuff in a new file, base/papi.{h,cc},
and expose the functionality there.  The idea would be to define a struct to put into
the cpu-profiler struct, and a function to fill it and a function to update it.  If
PAPI exists and is configured on, then these structs and functions do something useful,
otherwise, the struct is empty and the functions are noops.  Then all the PAPI-related
#ifdefs just have to be in the papi.* files.

In addition to minimizing ifdefs, it's nice to minimize configure options.  Is there
any reason not to turn on PAPI if it exists on a system?  Is it very expensive to compute?
 I guess one issue is that PAPI needs a driver, at least on linux (as I understand
it), so we can't do that testing at build time, we have to do it at program-run time,
since the program may run on a different machine than it's built on.  So some of the
papi tests would have to be done at runtime.  But maybe that's ok; we already do something
similar to see if the current machine supports thread-local storage.

1.  PAPI is an API for counting low-level CPU events, like total instruction executed,
L1/L2 cache misses, incorrectly predicted conditional branches. It uses hardware counters
on modern CPU, so it performs differently on different CPUs. It's helpful when we need
to point out why some functions are too slow: it is too long(has too many instructions)?
It doesn't use CPU cache effectively(too many cache misses)? Or it has too many conditional
branches(which will slow CPU speed as well)?
    So, it's only an ADDITION, not replacement.

2.  PAPI can monitor dozens of CPU events, but due to hardware resource limits(number
of hardware counters that can be used by PAPI), only a few events(on my core 2 duo
P8600, at most 4) can be monitored at the same time.
    So I leave the choice of events to users. Users can add all events they want to
monitor to a file and set path to the file as environment variable PAPI_EVENT_SET.
When the program is running, it will scan the file, and then try to add all events.
But how many events will be added successfully depends, and successfully added events
will be printed to stderr.
    In this aspect, I'm not collecting all useful information that PAPI provides, but
I am able to. And you can get different information you need by only editing the PAPI_EVENT_SET
file, without rebuilding your program or google-perftools.

3.  Minimizing #ifdefs and moving all PAPI related stuff to independent {.h .cc} files
is graceful. I totally agree. I can do this if you want this PAPI feature.

4.  PAPI function calls are very fast. I can't think of a reason not to turn on PAPI
if it exists.
    PAPI does need kernel support. Default Ubuntu kernel doesn't support it, but default
Fedora kernel does. If you want to use PAPI on Ubuntu, you have to build a custom kernel,
and then build PAPI library. But on Fedora, you only need to "yum install papi-devel".
    Even though, I think we can still decide it during ./configure by looking for headers
and libs of PAPI. The papi tests can be done at runtime, when initialing CPU Profiler.

Thanks, this is all very useful.  I know at google we do some cpu-level profiling of
this sort; I wonder if we use PAPI for that, and if so, how it fits in with the cpuprofiler
(which, no surprise, we use a lot internally).  So let me do a bit more research when
I get back from vacation next week.  If we do end up using this patch, it would be
great to refactor it, but it may be best to figure out exactly what the plan is, before
putting more work into it.

Speaking of which, can you sign our CLA?
   http://code.google.com/legal/individual-cla-v1.0.html
We'll need that to accept the patch.

Sure, enjoy your vacation :-)
I also thought google do this kind of CPU profiling well enough, I worked out this
patch mainly because a course project needs it.
I'll wait for your plan.

Craig: this might get the cpuprofiler working on the perfctr patched kernels. (http://code.google.com/p/google-perftools/issues/detail?id=227).
I'll try out the patches and update this thread. 

Thanks endlessroad1991. 

ashwinks:
I'm afraid I have to disappoint you, because this patch only add some information to
profile data, it won't get CPU Profiler working if it didn't work properly.
But it has something to do with perfctr. I mentioned above that PAPI does need kernel
support, that means it needs perfctr patch applied to kernel. Fedora default kernel
has the patch, Ubuntu doesn't. I'm using Fedora 14, it has perfctr patch, and CPU Profiler
works normally no matter if PAPI feature is turned on.
Anyway, good luck :-)

Which is the kernel version in Fedora 14? I am using a RHEL 4 system (Linux 2.6.9-78.ELsmp
with perfctr patch. RHEL 4.0.7 64-bit) with a  patched kernel. The problem I am facing
is that the timer may not be getting attached to the process. 

kernel.i686                        2.6.35.10-74.fc14         @updates
papi.i686                          4.1.0-1.fc14              @fedora            
papi-devel.i686                    4.1.0-1.fc14              @fedora
gcc.i686                           4.5.1-4.fc14              @fedora            

It turns out that within google, we access the PMU data directly, rather than going
through PAPI.  So we don't have any code we can share with this.  A good next step
would be to extract stuff out into a separate file, like I described before, and submit
the patch again.  Sound good?

Thanks. But now I'm busy preparing for final exams :-(
I will refactor the patch as you said within one week.

See README for everything.

By the way, I'm using Ubuntu 10.04 for your convenience.
Goobuntu is based on Ubuntu LTS, isn't it?

In previous comments I said default Ubuntu kernel doesn't support PAPI.
I was wrong. Kernel 2.6.32 is enough for PAPI, no need for perfctr patches.

So maybe you can test PAPI feature on your machine as in README.

This is great!  I'm a bit overwhelmed at the moment catching up from the holidays (and
starting all the stuff that I said, "It's too close to the holidays to start this up
now, I'll wait until 2011"...), but I'll take a close look at this when I get a chance.

I took a cursory look now.  Overall, I like how it's coming together.  A few comments:

1) I'm not sure what to make of the TODOs in pprof, with the size of the data possibly
changing.  Also, I'm not sure if using Math::BigInt is consistent with the rest of
pprof (which I only kinda barely understand, not being a perl expert myself).  Shouldn't
we make sure that all numeric values in the profile are the same size -- so on a 64-bit
system the papi data will be 64 bits, and on a 32-bit system the papi data will be
32 bits?  I already have complicated code to check bit-size and endianness of the profile
file.  Is there some way to leverage that?

We do see people create profiles on a 32-bit system and then try to analyze them on
a 64-bit system (and vice versa), so it would be nice if papi worked in that case.

2) I noticed that even in the 'no papi support' case, papi.cc creates an empty file
every time.  I imagine almost all our users will not be papi-enabled, and will have
to pay the cost of this extra zero-length file every time they run the cpu profile.
 It would be nice to avoid that cost if possible.  Is there any way to get pprof to
behave nicely without the need to create that empty file?  Or can this file be combined
with the existing profile file in some way?  I haven't looked into this closely enough
to know if that's way off base or not.

Thanks for doing this!  I think it will be very nice to get the improved profiler info.

Updated patch.

1) Remove TODOs in pprof
    It turns out that PAPI data is long long,
        so they're both 64bits on both 32-bit and 64-bit machines.
    Thus, I remove these TODOS in the updated patch.
    Also, 32/64-bit and little/big-endian are all judged from the profile file,
        not the machine running pprof, so we can't use $Config->{intsize} or etc.
    You may want to check if my patch to pprof is correct.
        I've only checked it on my laptop (32-bit and little endian).


2) papi.cc doesn't create an empty file any more
    It's useful only when PAPI is not supported,
        but user still specify PAPI_EVENT_SET for his program and pprof.
        In this situation, pprof results will be messed up.
        However, if you don't specify PAPI_EVENT_SET, there will be no problem.
    So creating the empty file is not necessary.
    But never specify PAPI_EVENT_SET for pprof, when PAPI is not supported.

Trying to port gperftools to Windows.
Any suggestions?

Do you mean porting perftools in general, or just the papi part?  If in general, then
a lot of perftools already runs on windows.  What part are you looking to port?   A
better place to talk about it than here, is probably google-perftools@googlegroups.com.

I finally had a few moments to look at this today.  I'm sorry it's taken me a couple
of weeks to get back to you.

I'm concentrating on pprof for the moment.  I don't like the fact "If PAPI feature
is enabled, you should set PAPI_EVNET_SET to the correct file used when profiling your
program, or all pprof results would be wrong and messy."  I think the event set should
be baked into the profile when it's created.

This should be easy to do, given the CPU profiler header:
---
  # Read header.  The current header version is a 5-element structure
  # containing:
  #   0: header count (always 0)
  #   1: header "words" (after this one: 3)
  #   2: format version (0)
  #   3: sampling period (usec)
  #   4: unused padding (always 0)
---
With PAPI, just add a new field, between 3 and 4, which is the size of PAPI_EVENT_SET,
followed by the names of each event (NUL-terminated, or \n-terminated, or whatever).
 Since 'size' here is in blocks (4 or 8 bytes, depending), you'll need to do a bit
of work to calculate the size (what is put in (1)).  Keep the unused padding at the
end of the header.

(Note I'm suggesting this though I'm not the primary pprof author, so this change will
still have to go through him.  I think it's more likely to get through than the current
version, though.)

I also am not fond of the fact that the internal representation of the profile depends
on the profile type: there are too many 'if cpu' tests in there.  I'd either make them
all the same -- that is, all be a hash with the key of 'n' for the value -- or put
the papi info in a separate data structure.  I don't know how feasible the latter is.

Finally, I don't know perl very well, so I don't know much about this BigInt package.
 Is it standard -- do all perls have it?  What version of perl was it added in?  It
seems like bigint might be overkill: is there another method we can use to represent
64-bit numbers in 32-bit perl?  (PAPI values are always 64 bits, even on 32-bit processors,
is that true?)

Whatever solution is used here, should be used for the existing code that tries to
read 64-bit values, and just dies if it does so with a 32-bit perl.  That could be
a separate patch, though.  You could add a TODO about that in the current patch (see
elsewhere in the file for TODO format).

A more minor note: in configure.ac, you don't need to have ENABLE_PAPI_IN_CPU_PROFILER,
I'm pretty sure.  Just do the AC_SUBST(PAPI_LIBS) always (it will be "" when papi isn't
found), and just add $(PAPI_LIBS) unconditionally to the link line when appropriate.

1. Bake PAPI event names into profile file
    I did this way because I wanted to keep pprof backward-compatible. Current pprof
can still handle previous profile files.
    In my opinion, CpuProfileStream uses a cache mechanism and from outside the class
we can only call get() to get a slot. So operations like reading a line or reading
a string become inconvenient(still can be done, anyway).
    An alternate solution is to save papi event id to profile header(getting intergers
from profile file is easy). Then we have to translate event code to event name. This
translation work is really simple(PAPI function: PAPI_event_code_to_name() does it),
but it needs a separate C program so we can do the translation work in pprof.
    Which do you prefer?

2. Too many 'if cpu' tests
    I think making them all the same(key 'n' for the value) is better. PAPI info is
just integers, and has no need for further extending, so why bother to put it in a
separate data structure?

3. BigInt
    Math::BigInt is one of perl's core modules and is old enough.
    I didn't find detailed history of bigint in perl. According to old documents of
Perl 5.8(http://perldoc.perl.org/5.8.8/Math/BigInt.html), Perl 5.8 has Math::BigInt.
And http://perl.active-venture.com/lib/Math/BigInt.html implies Perl 5.6 already had
Math::BigInt.
    It is overkill, but I googled a lot and didn't find a third way except BigInt and
pack.
    It's supported on both 32-bit and 64-bit Perl, and can solve the TODO you left
in pprof.
    pprof does only a little big int related calculations, therefore efficiency isn't
a problem.
    So what do you think?

4. Remove ENABLE_PAPI_IN_CPU_PROFILER from configura.ac and makefile.am
    Thanks for the note, a lot cleaner :-)

} I did this way because I wanted to keep pprof backward-compatible. Current pprof can
still handle previous profile files.

My suggestion is both forward- and backward-compatible (note the part where you modify
the header-size field in the cpu profile).  pprof was designed to allow for extensible
header information, thankfully.

You are right that storing strings in the cpu profile isn't really a natural fit for
the block-focused reader (and the fact the header size is indicated in terms of blocks
as well, so you may have to do some padding).  But it will certainly end up being much
cleaner than having to specify the same values twice, through an environment variable.

} PAPI info is just integers, and has no need for further extending, so why
} bother to put it in a separate data structure?

Well, the data structures are relatively happy now, being a map from a key to an integer
value.  You're proposing changing it to a map from a key to another map, one of whose
elements is an integer value (with a rather arbitrary name).  That adds comprehension
complexity, and makes me somewhat sad.  If you can support a separate PAPI map that
doesn't add as much complexity (in order to pass it around), then that will be better.

} Math::BigInt is one of perl's core modules and is old enough.

OK, sounds good.  I'm happy with that part.

Sample data was designed very simple, only an integer. Many places in pprof uses this
convenience directly, like plus/minus/get/set. If papi data is inserted, I think it's
better to extract sample data to a separate class, and replace all direct accesses
with method of the new sample data class.

Also, inserting event names to profile header makes profile header difficult to extend
in the future. Event names can be inserted before/after map, which is also pure text.

Event names are inserted before /proc/map. The only addition to profile header is one
slot: PAPI event count. 



Now ProfileData is an independent class. Origin count is accessed by $ProfileData->{n}.
PAPI data is accessed by $ProfileData->{papi}.

$ProfileData->badd() does addition for both {n} and {papi}, ->bsub() does subtraction.

GetEntry() is renamed to GetEntryCount(), because it does return a count, but now an
Entry is a $ProfileData.



Also, when read 64-bit profile on 32-bit perl(no pack available), use Math::BigInt.
In fact, Math::BigInt is easy to use. Only difference is when you create the variable,
use
    my $a = Math::BigInt->new('123');
Other operations like add or print is exactly like before:
    $a += 2;
    print STDERR $a;



I've tested the code on 32-bit and 64-bit Ubuntu: read 32-bit profile on 64-bit Ubuntu,
and read 64-bit profile on 32-bit Ubuntu. All work well.

ProfileData has changed, so I tested CPU profile and heap profile. Also works well.

Thanks for the new patch!  The design you describe sounds good to me.  It can be tough
for patches to make it past the pprof gatekeepers, but I'm pretty hopeful about this
one.

We're trying to get a kernel set up locally that supports papi, so we can try the patch
out for a little while before committing it to the SVN tree.  This means this patch
may not make it into the next release (which I'm hoping to do in the next week or so),
but we're making steady progress on it.

Refer to Linux section of file INSTALL in PAPI source directory.  In general, kernel
2.6.32 or newer doesn't need any patch to support PAPI.  But for kernel prior to 2.6.32,
perfctr patches are needed.  My laptop is Core 2 Duo P8600, and Ubuntu 10.04 default
kernel supports PAPI "out of the box".

Thanks for being so patient.

I should be thanking *you* for being patient!  We'll get to this patch eventually...

Hi.  Craig asked me to take a look at this patch (months ago, so the
blame's on me, not him :)

First of all, I don't understand the use-case here.  Adding event
counts to aggregated time-based stack samples doesn't make much sense
in my mind.  Please elaborate.

Suppose we are counting cache misses and we have two functions:
MissesCache and HitsCache.  And each of those functions consume ~50% of
the CPU profile.  If these two functions are called alternatingly,
then HitsCache would also be blamed for cache misses.  Attached is a
microbenchmark (compiled with g++ -O1 -fno-omit-frame-pointer)
illustrating this (and verified using perf_events).  Also attached is
the pprof gv output from collecting PAPI_L1_DCM.

Documentation:
- The README file doesn't mention how to build or use the PAPI
  features.

configure:
- The enable-papi-in-cpu-profiler option doesn't seem to be optional.
  If PAPI is found, it's enabled by default.
- I'm autoconf/automake illiterate, but shouldn't this patch include
  the updated configure script as well?

make:
- I got this failure (but I could work around it to do my own testing):
  make: *** No rule to make target `-lpapi', needed by
  `profiler2_unittest'.  Stop.

pprof:
- I'm completely Perl-illiterate, so I'm going to punt on reviewing pprof.
- I did *use* pprof, and I get lots of "nan%" when using it on
  profiles collected with or without PAPI_EVENT_SET (or even
  with papi-in-cpu-profiler disabled)

Style:
- Don't worry about fixing style comments until the usefulness and
  functionality comments are addressed.
- Many of your comments are redundant.  Only comment when it explains
  something not obvious.  E.g.,
  // Dump PAPI event names
  DumpPAPIEventNames(out_);
- Run cpplint.
  http://google-styleguide.googlecode.com/svn/trunk/cpplint/cpplint.py
- Per the style guide, curly braces are optional.  But the code you're
  modifying seems to always use them.  Please do the same for
  consistency.

Tipp, thanks for taking the time to look at this!

After talking things over with folks inside google, we've decided that there are a lot
of different ways to profile, and we want to focus on just one rather than complexifying
the profiler and pprof.  So I think it's great to have this patch around for anyone
who is interested, and I encourage you to keep a fork of the profiler around with this
stuff in it if you want, but I'm going to close this WillNotUse.