Closed GoogleCodeExporter closed 9 years ago
Interesting -- and I like how small the code is. However, I'm wary of
introducing a new flag for a pretty specialized feature; the general feeling
here is that pprof has too many flags already.
I wonder if there's another way to provide this functionality without a flag.
Is there somehow you could make it part of the --raw setting? Certainly, you
could augment raw input-reading to be able to read this kind of data too (in
addition to the /proc/self/maps data that's already in --raw output). You may
have to change the code you have that produces these external symbols, to
produce an entire raw output file, but that may not be that impractical.
Original comment by csilv...@gmail.com
on 21 Jun 2010 at 4:49
Is --raw option documented anywhere?
I see that --raw prints raw profile file (profile.raw) that has both in-memory
symbols (in text) and some binary part, presumably the converted .heap file.
Then pprof can be rerun with just this profile.raw, and the same graphical
output will be generated?
So in my case I should:
1. run 'pprof' first as if without LLVM symbols. Then add symbols from LLVM to
this profile.raw in the --symbols section.
2. run 'pprof' again with this profile.raw
When I did this profile looks very different, and now talks about samples
instead of memory now.
This looks very much like a hack to me. Am I misunderstanding something?
Also, LLVM can't be generating pprof-specific raw profile because it is besides
the point of llvm, it can't have separate option for each debugging/profiling
tool. Just like you are saying there are too many options to pprof. So I was
trying to find some common ground here. This file I am generating in llvm is
very universal and concise and contains just the relevant information is a very
simple format.
pprof already reads symbols from executables, since LLVM symbols can't be read
from executable this option I suggested is a very logical and minor addition
doing the job.
Original comment by y...@tsoft.com
on 21 Jun 2010 at 10:03
I don't think --raw is documented beyond pprof --help. But I guess that's true
of most pprof options.
Your general idea is right: create a profile.raw file, adds in your extra info,
and then read the raw profile by pprof.
I'm not sure what you're doing that turns your profile from a heap profile into
a cpu profile. What command did you run to generate the raw profile, and how
did you add back in the data? If you're willing to attach both a 'before' and
'after' raw profile file, I can take a look at them.
I would expect you'd have to modify pprof to understand your new type of --raw
file, too. But maybe not, depending on how you're modifying the raw file.
} Also, LLVM can't be generating pprof-specific raw profile
Right, I'm not suggesting that. You should generate a normal raw file by
running pprof, append your external_symbols file to it in some way, and then be
able to look at the results from running pprof on the raw file. This is a few
more steps than having llvm create the external-symbols file and then
specifying it as a flag to pprof, but doesn't seem that difficult, and would be
easy to automate in a small script.
} this option I suggested is a very logical and minor addition
Very true. The only problem is it adds a user-visible flag that 99% of pprof
users will never need. I'd like to avoid that if at all possible.
Original comment by csilv...@gmail.com
on 21 Jun 2010 at 10:21
> Very true. The only problem is it adds a user-visible flag that 99% of pprof
users will never need. I'd like to avoid that if at all possible.
You are suggesting to keep the option away from pprof. But this will cause the
creation of some shell script, essentially another tool. It has to be hosted
somewhere, maintained, documented. Whole thing has to be easy to use. And it
will have to be hosted within google-perftools as well since it directly
pertains to how perftools work. Number of lines in this tool will likely be
higher than in my patch.
On another note: every singe UNIX tool (ls, find, even cat) has options that
are never used consciously by 99.9% of people. This is the nature of tool
options.
Your suggestion keeps pprof simpler, but makes the whole tool chain more
complex for end users and developers.
Original comment by y...@tsoft.com
on 21 Jun 2010 at 10:37
} But this will cause the creation of some shell script, essentially another
tool.
Sounds like a challenge. :-) Can you figure out a way to do this that doesn't
add a lot of machinery, but also doesn't require adding flags anywhere?
Original comment by csilv...@gmail.com
on 22 Jun 2010 at 3:24
} But this will cause the creation of some shell script, essentially another
tool.
Sounds like a challenge. :-) Can you figure out a way to do this that doesn't
add a lot of machinery, but also doesn't require adding flags anywhere?
Original comment by csilv...@gmail.com
on 22 Jun 2010 at 3:24
I suggested one new option for one new feature: reading of the external symbol
files.
You are saying: don't 'add a lot of machinery', 'don't add a new flag'.
I am kind of lost here: new feature has to have new flag to use it unless it
expects some predefined file name. Am I missing something?
So what do you think solution for this might be?
Original comment by y...@tsoft.com
on 18 Dec 2010 at 3:50
Hello again!
My personal feeling, as I suggested earlier, is that this is best done by
incorporating this data into --raw mode somehow.
Original comment by csilv...@gmail.com
on 18 Dec 2010 at 4:45
I've been thinking about this some more, and I'm wondering if maybe it wouldn't
be better to have the functionality like you've written, and rewrite --raw in
terms of that (and maybe even deprecate --raw). Your technique, unlike --raw,
involves having two data files, but it's a lot easier to reason about, I think,
since the input is more similar to what pprof is used to getting.
I'll talk it over with some of the pprof authors to see what they think. But I
just wanted to let you know we haven't forgotten about this patch.
Original comment by csilv...@gmail.com
on 11 Feb 2011 at 9:38
Ping -- are you still interested in this?
I've been talking with the perftools authors -- the people who actually write
and hack on this code, which for the most part doesn't include me -- and
there's a push for keeping the number of new features to a minimum. Since this
is the only instance I've seen so far of someone wanting this functionality, if
you're no longer interested I'll let it drop.
Original comment by csilv...@gmail.com
on 14 Oct 2011 at 1:23
Original issue reported on code.google.com by
y...@tsoft.com
on 19 Jun 2010 at 8:12Attachments: