Repository: google/pprof
Pull request #559 (2020-09-25 03:30:56)

Modify Scale() and ScaleN() to round and drop all-zero samples when scaling

codecov-commenter: # [Codecov](https://codecov.io/gh/google/pprof/pull/559?src=pr&el=h1) Report
> Merging [#559](https://codecov.io/gh/google/pprof/pull/559?src=pr&el=desc) into [master](https://codecov.io/gh/google/pprof/commit/acf8798be1f76c82483eaa0daf3fc93ab3d493b6?el=desc) will **decrease** coverage by `1.28%`.
> The diff coverage is `100.00%`.

[![Impacted file tree graph](https://codecov.io/gh/google/pprof/pull/559/graphs/tree.svg?width=650&height=150&src=pr&token=nHgs4Smibv)](https://codecov.io/gh/google/pprof/pull/559?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master     #559      +/-   ##
==========================================
- Coverage   68.46%   67.18%   -1.29%     
==========================================
  Files          78       39      -39     
  Lines       16108     7039    -9069     
==========================================
- Hits        11028     4729    -6299     
+ Misses       4225     1898    -2327     
+ Partials      855      412     -443     
```


| [Impacted Files](https://codecov.io/gh/google/pprof/pull/559?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [profile/profile.go](https://codecov.io/gh/google/pprof/pull/559/diff?src=pr&el=tree#diff-cHJvZmlsZS9wcm9maWxlLmdv) | `69.67% <100.00%> (-1.56%)` | :arrow_down: |
| [internal/driver/settings.go](https://codecov.io/gh/google/pprof/pull/559/diff?src=pr&el=tree#diff-aW50ZXJuYWwvZHJpdmVyL3NldHRpbmdzLmdv) | `67.21% <0.00%> (-5.01%)` | :arrow_down: |
| [internal/binutils/addr2liner\_llvm.go](https://codecov.io/gh/google/pprof/pull/559/diff?src=pr&el=tree#diff-aW50ZXJuYWwvYmludXRpbHMvYWRkcjJsaW5lcl9sbHZtLmdv) | `64.51% <0.00%> (-4.06%)` | :arrow_down: |
| [profile/proto.go](https://codecov.io/gh/google/pprof/pull/559/diff?src=pr&el=tree#diff-cHJvZmlsZS9wcm90by5nbw==) | `79.53% <0.00%> (-3.73%)` | :arrow_down: |
| [internal/driver/flags.go](https://codecov.io/gh/google/pprof/pull/559/diff?src=pr&el=tree#diff-aW50ZXJuYWwvZHJpdmVyL2ZsYWdzLmdv) | `15.38% <0.00%> (-3.67%)` | :arrow_down: |
| [internal/driver/fetch.go](https://codecov.io/gh/google/pprof/pull/559/diff?src=pr&el=tree#diff-aW50ZXJuYWwvZHJpdmVyL2ZldGNoLmdv) | `62.88% <0.00%> (-3.17%)` | :arrow_down: |
| [internal/symbolz/symbolz.go](https://codecov.io/gh/google/pprof/pull/559/diff?src=pr&el=tree#diff-aW50ZXJuYWwvc3ltYm9sei9zeW1ib2x6Lmdv) | `76.92% <0.00%> (-3.08%)` | :arrow_down: |
| [internal/graph/graph.go](https://codecov.io/gh/google/pprof/pull/559/diff?src=pr&el=tree#diff-aW50ZXJuYWwvZ3JhcGgvZ3JhcGguZ28=) | `27.87% <0.00%> (-2.84%)` | :arrow_down: |
| [internal/driver/commands.go](https://codecov.io/gh/google/pprof/pull/559/diff?src=pr&el=tree#diff-aW50ZXJuYWwvZHJpdmVyL2NvbW1hbmRzLmdv) | `27.50% <0.00%> (-2.72%)` | :arrow_down: |
| [internal/driver/webui.go](https://codecov.io/gh/google/pprof/pull/559/diff?src=pr&el=tree#diff-aW50ZXJuYWwvZHJpdmVyL3dlYnVpLmdv) | `53.50% <0.00%> (-2.70%)` | :arrow_down: |
| ... and [69 more](https://codecov.io/gh/google/pprof/pull/559/diff?src=pr&el=tree-more) | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/google/pprof/pull/559?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/google/pprof/pull/559?src=pr&el=footer). Last update [acf8798...925e498](https://codecov.io/gh/google/pprof/pull/559?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

kalyanac: Restarted the failed test.
nolanmar511: I think the two test failures are unrelated to this change; I don' t think re-running will help.

See https://github.com/google/pprof/pull/560#issuecomment-699088881.
codecov-io: # [Codecov](https://codecov.io/gh/google/pprof/pull/559?src=pr&el=h1) Report
> Merging [#559](https://codecov.io/gh/google/pprof/pull/559?src=pr&el=desc) (54167b5) into [master](https://codecov.io/gh/google/pprof/commit/ae444373da1930279a05f306241dbc990a4f8798?el=desc) (ae44437) will **increase** coverage by `0.05%`.
> The diff coverage is `100.00%`.

[![Impacted file tree graph](https://codecov.io/gh/google/pprof/pull/559/graphs/tree.svg?width=650&height=150&src=pr&token=nHgs4Smibv)](https://codecov.io/gh/google/pprof/pull/559?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master     #559      +/-   ##
==========================================
+ Coverage   67.14%   67.19%   +0.05%     
==========================================
  Files          78       78              
  Lines       14082    14098      +16     
==========================================
+ Hits         9455     9473      +18     
+ Misses       3792     3791       -1     
+ Partials      835      834       -1     
```


| [Impacted Files](https://codecov.io/gh/google/pprof/pull/559?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [profile/profile.go](https://codecov.io/gh/google/pprof/pull/559/diff?src=pr&el=tree#diff-cHJvZmlsZS9wcm9maWxlLmdv) | `69.67% <100.00%> (+1.46%)` | :arrow_up: |
| [.../github.com/google/pprof/internal/report/source.go](https://codecov.io/gh/google/pprof/pull/559/diff?src=pr&el=tree#diff-c3JjL2dpdGh1Yi5jb20vZ29vZ2xlL3Bwcm9mL2ludGVybmFsL3JlcG9ydC9zb3VyY2UuZ28=) | `80.76% <0.00%> (-0.65%)` | :arrow_down: |
| [src/github.com/google/pprof/profile/profile.go](https://codecov.io/gh/google/pprof/pull/559/diff?src=pr&el=tree#diff-c3JjL2dpdGh1Yi5jb20vZ29vZ2xlL3Bwcm9mL3Byb2ZpbGUvcHJvZmlsZS5nbw==) | `69.67% <0.00%> (+1.46%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/google/pprof/pull/559?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/google/pprof/pull/559?src=pr&el=footer). Last update [ae44437...54167b5](https://codecov.io/gh/google/pprof/pull/559?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

aalexand: @nolanmar511 Interestingly there are still CI failures.
nolanmar511: Just looking at this. The failures are also happening on the main branch.
I've filed https://github.com/google/pprof/issues/585.
nolanmar511: Tests are all passing now. PTAL

Patch:
diff --git a/profile/profile.go b/profile/profile.go
index d94d8b3..2590c8d 100644
--- a/profile/profile.go
+++ b/profile/profile.go
@@ -22,6 +22,7 @@ import (
 	"fmt"
 	"io"
 	"io/ioutil"
+	"math"
 	"path/filepath"
 	"regexp"
 	"sort"
@@ -712,7 +713,8 @@ func (s *Sample) DiffBaseSample() bool {
 	return s.HasLabel("pprof::base", "true")
 }
 
-// Scale multiplies all sample values in a profile by a constant.
+// Scale multiplies all sample values in a profile by a constant and keeps
+// only samples that have at least one non-zero value.
 func (p *Profile) Scale(ratio float64) {
 	if ratio == 1 {
 		return
@@ -724,7 +726,8 @@ func (p *Profile) Scale(ratio float64) {
 	p.ScaleN(ratios)
 }
 
-// ScaleN multiplies each sample values in a sample by a different amount.
+// ScaleN multiplies each sample values in a sample by a different amount
+// and keeps only samples that have at least one non-zero value.
 func (p *Profile) ScaleN(ratios []float64) error {
 	if len(p.SampleType) != len(ratios) {
 		return fmt.Errorf("mismatched scale ratios, got %d, want %d", len(ratios), len(p.SampleType))
@@ -739,13 +742,22 @@ func (p *Profile) ScaleN(ratios []float64) error {
 	if allOnes {
 		return nil
 	}
+	fillIdx := 0
 	for _, s := range p.Sample {
+		keepSample := false
 		for i, v := range s.Value {
 			if ratios[i] != 1 {
-				s.Value[i] = int64(float64(v) * ratios[i])
+				val := int64(math.Round(float64(v) * ratios[i]))
+				s.Value[i] = val
+				keepSample = keepSample || val != 0
 			}
 		}
+		if keepSample {
+			p.Sample[fillIdx] = s
+			fillIdx++
+		}
 	}
+	p.Sample = p.Sample[:fillIdx]
 	return nil
 }
 


Test patch:
diff --git a/internal/driver/fetch_test.go b/internal/driver/fetch_test.go
index f7fc1b8..fb1cc06 100644
--- a/internal/driver/fetch_test.go
+++ b/internal/driver/fetch_test.go
@@ -336,27 +336,27 @@ func TestFetchWithBase(t *testing.T) {
 			true,
 			[]WantSample{
 				{
-					values: []int64{-229, -370},
+					values: []int64{-229, -369},
 					labels: map[string][]string{},
 				},
 				{
-					values: []int64{28, 0},
+					values: []int64{29, 0},
 					labels: map[string][]string{},
 				},
 				{
-					values: []int64{57, 0},
+					values: []int64{57, 1},
 					labels: map[string][]string{},
 				},
 				{
-					values: []int64{28, 80},
+					values: []int64{29, 80},
 					labels: map[string][]string{},
 				},
 				{
-					values: []int64{28, 0},
+					values: []int64{29, 0},
 					labels: map[string][]string{},
 				},
 				{
-					values: []int64{85, 287},
+					values: []int64{86, 288},
 					labels: map[string][]string{},
 				},
 			},
diff --git a/profile/profile_test.go b/profile/profile_test.go
index 49e1022..0bf51a8 100644
--- a/profile/profile_test.go
+++ b/profile/profile_test.go
@@ -641,6 +641,74 @@ func checkAggregation(prof *Profile, a *aggTest) error {
 	return nil
 }
 
+// TestScale tests that Scale() rounds values and drops samples
+// as expected.
+func TestScale(t *testing.T) {
+	for _, tc := range []struct {
+		desc        string
+		ratio       float64
+		p           *Profile
+		wantSamples [][]int64
+	}{
+		{
+			desc:  "scale by 1",
+			ratio: 1.0,
+			p:     testProfile1.Copy(),
+			wantSamples: [][]int64{
+				{1000, 1000},
+				{100, 100},
+				{10, 10},
+				{10000, 10000},
+				{1, 1},
+			},
+		},
+		{
+			desc:  "sample values will be rounded up",
+			ratio: .66666,
+			p:     testProfile1.Copy(),
+			wantSamples: [][]int64{
+				{667, 667},
+				{67, 67},
+				{7, 7},
+				{6667, 6667},
+				{1, 1},
+			},
+		},
+		{
+			desc:  "sample values will be rounded down",
+			ratio: .33333,
+			p:     testProfile1.Copy(),
+			wantSamples: [][]int64{
+				{333, 333},
+				{33, 33},
+				{3, 3},
+				{3333, 3333},
+			},
+		},
+		{
+			desc:        "all sample values will be dropped",
+			ratio:       0.00001,
+			p:           testProfile1.Copy(),
+			wantSamples: [][]int64{},
+		},
+	} {
+		t.Run(tc.desc, func(t *testing.T) {
+			tc.p.Scale(tc.ratio)
+			if got, want := len(tc.p.Sample), len(tc.wantSamples); got != want {
+				t.Fatalf("got %d samples, want %d", got, want)
+			}
+			for i, s := range tc.p.Sample {
+				for j, got := range s.Value {
+					want := tc.wantSamples[i][j]
+					if want != got {
+						t.Errorf("For value %d of sample %d, got %d want %d", j, i, got, want)
+					}
+				}
+			}
+		})
+	}
+}
+
 // TestMergeMain tests merge leaves the main binary in place.
 func TestMergeMain(t *testing.T) {
 	prof := testProfile1.Copy()
@@ -864,10 +932,9 @@ func TestNormalizeByDifferentProfile(t *testing.T) {
 	}
 
 	expectedSampleValues := [][]int64{
-		{19, 1000},
-		{1, 100},
-		{0, 10},
-		{198, 10000},
+		{20, 1000},
+		{2, 100},
+		{199, 10000},
 		{0, 1},
 	}