From b97a9e861851c578d409a80b21bb28221a173baa Mon Sep 17 00:00:00 2001
From: Marco Concetto Rudilosso <marcoconcettorudilosso@gmail.com>
Date: Mon, 18 Dec 2017 13:18:34 +0000
Subject: [PATCH] review suggestions implemented

---
 .../testdata/pprof.cpu.flat.addresses.weblist | 21 ++---
 internal/report/source.go                     | 71 +++++++---------
 internal/report/source_html.go                | 15 +---
 internal/report/source_test.go                | 83 +++++++++++++------
 4 files changed, 98 insertions(+), 92 deletions(-)

diff --git a/internal/driver/testdata/pprof.cpu.flat.addresses.weblist b/internal/driver/testdata/pprof.cpu.flat.addresses.weblist
index 4146534fb..8a1d18576 100644
--- a/internal/driver/testdata/pprof.cpu.flat.addresses.weblist
+++ b/internal/driver/testdata/pprof.cpu.flat.addresses.weblist
@@ -38,20 +38,11 @@ background-color: #eeeeee;
 color: #008800;
 display: none;
 }
-.percentile_80 {
+.ptile_95 {
 background-color: #fe5252
 }
-.percentile_60 {
-background-color: #e09764
-}
-.percentile_40 {
-background-color: #d1b96d
-}
-.percentile_20 {
-background-color: #c3dc76
-}
-.percentile_10 {
-background-color: #b3ff80
+.ptile_80 {
+background-color: #ffb6c1
 }
 </style>
 <script type="text/javascript">
@@ -79,7 +70,7 @@ Type: cpu<br>
 Duration: 10s, Total samples = 1.12s (11.20%)<br>Total: 1.12s</div><h2>line1000</h2><p class="filename">testdata/file1000.src</p>
 <pre onClick="pprof_toggle_asm(event)">
   Total:       1.10s      1.10s (flat, cum) 98.21%
-<span class=line>      1</span> <span class="deadsrc percentile_80">       1.10s      1.10s           line1 </span><span class=asm>               1.10s      1.10s     1000:     instruction one                                                              <span class=unimportant>file1000.src:1</span>
+<span class=line>      1</span> <span class="deadsrc ptile_80">       1.10s      1.10s           line1 </span><span class=asm>               1.10s      1.10s     1000:     instruction one                                                              <span class=unimportant>file1000.src:1</span>
                    .          .     1001:     instruction two                                                              <span class=unimportant>file1000.src:1</span>
                                      ⋮
                    .          .     1003:     instruction four                                                             <span class=unimportant>file1000.src:1</span>
@@ -100,11 +91,11 @@ Duration: 10s, Total samples = 1.12s (11.20%)<br>Total: 1.12s</div><h2>line1000<
 <span class=line>      3</span> <span class="nop">           .          .           line3 </span>
 <span class=line>      4</span> <span class="nop">           .          .           line4 </span>
 <span class=line>      5</span> <span class="nop">           .          .           line5 </span>
-<span class=line>      6</span> <span class="deadsrc percentile_80">        10ms      1.01s           line6 </span><span class=asm>                10ms      1.01s     3000:     instruction one                                                              <span class=unimportant>file3000.src:6</span>
+<span class=line>      6</span> <span class="deadsrc ptile_80">        10ms      1.01s           line6 </span><span class=asm>                10ms      1.01s     3000:     instruction one                                                              <span class=unimportant>file3000.src:6</span>
 </span>
 <span class=line>      7</span> <span class="nop">           .          .           line7 </span>
 <span class=line>      8</span> <span class="nop">           .          .           line8 </span>
-<span class=line>      9</span> <span class="deadsrc percentile_60">           .      110ms           line9 </span><span class=asm>                   .      100ms     3001:     instruction two                                                              <span class=unimportant>file3000.src:9</span>
+<span class=line>      9</span> <span class="deadsrc ptile_80">           .      110ms           line9 </span><span class=asm>                   .      100ms     3001:     instruction two                                                              <span class=unimportant>file3000.src:9</span>
                    .       10ms     3002:     instruction three                                                            <span class=unimportant>file3000.src:9</span>
                    .          .     3003:     instruction four                                                             <span class=unimportant></span>
                    .          .     3004:     instruction five                                                             <span class=unimportant></span>
diff --git a/internal/report/source.go b/internal/report/source.go
index 3667b6276..ddf82af6e 100644
--- a/internal/report/source.go
+++ b/internal/report/source.go
@@ -215,71 +215,64 @@ func PrintWebList(w io.Writer, rpt *Report, obj plugin.ObjTool, maxFiles int) er
 		}
 
 		printFunctionHeader(w, ff.functionName, path, n.Flat, n.Cum, rpt)
-		percentiles := calculatePercentiles(fnodes)
+		ptiles := calculatePtiles(fnodes)
 		for _, fn := range fnodes {
-			printFunctionSourceLine(w, fn, asm[fn.Info.Lineno], reader, getPercentileString(float64(fn.Cum), percentiles), rpt)
+			printFunctionSourceLine(w, fn, asm[fn.Info.Lineno], reader, getPtileCSSClassName(fn.Cum, ptiles), rpt)
 		}
 		printFunctionClosing(w)
 	}
 	return nil
 }
 
