add -diff flag for better profile comparision #369

nolanmar511 · 2018-05-01T15:35:59Z

Part of fixing #323.

Changes made to design:

Using key "base" and value "1" to mark profiles in the base profile, rather than key "pprof::base" (or "pprof::diff") and value "true", because we don't want to use long values (the are lots of samples, so adding lots of tags might increase memory usage too much).
Using -diff_base, rather than -diff as the flag name. It should be clearer.

aalexand · 2018-05-01T18:17:49Z

internal/driver/cli.go

@@ -140,7 +142,15 @@ func parseFlags(o *plugin.Options) (*source, []string, error) {
 		Comment:      *flagAddComment,
 	}

-	for _, s := range *flagBase {
+	if len(*flagBase) > 0 && len(*flagDiff) > 0 {


Compare strings for emptiness using x != "" so that it's clear it's not a slice.

Err, ignore, those are slices.

aalexand · 2018-05-01T18:18:29Z

internal/driver/cli.go

@@ -140,7 +142,15 @@ func parseFlags(o *plugin.Options) (*source, []string, error) {
 		Comment:      *flagAddComment,
 	}

-	for _, s := range *flagBase {
+	if len(*flagBase) > 0 && len(*flagDiff) > 0 {
+		return nil, nil, fmt.Errorf("only -base or -diff flag can be specified")


The message could be clearer - it now reads as if no other flags can be passed other than the two mentioned.

Adjusted the commented.

aalexand · 2018-05-01T18:21:29Z

internal/report/report.go

+
+		// Also remove pprof::diff=true string tag.
+		if values, ok := s.Label["pprof::diff"]; ok {
+			newVals := []string{}


var newVals []string

This code has been removed.

aalexand · 2018-05-01T18:27:28Z

internal/report/report.go

@@ -261,6 +261,21 @@ func (rpt *Report) newGraph(nodes graph.NodeSet) *graph.Graph {
 		}
 		s.NumLabel = numLabels
 		s.NumUnit = numUnits
+
+		// Also remove pprof::diff=true string tag.


It would be good to clarify why. Also this code is not super clear. Perhaps make a separate function, and look into how to simplify it. I would also look into whether code could be simpler if the pprof::diff tag was removed in the aggregate() code.

Would removing the tag in aggregation() prevent it from being used in later to separate out source and base profiles?

For now, I'm separating this into a different function.

Using the SetTag function to clear pprof::diff tag.
And adding comment to explain why this tag shouldn't be set.

aalexand · 2018-05-01T18:28:54Z

internal/report/report_test.go

@@ -285,3 +285,122 @@ func TestLegendActiveFilters(t *testing.T) {
 		}
 	}
 }
+
+func TestComputTotal(t *testing.T) {


aalexand · 2018-05-01T18:31:23Z

internal/report/report.go

@@ -1211,10 +1226,10 @@ func NewDefault(prof *profile.Profile, options Options) *Report {
 	return New(prof, o)
 }

-// computeTotal computes the sum of all sample values. This will be
-// used to compute percentages.
+// computeTotal computes the sum of the absolute value of all sample values.


Doc should be updated when such non-trivial logic is added in the function code.

aalexand · 2018-05-01T18:32:43Z

internal/report/report.go

+		total += v
+		if sample.HasTag("pprof::diff", "true") {
+			diffTotal += v
+		}
 		div += d


Assuming that it's intentional that accumulating the div is not under the tag condition, can you add a comment on why so?

aalexand · 2018-05-01T18:44:32Z

profile/profile.go

+// profile.
+func (p *Profile) AddTag(key, value string) {
+	for _, sample := range p.Sample {
+		if sample.HasTag(key, value) {


Calling HasTag could be avoided and handled below.

Changing AddTag to SetTag, so I think this no longer applies.

aalexand · 2018-05-01T18:45:06Z

profile/profile.go

@@ -674,6 +674,36 @@ func numLabelsToString(numLabels map[string][]int64, numUnits map[string][]strin
 	return strings.Join(ls, " ")
 }

+// AddTag adds a tag with the specified key and value to all samples in the
+// profile.
+func (p *Profile) AddTag(key, value string) {


I'd maybe do this as SetTag(), it would simplify the code and I don't think we really need to support multiple values per tag in this PR. Would simplify the tests as well.

aalexand · 2018-05-01T18:47:17Z

internal/report/report_test.go

+		{
+			Location: []*profile.Location{testL[2], testL[1], testL[0]},
+			Value:    []int64{-9000, 3},
+			Label:    map[string][]string{"pprof::diff": {"true"}},


Given that "pprof::diff" and "true" are going to be stored as strings for each sample, I wonder if we should come up with shorter strings to save memory.

I agree -- maybe "1"?

codecov-io · 2018-05-02T19:55:15Z

Codecov Report

Merging #369 into master will increase coverage by 0.18%.
The diff coverage is 88%.

@@            Coverage Diff             @@
##           master     #369      +/-   ##
==========================================
+ Coverage   66.89%   67.08%   +0.18%     
==========================================
  Files          36       36              
  Lines        7495     7532      +37     
==========================================
+ Hits         5014     5053      +39     
+ Misses       2079     2078       -1     
+ Partials      402      401       -1

Impacted Files	Coverage Δ
internal/driver/fetch.go	`68.34% <100%> (+0.18%)`	⬆️
profile/profile.go	`70.92% <100%> (+1.14%)`	⬆️
internal/driver/cli.go	`79.6% <78.26%> (+2.06%)`	⬆️
internal/report/report.go	`31.33% <90.9%> (+0.97%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4d67f66...9558b3c. Read the comment docs.

nolanmar511 · 2018-05-02T20:39:49Z

PTAL

aalexand · 2018-05-04T01:47:27Z

doc/pprof.md

@@ -214,6 +214,10 @@ search for them in a directory pointed to by the environment variable
 * **-weblist= _regex_:** Generates a source/assembly combined annotated listing for
  functions matching *regex*, and starts a web browser to display it.

+Samples in a profile may have tags. These tags have a name and a value; this


This added paragraph sounds a bit out of tune in this "Annotated code" section. Why here?

Made an error when moving docs around. Fixed.

aalexand · 2018-05-04T01:48:55Z

doc/pprof.md

+
+The **-base** option can be used in the place of the **-diff_base** option, and
+may be preferrable when comparing two cumulative profiles, for example two
+contention profiles collected from the same program at different times. When 


This should be more specific that we are talking about contention reports from Go programs. Linking to Go's documentation would be good.

I can't find much in go documentation on contention reports.

This is the main thing I found (from here):

// goroutine - stack traces of all current goroutines // heap - a sampling of all heap allocations // threadcreate - stack traces that led to the creation of new OS threads // block - stack traces that led to blocking on synchronization primitives // mutex - stack traces of holders of contended mutexes

I'm guessing that block profiles are what we would consider contention profiles?
I could look for blog posts?

Maybe link to https://golang.org/doc/diagnostics.html#profiling? It's not exclusively about contention but has something.

aalexand · 2018-05-04T01:49:55Z

doc/pprof.md

+may be preferrable when comparing two cumulative profiles, for example two
+contention profiles collected from the same program at different times. When 
+this flag is used and no negative values appear in the merged profile when
+aggregated at the address-level, percentages reported will be relative to the 


I'd somehow leave out the detail about address level or move it closer to the end of the section. Telling the whole story feels overwhelming a bit.

Tried to change this.
Possibly fixed.

aalexand · 2018-05-04T01:50:36Z

doc/pprof.md

+
+## Comparing profiles
+
+pprof can subtract a profile from another, provided the profiles are of 


Maybe start this section with a very high-level note that there are two ways to compare profiles, briefly characterize each of them, then talk about each in more detail.

aalexand · 2018-05-04T01:50:57Z

doc/pprof.md

+compatible types, in order to compare them. For that, use the 
+**-diff_base= _profile_** option, where *profile* is the filename or URL for the
+profile to be subtracted. This may result in some report entries having negative
+values.  Percentages in the report are relative to the total of 


Nit: double space is a typo?

aalexand · 2018-05-04T01:55:31Z

internal/driver/cli.go

@@ -140,12 +142,30 @@ func parseFlags(o *plugin.Options) (*source, []string, error) {
 		Comment:      *flagAddComment,
 	}

+	var base []string


This fragment of code of dealing with base and diffBase flags is a bit long now. Consider refactoring it.

Did some refactoring.

aalexand · 2018-05-04T01:58:41Z

internal/report/report.go

+		if sample.HasTag("pprof::base", "true") {
+			diffTotal += v
+			diffDiv += d
+		}
 		div += d


I think I had this comment but it was not answered before: Assuming that it's intentional that accumulating the div is not under the tag condition, can you add a comment on why so?

aalexand · 2018-05-04T02:00:04Z

profile/profile.go

+func (p *Profile) SetTag(key string, value []string) {
+	for _, sample := range p.Sample {
+		if sample.Label == nil {
+			sample.Label = make(map[string][]string)


sample.Label = map[string]string{} is a bit cheaper.

aalexand · 2018-05-04T02:02:17Z

profile/profile_test.go

+	}{
+		{
+			desc: "empty label does not have tag",
+			sample: &Sample{


I think you could avoid repeating the Sample and its Location and Value in every test case. You only care about Label field, you could reuse the rest.

aalexand · 2018-05-04T02:03:26Z

profile/profile_test.go

+		wantTags []map[string][]string
+	}{
+		{
+			desc:    "some samples have tag already",


Did you mean to add more test cases? E.g. what if sample doesn't have the tags.

Done.
Also did some refactoring of this test.

nolanmar511 · 2018-05-04T16:09:12Z

PTAL, except for at docs.
(Will add another PTAL when docs are ready to be looked at again)

nolanmar511 · 2018-05-04T19:33:58Z

Also addressed (tried to address) comments on docs.
Those are ready for PTAL, too.

nolanmar511 · 2018-05-09T16:54:20Z

PTAL

aalexand · 2018-05-09T17:06:54Z

internal/driver/fetch_test.go

 		},
 	}

+	// Allow more test cases to run in parallel.
+	runtime.GOMAXPROCS(len(testcases))


I do not understand why this is needed. You are not calling https://golang.org/pkg/testing/#T.Parallel here.

aalexand · 2018-05-09T19:26:38Z

@nolanmar511 Please LMK when you remove the runtime.GOMAXPROCS(len(testcases)) code and I'll look again.

nolanmar511 · 2018-05-09T19:34:58Z

Removed the runtime.GOMAXPROCS(len(testcases)) code.
PTAL

aalexand · 2018-05-09T20:23:19Z

internal/driver/fetch_test.go

 			base := make([]*string, len(tc.bases))
 			for i, s := range tc.bases {
 				base[i] = &s
 			}

+			diffBase := make([]*string, len(tc.diffBases))
+			for i, s := range tc.diffBases {


Nit: there is a similar loop above, maybe testFlags could be made having stringLists as slice of regular strings, not pointers and translate that to pointers when needed in its implementation. Very much optional suggestion - judge depending on how much noise in the test code this pointerization has.

aalexand · 2018-05-09T20:23:58Z

internal/driver/fetch_test.go

+				if err == nil {
+					t.Fatalf("got nil, want error %q", tc.wantErrorMsg)
+				}
+				gotErrMsg := err.Error()


Merge assignment into the if statement on next line?

aalexand · 2018-05-09T20:24:40Z

internal/driver/fetch_test.go

 			if err != nil {
 				t.Fatal(err)
 			}

-			if want, got := len(tc.expectedSamples), len(p.Sample); want != got {
-				t.Fatalf("want %d samples got %d", want, got)
+			if want, got := len(tc.wantSamples), len(p.Sample); want != got {


Super-nit: I'd reverse want, got into got, want on the assignment and condition as well.

aalexand · 2018-05-09T20:25:03Z

internal/report/report.go

@@ -264,6 +264,10 @@ func (rpt *Report) newGraph(nodes graph.NodeSet) *graph.Graph {
 		s.NumUnit = numUnits
 	}

+	// Remove tag marking samples from the base profiles, so it does not appear


tag -> label

Sorry -- just pushed.

aalexand · 2018-05-09T20:25:23Z

internal/report/report.go

-// computeTotal computes the sum of all sample values. This will be
-// used to compute percentages.
+// computeTotal computes the sum of the absolute value of all sample values.
+// If any samples have the tag "pprof::base" with value "true", then the total


tag -> label

nolanmar511 · 2018-05-11T22:45:55Z

PTAL

aalexand · 2018-05-22T22:11:44Z

internal/driver/cli.go

 	}

 	normalize := pprofVariables["normalize"].boolValue()
 	if normalize && len(source.Base) == 0 {
-		return nil, nil, fmt.Errorf("Must have base profile to normalize by")
+		return nil, nil, errors.New("Must have base profile to normalize by")


Must -> must, https://github.com/golang/go/wiki/CodeReviewComments#error-strings

aalexand · 2018-05-22T22:12:30Z

internal/driver/cli.go

+	return nil
+}
+
+// dropEmpty list takes a StringList flag, and outputs an array of non-empty


It does not take a flag, it takes a slice of string pointers. It does not return an array, it returns a slice.

aalexand · 2018-05-22T22:12:55Z

internal/driver/cli.go

+// profiles are specified.
+func (source *source) addBaseProfiles(flagBase, flagDiffBase []*string) error {
+	base, diffBase := dropEmpty(flagBase), dropEmpty(flagDiffBase)
+


Nit: remove empty line.

aalexand · 2018-05-22T23:56:38Z

internal/driver/driver_test.go

@@ -355,7 +355,11 @@ func (f testFlags) StringVar(p *string, s, d, c string) {

 func (f testFlags) StringList(s, d, c string) *[]*string {
 	if t, ok := f.stringLists[s]; ok {
-		return &t
+		tp := make([]*string, len(t))


Why this change? It's not obvious, a comment would be useful.

This was to address a previous comment for simplifying testing.
I changed the type of the stringLists field in the testFlags structure from map[string][]*string to map[string][]string to make it easier to construct flags for tests.

Added a comment.

aalexand · 2018-05-22T23:59:59Z

internal/report/report_test.go

+
+	for _, tc := range testcases {
+		t.Run(tc.desc, func(t *testing.T) {
+			gotTotal := computeTotal(tc.prof, tc.value, tc.meanDiv)


Merge with next line.

aalexand · 2018-05-23T00:01:49Z

profile/profile.go

+// profile.
+func (p *Profile) SetLabel(key string, value []string) {
+	for _, sample := range p.Sample {
+		if sample.Label == nil {


Could avoid lookup on the "no key" case as

if sample.Label == nil { sample.Label = map[string][]string{key: value} } else { sample.Label[key] = value }

aalexand · 2018-05-23T00:03:09Z

profile/profile.go

+// samples in the profile.
+func (p *Profile) RemoveLabel(key string) {
+	for _, sample := range p.Sample {
+		if _, ok := sample.Label[key]; ok {


https://blog.golang.org/go-maps-in-action: "The delete function doesn't return anything, and will do nothing if the specified key doesn't exist.", so I think you could drop the check?

aalexand · 2018-05-23T00:04:56Z

profile/profile.go

+
+// HasLabel returns true if a sample has a label with indicated key and value.
+func (s *Sample) HasLabel(key, value string) bool {
+	if values, ok := s.Label[key]; ok {


I think you could drop the "ok" check and just do

for _, v := range s.Label[key] {

as it will return nil slice if the key is missing which will immediately terminate the loop anyway.

aalexand · 2018-05-23T00:06:46Z

profile/profile_test.go

+				for wantKey, wantValues := range wantLabels {
+					if gotValues, ok := sample.Label[wantKey]; ok {
+						if !reflect.DeepEqual(gotValues, wantValues) {
+							t.Fatalf("for key %s, got values %v, want values %v", wantKey, gotValues, wantValues)


Why Fatalf?

Errorf works.
Done.

nolanmar511 · 2018-05-24T19:49:56Z

PTAL

aalexand

Just couple nits. Please also confirm you tested the new functionality manually too.

aalexand · 2018-05-29T16:53:36Z

internal/driver/fetch_test.go

 			true,
-			[][]int64{},
+			[]WantSample{},


Here and elsewhere: use nil for empty slices.

aalexand · 2018-05-29T16:55:24Z

internal/driver/fetch_test.go

 			if err != nil {
-				t.Fatalf("%s: %v", tc.desc, err)
+				t.Fatal(err)


Nit: this perhaps should be t.Fatalf("got error %q, want no error", err).

nolanmar511 · 2018-05-29T18:25:59Z

PTAL

nolanmar511 · 2018-05-29T19:19:10Z

Did some manual testing. Looking at the graph view, the value for "Total samples" is equal to the total of the base profile.

Used -proto to output the profile, then used pprof again to inspect that profile, and saw same total reported with graph view.

* 'master' of github.com:google/pprof: (32 commits) record diff base profile label key/value constant (google#384) apply additional command overrides based on report format (google#381) profile: fix legacy format Go heap profile parsing (google#382) add -diff flag for better profile comparision (google#369) Use -u flag in pprof installation command so that it updates if needed. (google#376) Add GetBase support for ASLR kernel mappings (google#371) Add show_from profile filter. (google#372) Update README.md due to 8dff45d (google#375) Remove stale docs, move useful ones. (google#374) internal/driver: skip tests requiring tcp on js (google#373) Add "trim path" option which can be used to relocate sources. (google#366) Move update_d3flamegraph.sh script from the root directory. (google#370) Skip unsymbolizable mapping during symbolz pass. (google#368) remove -positive_percentages flag (google#365) Render icicle graph in "Flame Graph" view (google#367) Add command-line editing support for interactive pprof (google#362) Improve profile filter unit tests, fix show filtering of mappings (google#354) Fake mappings should be merged into a single one during merging (google#357) Hack the code so that both existing Go versions and current Go master format it the same (google#358) moved filter tests into their own test file which matches the implementation file, filter.go. (google#356) ...

googlebot added the cla: yes label May 1, 2018

nolanmar511 force-pushed the diff branch 3 times, most recently from daf6352 to a7b32db Compare May 1, 2018 16:14

aalexand reviewed May 1, 2018

View reviewed changes

nolanmar511 force-pushed the diff branch 8 times, most recently from 542d764 to c15f92c Compare May 2, 2018 19:42

nolanmar511 force-pushed the diff branch from 6b43001 to 5b29f9e Compare May 4, 2018 00:32

aalexand reviewed May 4, 2018

View reviewed changes

nolanmar511 force-pushed the diff branch from 5b29f9e to 6dabcdd Compare May 4, 2018 16:08

nolanmar511 force-pushed the diff branch from aad9dda to 107e488 Compare May 4, 2018 17:49

address comments

79b3df8

nolanmar511 force-pushed the diff branch from d624f72 to d4336ce Compare May 9, 2018 16:52

aalexand reviewed May 9, 2018

View reviewed changes

use t.Run

ee3be39

nolanmar511 force-pushed the diff branch from d4336ce to ee3be39 Compare May 9, 2018 17:26

aalexand reviewed May 9, 2018

View reviewed changes

address comments

562c51d

nolanmar511 force-pushed the diff branch from 3177898 to 562c51d Compare May 11, 2018 22:40

aalexand reviewed May 23, 2018

View reviewed changes

nolanmar511 force-pushed the diff branch 2 times, most recently from c0dc0bd to e1f7505 Compare May 24, 2018 19:49

address comments

4b9834b

nolanmar511 force-pushed the diff branch from e1f7505 to 4b9834b Compare May 24, 2018 20:07

aalexand reviewed May 29, 2018

View reviewed changes

address comments

9558b3c

aalexand approved these changes May 29, 2018

View reviewed changes

aalexand merged commit c283c2c into google:master May 29, 2018

gmarin13 pushed a commit to gmarin13/pprof that referenced this pull request Dec 17, 2020

add -diff flag for better profile comparision (google#369)

448c208


		## Comparing profiles

		pprof can subtract a profile from another, provided the profiles are of

add -diff flag for better profile comparision #369

add -diff flag for better profile comparision #369

Conversation

nolanmar511 commented May 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aalexand May 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented May 2, 2018 • edited Loading

Codecov Report

nolanmar511 commented May 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nolanmar511 commented May 4, 2018

nolanmar511 commented May 4, 2018

nolanmar511 commented May 9, 2018

Choose a reason for hiding this comment

aalexand commented May 9, 2018

nolanmar511 commented May 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nolanmar511 commented May 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nolanmar511 commented May 1, 2018 •

edited

Loading

aalexand May 1, 2018 •

edited

Loading

codecov-io commented May 2, 2018 •

edited

Loading