Make '-noinlines' a separate flag, introduce '-filefunctions' granularity #420
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nolanmar511
previously approved these changes
Sep 20, 2018
Codecov Report
@@ Coverage Diff @@
## master #420 +/- ##
==========================================
+ Coverage 67.59% 67.61% +0.02%
==========================================
Files 36 36
Lines 7612 7611 -1
==========================================
+ Hits 5145 5146 +1
+ Misses 2062 2060 -2
Partials 405 405
Continue to review full report at Codecov.
|
hyangah
reviewed
Sep 20, 2018
doc/README.md
Outdated
| describe the same function will be merged into a report entry. | ||
| * **-addresses:** Accumulate samples at the instruction address; profile locations | ||
| that describe the same function address will be merged into a report entry. | ||
| * **-flat** [default], **-cum** Sort entries based on their flat or cumulative |
…rity. This change consists of two relatively independent parts, but they are both about handling the granularity, so bundling them together. First, in google#415 there is a discussion that having a granularity mode by source lines but with inlines hidden would be useful. The agreement is also that adding `-linenoinlines` granularity would make the granularity flags too messy (and they are already somewhat messy with `-addresses` and `-addressnoinlines`. So, it was proposed to make `-noinlines` a separate flag, which is what this change does. Note that the flag is now pulled out of the granularity group so it's a bit of backward incompatible change but I think it is acceptable. For the example in issue google#415 the user would now be able to specify `-list foo -noinlines` to produce annotated source where the metrics from the inlined functions are attributed to the calling inliner line. With this change, I am also dropping the `-addressnoinlines` granularity which is now supported as `-addresses -noinlines` combination. I couldn't find any usage of this option at least internally at Google, so I think it's safe to remove it. Second, I am adding a separate `-filefunctions` granularity which groups the data by both function and file. This is a follow-up to google#110 from the past where we changed the `-functions` granularity to not group by file (it used to), and since then there was a couple of reports where using just function name alone would over-aggregate the data in cases when a function with the same name is contained in multiple source files (e.g. see b/18874275 internally). Also, make a number of assorted documentation and `-help` fixes.
aalexand
dismissed stale reviews from nolanmar511 and rauls5382
via
1afa21f
|
@nolanmar511 @rauls5382 Could you re-approve the most recent changes please? |
nolanmar511
approved these changes
Sep 21, 2018
|
What version will this change in be @aalexand |
|
The default on the console is -noinlines, I need the opposite, I want the list command to use inlining. |
Do you mean which Go version? I am not 100% sure about that, Go team would know. @hyangah do you know if 1.12 is going to have this change in?
Is that with pprof from master branch of this repo? |
|
Inside pprof I need to be able to say
The default is to use It hurts to have to use |
|
I had this issue and it was closed? Do I need to create a new one from this? |
|
@ardan-bkennedy Do you use pprof from the master branch? The list command should have -inlines as the default, see pprof/internal/driver/driver.go Line 180 in 985cf9b |
|
Sorry, I didn't try the master branch. If this is the case, then we are good. Thanks for making that change. I can't wait for it to land. |
gmarin13
pushed a commit
to gmarin13/pprof
that referenced
this pull request
Dec 17, 2020
…rity. (google#420) This change consists of two relatively independent parts, but they are both about handling the granularity, so bundling them together. First, in google#415 there is a discussion that having a granularity mode by source lines but with inlines hidden would be useful. The agreement is also that adding `-linenoinlines` granularity would make the granularity flags too messy (and they are already somewhat messy with `-addresses` and `-addressnoinlines`. So, it was proposed to make `-noinlines` a separate flag, which is what this change does. Note that the flag is now pulled out of the granularity group so it's a bit of backward incompatible change but I think it is acceptable. For the example in issue google#415 the user would now be able to specify `-list foo -noinlines` to produce annotated source where the metrics from the inlined functions are attributed to the calling inliner line. With this change, I am also dropping the `-addressnoinlines` granularity which is now supported as `-addresses -noinlines` combination. I couldn't find any usage of this option at least internally at Google, so I think it's safe to remove it. Second, I am adding a separate `-filefunctions` granularity which groups the data by both function and file. This is a follow-up to google#110 from the past where we changed the `-functions` granularity to not group by file (it used to), and since then there was a couple of reports where using just function name alone would over-aggregate the data in cases when a function with the same name is contained in multiple source files (e.g. see b/18874275 internally). Also, make a number of assorted documentation and `-help` fixes.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This change consists of two relatively independent parts, but they are
both about handling the granularity, so bundling them together.
First, in #415 there is a discussion that having a granularity mode by
source lines but with inlines hidden would be useful. The agreement is
also that adding
-linenoinlinesgranularity would make the granularityflags too messy (and they are already somewhat messy with
-addressesand
-addressnoinlines. So, it was proposed to make-noinlinesaseparate flag, which is what this change does. Note that the flag is now
pulled out of the granularity group so it's a bit of backward
incompatible change but I think it is acceptable. For the example in
issue #415 the user would now be able to specify
-list foo -noinlinesto produce annotated source where the metrics from the inlined functions
are attributed to the calling inliner line.
With this change, I am also dropping the
-addressnoinlinesgranularitywhich is now supported as
-addresses -noinlinescombination. Icouldn't find any usage of this option at least internally at Google, so
I think it's safe to remove it.
Second, I am adding a separate
-filefunctionsgranularity which groupsthe data by both function and file. This is a follow-up to #110 from the
past where we changed the
-functionsgranularity to not group by file(it used to), and since then there was a couple of reports where using
just function name alone would over-aggregate the data in cases when a
function with the same name is contained in multiple source files (e.g.
see b/18874275 internally).
Also, make a number of assorted documentation and
-helpfixes.