Make '-noinlines' a separate flag, introduce '-filefunctions' granularity. (#420)

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 `-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 #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 #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.

doc/README.md

@@ -80,21 +80,21 @@ other.
 
 Some common pprof options are:
 
-* **-flat [default]:** Sort entries based on their flat weight, on text reports.
-* **-cum:** Sort entries based on cumulative weight, on text reports.
-* **-functions [default]:** Accumulate samples at the function level; profile
-  locations that describe the same function will be merged into a report entry.
-* **-lines:** Accumulate samples at the source line level; profile locations that
-  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
+  weight respectively, on text reports.
+* **-functions** [default], **-filefunctions**, **-files**, **-lines**,
+  **-addresses**: Generate the report using the specified granularity.
+* **-noinlines**: Attribute inlined functions to their first out-of-line caller.
+  For example, a command like `pprof -list foo -noinlines profile.pb.gz` can be
+  used to produce the annotated source listing attributing the metrics in the
+  inlined functions to the out-of-line calling line.
 * **-nodecount= _int_:** Maximum number of entries in the report. pprof will only print
   this many entries and will use heuristics to select which entries to trim.
 * **-focus= _regex_:** Only include samples that include a report entry matching
   *regex*.
 * **-ignore= _regex_:** Do not include samples that include a report entry matching
   *regex*.
-* **-show\_from= _regex_:** Do not show entries above the first one that 
+* **-show\_from= _regex_:** Do not show entries above the first one that
   matches *regex*.
 * **-show= _regex_:** Only show entries that match *regex*.
 * **-hide= _regex_:** Do not show entries that match *regex*.
@@ -209,12 +209,12 @@ default it will search for those tools in the current path, but it can also
 search for them in a directory pointed to by the environment variable
 `$PPROF_TOOLS`.
 
-* **-disasm= _regex_:** Generates an annotated source listing for functions matching
-  regex, with flat/cum weights for each source line.
-* **-list= _regex_:** Generates an annotated disassembly listing for functions
-  matching *regex*.
-* **-weblist= _regex_:** Generates a source/assembly combined annotated listing for
-  functions matching *regex*, and starts a web browser to display it.
+* **-list= _regex_:** Generates an annotated source listing for functions
+  matching *regex*, with flat/cum weights for each source line.
+* **-disasm= _regex_:** Generates an annotated disassembly listing for
+  functions matching *regex*.
+* **-weblist= _regex_:** Generates a source/assembly combined annotated listing
+  for functions matching *regex*, and starts a web browser to display it.
 
 ## Comparing profiles
 

internal/driver/commands.go

@@ -228,17 +228,17 @@ var pprofVariables = variables{
 	// Output granularity
 	"functions": &variable{boolKind, "t", "granularity", helpText(
 		"Aggregate at the function level.",
-		"Takes into account the filename/lineno where the function was defined.")},
+		"Ignores the filename where the function was defined.")},
+	"filefunctions": &variable{boolKind, "t", "granularity", helpText(
+		"Aggregate at the function level.",
+		"Takes into account the filename where the function was defined.")},
 	"files": &variable{boolKind, "f", "granularity", "Aggregate at the file level."},
 	"lines": &variable{boolKind, "f", "granularity", "Aggregate at the source code line level."},
 	"addresses": &variable{boolKind, "f", "granularity", helpText(
-		"Aggregate at the function level.",
+		"Aggregate at the address level.",
 		"Includes functions' addresses in the output.")},
-	"noinlines": &variable{boolKind, "f", "granularity", helpText(
-		"Aggregate at the function level.",
-		"Attributes inlined functions to their first out-of-line caller.")},
-	"addressnoinlines": &variable{boolKind, "f", "granularity", helpText(
-		"Aggregate at the function level, including functions' addresses in the output.",
+	"noinlines": &variable{boolKind, "f", "", helpText(
+		"Ignore inlines.",
 		"Attributes inlined functions to their first out-of-line caller.")},
 }
 

internal/driver/driver.go

@@ -152,20 +152,28 @@ func generateReport(p *profile.Profile, cmd []string, vars variables, o *plugin.
 }
 
 func applyCommandOverrides(cmd string, outputFormat int, v variables) variables {
+	// Some report types override the trim flag to false below. This is to make
+	// sure the default heuristics of excluding insignificant nodes and edges
+	// from the call graph do not apply. One example where it is important is
+	// annotated source or disassembly listing. Those reports run on a specific
+	// function (or functions), but the trimming is applied before the function
+	// data is selected. So, with trimming enabled, the report could end up
+	// showing no data if the specified function is "uninteresting" as far as the
+	// trimming is concerned.
 	trim := v["trim"].boolValue()
 
 	switch cmd {
-	case "callgrind", "kcachegrind":
-		trim = false
-		v.set("addresses", "t")
 	case "disasm", "weblist":
 		trim = false
-		v.set("addressnoinlines", "t")
+		v.set("addresses", "t")
+		v.set("noinlines", "t")
 	case "peek":
 		trim = false
 	case "list":
-		v.set("nodecount", "0")
+		trim = false
 		v.set("lines", "t")
+		// Do not force 'noinlines' to be false so that specifying
+		// "-list foo -noinlines" is supported and works as expected.
 	case "text", "top", "topproto":
 		if v["nodecount"].intValue() == -1 {
 			v.set("nodecount", "0")
@@ -176,9 +184,11 @@ func applyCommandOverrides(cmd string, outputFormat int, v variables) variables
 		}
 	}
 
-	if outputFormat == report.Proto || outputFormat == report.Raw {
+	switch outputFormat {
+	case report.Proto, report.Raw, report.Callgrind:
 		trim = false
 		v.set("addresses", "t")
+		v.set("noinlines", "f")
 	}
 
 	if !trim {
@@ -190,28 +200,28 @@ func applyCommandOverrides(cmd string, outputFormat int, v variables) variables
 }
 
 func aggregate(prof *profile.Profile, v variables) error {
-	var inlines, function, filename, linenumber, address bool
+	var function, filename, linenumber, address bool
+	inlines := !v["noinlines"].boolValue()
 	switch {
 	case v["addresses"].boolValue():
-		return nil
+		if inlines {
+			return nil
+		}
+		function = true
+		filename = true
+		linenumber = true
+		address = true
 	case v["lines"].boolValue():
-		inlines = true
 		function = true
 		filename = true
 		linenumber = true
 	case v["files"].boolValue():
-		inlines = true
 		filename = true
 	case v["functions"].boolValue():
-		inlines = true
 		function = true
-	case v["noinlines"].boolValue():
-		function = true
-	case v["addressnoinlines"].boolValue():
+	case v["filefunctions"].boolValue():
 		function = true
 		filename = true
-		linenumber = true
-		address = true
 	default:
 		return fmt.Errorf("unexpected granularity")
 	}

internal/driver/driver_test.go

@@ -53,6 +53,9 @@ func TestParse(t *testing.T) {
 		flags, source string
 	}{
 		{"text,functions,flat", "cpu"},
+		{"text,functions,noinlines,flat", "cpu"},
+		{"text,filefunctions,noinlines,flat", "cpu"},
+		{"text,addresses,noinlines,flat", "cpu"},
 		{"tree,addresses,flat,nodecount=4", "cpusmall"},
 		{"text,functions,flat,nodecount=5,call_tree", "unknown"},
 		{"text,alloc_objects,flat", "heap_alloc"},
@@ -248,7 +251,8 @@ func testSourceURL(port int) string {
 func solutionFilename(source string, f *testFlags) string {
 	name := []string{"pprof", strings.TrimPrefix(source, testSourceURL(8000))}
 	name = addString(name, f, []string{"flat", "cum"})
-	name = addString(name, f, []string{"functions", "files", "lines", "addresses"})
+	name = addString(name, f, []string{"functions", "filefunctions", "files", "lines", "addresses"})
+	name = addString(name, f, []string{"noinlines"})
 	name = addString(name, f, []string{"inuse_space", "inuse_objects", "alloc_space", "alloc_objects"})
 	name = addString(name, f, []string{"relative_percentages"})
 	name = addString(name, f, []string{"seconds"})

internal/driver/interactive_test.go

@@ -259,12 +259,13 @@ func TestInteractiveCommands(t *testing.T) {
 		{
 			"weblist  find -test",
 			map[string]string{
-				"functions":        "false",
-				"addressnoinlines": "true",
-				"nodecount":        "0",
-				"cum":              "false",
-				"flat":             "true",
-				"ignore":           "test",
+				"functions": "false",
+				"addresses": "true",
+				"noinlines": "true",
+				"nodecount": "0",
+				"cum":       "false",
+				"flat":      "true",
+				"ignore":    "test",
 			},
 		},
 		{

internal/driver/testdata/pprof.cpu.flat.addresses.noinlines.text

@@ -0,0 +1,7 @@
+Showing nodes accounting for 1.12s, 100% of 1.12s total
+Dropped 1 node (cum <= 0.06s)
+      flat  flat%   sum%        cum   cum%
+     1.10s 98.21% 98.21%      1.10s 98.21%  0000000000001000 line1000 testdata/file1000.src:1
+     0.01s  0.89% 99.11%      1.01s 90.18%  0000000000002000 line2000 testdata/file2000.src:4
+     0.01s  0.89%   100%      1.01s 90.18%  0000000000003000 line3000 testdata/file3000.src:6
+         0     0%   100%      0.10s  8.93%  0000000000003001 line3000 testdata/file3000.src:9

internal/driver/testdata/pprof.cpu.flat.filefunctions.noinlines.text

@@ -0,0 +1,5 @@
+Showing nodes accounting for 1.12s, 100% of 1.12s total
+      flat  flat%   sum%        cum   cum%
+     1.10s 98.21% 98.21%      1.10s 98.21%  line1000 testdata/file1000.src
+     0.01s  0.89% 99.11%      1.01s 90.18%  line2000 testdata/file2000.src
+     0.01s  0.89%   100%      1.12s   100%  line3000 testdata/file3000.src

internal/driver/testdata/pprof.cpu.flat.functions.noinlines.text

@@ -0,0 +1,5 @@
+Showing nodes accounting for 1.12s, 100% of 1.12s total
+      flat  flat%   sum%        cum   cum%
+     1.10s 98.21% 98.21%      1.10s 98.21%  line1000
+     0.01s  0.89% 99.11%      1.01s 90.18%  line2000
+     0.01s  0.89%   100%      1.12s   100%  line3000