-func getPercentileString(cumSum float64, percentiles map[float64]float64) string {
+func getPtileCSSClassName(cumSum int64, ptiles map[int64]int64) string {
 	if cumSum == 0 {
 		return ""
 	}
-	switch {
-	case cumSum >= percentiles[80]:
-		return " percentile_80"
-	case cumSum >= percentiles[60]:
-		return " percentile_60"
-	case cumSum >= percentiles[40]:
-		return " percentile_40"
-	case cumSum >= percentiles[20]:
-		return " percentile_20"
-	case cumSum >= percentiles[10]:
-		return " percentile_10"
+	for key, value := range ptiles {
+		if cumSum > value {
+			return " ptile_" + strconv.FormatInt(key, 10)
+		}
 	}
 	return ""
 }
 
-// calculate percentile expects cumSums to be sorted and
-// to contain unique elements. It also expects the percentile to be between 0 and 99.
-func calculatePercentile(percentile float64, cumSums []float64) float64 {
-	rank := percentile / 100 * float64(len(cumSums))
+// calculatePtile expects cumSums to be sorted and
+// to contain unique elements. It also expects the ptile to be between 0 and 99.
+func calculatePtile(ptile int64, cumSums []int64) int64 {
+	rank := float64(ptile) / 100 * float64(len(cumSums))
 	return cumSums[int64(rank)]
 }
 
-func getSetOfCumValues(fnodes graph.Nodes) []float64 {
-	mapOfCumValues := make(map[int64]bool)
+func getArrayOfCumValues(fnodes graph.Nodes) []int64 {
+	arrayOfCumValues := make([]int64, 0, len(fnodes))
 
 	for _, fn := range fnodes {
-		if _, ok := mapOfCumValues[fn.Cum]; !ok {
-			mapOfCumValues[fn.Cum] = true
-		}
+		arrayOfCumValues = append(arrayOfCumValues, fn.Cum)
 	}
 
-	setOfCumValues := make([]float64, 0, len(mapOfCumValues))
-	for key := range mapOfCumValues {
-		setOfCumValues = append(setOfCumValues, float64(key))
-	}
-	return setOfCumValues
+	return arrayOfCumValues
 }
 
-// calculatePercentiles returns nil when the fnodes is 0
+type int64Slice []int64
+
+func (a int64Slice) Len() int           { return len(a) }
+func (a int64Slice) Swap(i, j int)      { a[i], a[j] = a[j], a[i] }
+func (a int64Slice) Less(i, j int) bool { return a[i] < a[j] }
+
+// calculatePtiles returns nil when the fnodes is 0
 // because its result will never be used in such a case.
-func calculatePercentiles(fnodes graph.Nodes) map[float64]float64 {
+func calculatePtiles(fnodes graph.Nodes) map[int64]int64 {
 	if len(fnodes) == 0 {
 		return nil
 	}
-	setOfCumValues := getSetOfCumValues(fnodes)
-	percentiles := map[float64]float64{80: 0, 60: 0, 40: 0, 20: 0, 10: 0}
-	sort.Float64s(setOfCumValues)
-	for key := range percentiles {
-		percentiles[key] = calculatePercentile(key, setOfCumValues)
+	arrayOfCumValues := getArrayOfCumValues(fnodes)
+	ptiles := map[int64]int64{95: 0, 80: 0}
+	sort.Sort(int64Slice(arrayOfCumValues))
+	for key := range ptiles {
+		ptiles[key] = calculatePtile(key, arrayOfCumValues)
 	}
 
-	return percentiles
+	return ptiles
 }
 
 // sourceCoordinates returns the lowest and highest line numbers from
@@ -408,12 +401,12 @@ func printFunctionHeader(w io.Writer, name, path string, flatSum, cumSum int64,
 }
 
 // printFunctionSourceLine prints a source line and the corresponding assembly.
-func printFunctionSourceLine(w io.Writer, fn *graph.Node, assembly []assemblyInstruction, reader *sourceReader, percentileString string, rpt *Report) {
+func printFunctionSourceLine(w io.Writer, fn *graph.Node, assembly []assemblyInstruction, reader *sourceReader, ptileCSSClassName string, rpt *Report) {
 	if len(assembly) == 0 {
 		fmt.Fprintf(w,
 			"<span class=line> %6d</span> <span class=\"nop%s\">  %10s %10s %8s  %s </span>\n",
 			fn.Info.Lineno,
-			percentileString,
+			ptileCSSClassName,
 			valueOrDot(fn.Flat, rpt), valueOrDot(fn.Cum, rpt),
 			"", template.HTMLEscapeString(fn.Info.Name))
 		return
@@ -422,7 +415,7 @@ func printFunctionSourceLine(w io.Writer, fn *graph.Node, assembly []assemblyIns
 	fmt.Fprintf(w,
 		"<span class=line> %6d</span> <span class=\"deadsrc%s\">  %10s %10s %8s  %s </span>",
 		fn.Info.Lineno,
-		percentileString,
+		ptileCSSClassName,
 		valueOrDot(fn.Flat, rpt), valueOrDot(fn.Cum, rpt),
 		"", template.HTMLEscapeString(fn.Info.Name))
 	srcIndent := indentation(fn.Info.Name)
diff --git a/internal/report/source_html.go b/internal/report/source_html.go
index c1d50d7bf..92ecb5e44 100644
--- a/internal/report/source_html.go
+++ b/internal/report/source_html.go
@@ -58,20 +58,11 @@ background-color: #eeeeee;
 color: #008800;
 display: none;
 }
-.percentile_80 {
+.ptile_95 {
 background-color: #fe5252
 }
-.percentile_60 {
-background-color: #e09764
-}
-.percentile_40 {
-background-color: #d1b96d
-}
-.percentile_20 {
-background-color: #c3dc76
-}
-.percentile_10 {
-background-color: #b3ff80
+.ptile_80 {
+background-color: #ffb6c1
 }
 </style>`
 
diff --git a/internal/report/source_test.go b/internal/report/source_test.go
index 50a1852f5..0471ec71d 100644
--- a/internal/report/source_test.go
+++ b/internal/report/source_test.go
@@ -7,6 +7,7 @@ import (
 	"reflect"
 	"regexp"
 	"runtime"
+	"strconv"
 	"strings"
 	"testing"
 
@@ -15,43 +16,73 @@ import (
 	"github.com/google/pprof/profile"
 )
 
-func TestCalculatePercentiles(t *testing.T) {
+type prettyNodes graph.Nodes
+
+func (nodes prettyNodes) String() string {
+	strs := make([]string, 0, len(nodes)+2)
+	strs = append(strs, "[")
+	for _, node := range nodes {
+		strs = append(strs, "Cum: "+strconv.FormatInt(node.Cum, 10))
+	}
+	strs = append(strs, "]")
+	return strings.Join(strs, " ")
+}
+
+func TestCalculatePtiles(t *testing.T) {
 	for _, testCase := range []struct {
-		fnodes graph.Nodes
-		output map[float64]float64
+		nodes graph.Nodes
+		want  map[int64]int64
 	}{
 		{
-			fnodes: []*graph.Node{},
-			output: nil,
+			nodes: nil,
+			want:  nil,
+		},
+		{
+			nodes: []*graph.Node{
+				{Cum: 0},
+				{Cum: 8},
+			},
+			want: map[int64]int64{80: 8, 95: 8},
+		},
+		{
+			nodes: []*graph.Node{
+				{Cum: 10},
+				{Cum: 5},
+				{Cum: 8},
+			},
+			want: map[int64]int64{80: 10, 95: 10},
 		},
 		{
-			fnodes: []*graph.Node{
-				{
-					Cum: 0,
-				},
-				{
-					Cum: 8,
-				},
+			nodes: []*graph.Node{
+				{Cum: 0},
+				{Cum: 0},
+				{Cum: 0},
+				{Cum: 0},
+				{Cum: 11},
+				{Cum: 2},
+				{Cum: 9},
+				{Cum: 4},
+				{Cum: 8},
+				{Cum: 2},
+				{Cum: 16},
+				{Cum: 22},
+				{Cum: 13},
 			},
-			output: map[float64]float64{10: 0, 20: 0, 40: 0, 60: 8, 80: 8},
+			want: map[int64]int64{80: 13, 95: 22},
 		},
 		{
-			fnodes: []*graph.Node{
-				{
-					Cum: 10,
-				},
-				{
-					Cum: 5,
-				},
-				{
-					Cum: 8,
-				},
+			nodes: []*graph.Node{
+				{Cum: 10},
+				{Cum: 10},
+				{Cum: 10},
+				{Cum: 10},
+				{Cum: 10},
 			},
-			output: map[float64]float64{10: 5, 20: 5, 40: 8, 60: 8, 80: 10},
+			want: map[int64]int64{80: 10, 95: 10},
 		},
 	} {
-		if percentiles := calculatePercentiles(testCase.fnodes); !reflect.DeepEqual(percentiles, testCase.output) {
-			t.Errorf("%v is not equal to %v", percentiles, testCase.output)
+		if ptiles := calculatePtiles(testCase.nodes); !reflect.DeepEqual(ptiles, testCase.want) {
+			t.Errorf("calculatePtiles(%v) = %v; want %v", prettyNodes(testCase.nodes), ptiles, testCase.want)
 		}
 	}
 }