Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Colorize weblist output #271

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Colorize weblist output #271

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
b64b875
Colorize weblist output
Maaarcocr Nov 29, 2017
3c1cc88
updating testdata
Maaarcocr Dec 1, 2017
f20b0fa
getting rid of percentile_0
Maaarcocr Dec 1, 2017
54536e0
Actually calculating percentiles. Missing tests.
Maaarcocr Dec 1, 2017
2393a99
added tests
Maaarcocr Dec 1, 2017
7f5dfe6
formatted
Maaarcocr Dec 2, 2017
f8123b3
review suggestions implemented
Maaarcocr Dec 18, 2017
3e3a1c0
Merge branch 'master' into master
nolanmar511 Dec 16, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
57 changes: 36 additions & 21 deletions internal/driver/testdata/pprof.cpu.flat.addresses.weblist
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,21 @@ background-color: #eeeeee;
color: #008800;
display: none;
}
.percentile_80 {
background-color: #fe5252
}
.percentile_60 {
background-color: #e09764
}
.percentile_40 {
background-color: #d1b96d
}
.percentile_20 {
background-color: #c3dc76
}
.percentile_10 {
background-color: #b3ff80
}
</style>
<script type="text/javascript">
function pprof_toggle_asm(e) {
Expand All @@ -64,41 +79,41 @@ 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> 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 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>
. . 1001: instruction two <span class=unimportant>file1000.src:1</span>
⋮
. . 1003: instruction four <span class=unimportant>file1000.src:1</span>
</span>
<span class=line> 2</span> <span class=deadsrc> . . line2 </span><span class=asm> . . 1002: instruction three <span class=unimportant>file1000.src:2</span>
<span class=line> 2</span> <span class="deadsrc"> . . line2 </span><span class=asm> . . 1002: instruction three <span class=unimportant>file1000.src:2</span>
</span>
<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=nop> . . line6 </span>
<span class=line> 7</span> <span class=nop> . . line7 </span>
<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="nop"> . . line6 </span>
<span class=line> 7</span> <span class="nop"> . . line7 </span>
</pre>
<h2>line3000</h2><p class="filename">testdata/file3000.src</p>
<pre onClick="pprof_toggle_asm(event)">
Total: 10ms 1.12s (flat, cum) 100%
<span class=line> 1</span> <span class=nop> . . line1 </span>
<span class=line> 2</span> <span class=nop> . . line2 </span>
<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> 10ms 1.01s line6 </span><span class=asm> 10ms 1.01s 3000: instruction one <span class=unimportant>file3000.src:6</span>
<span class=line> 1</span> <span class="nop"> . . line1 </span>
<span class=line> 2</span> <span class="nop"> . . line2 </span>
<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>
<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> . 110ms line9 </span><span class=asm> . 100ms 3001: instruction two <span class=unimportant>file3000.src:9</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>
. 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>
</span>
<span class=line> 10</span> <span class=nop> . . line0 </span>
<span class=line> 11</span> <span class=nop> . . line1 </span>
<span class=line> 12</span> <span class=nop> . . line2 </span>
<span class=line> 13</span> <span class=nop> . . line3 </span>
<span class=line> 14</span> <span class=nop> . . line4 </span>
<span class=line> 10</span> <span class="nop"> . . line0 </span>
<span class=line> 11</span> <span class="nop"> . . line1 </span>
<span class=line> 12</span> <span class="nop"> . . line2 </span>
<span class=line> 13</span> <span class="nop"> . . line3 </span>
<span class=line> 14</span> <span class="nop"> . . line4 </span>
</pre>

</body>
Expand Down
70 changes: 66 additions & 4 deletions internal/report/source.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import (
"io"
"os"
"path/filepath"
"sort"
"strconv"
"strings"

Expand Down Expand Up @@ -214,14 +215,73 @@ 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)
for _, fn := range fnodes {
printFunctionSourceLine(w, fn, asm[fn.Info.Lineno], reader, rpt)
printFunctionSourceLine(w, fn, asm[fn.Info.Lineno], reader, getPercentileString(float64(fn.Cum), percentiles), rpt)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getPercentileString - this is not just string, this is CSS class name?

}
printFunctionClosing(w)
}
return nil
}

func getPercentileString(cumSum float64, percentiles map[float64]float64) string {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this function can be simplified with a for loop, there is unnecessary repetition.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using float64 as the map key is quite unusual.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd suggest replacing "Percentile" with "Ptile" throughout. Using the full term makes the code unnecessarily verbose for the task.

if cumSum == 0 {
return ""
}
switch {
case cumSum >= percentiles[80]:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you choose the specific percentiles of 10, 20, 40, 60 and 80?

Looking at the whole PR, I think that we should start with something simpler.

  • Let's start with just two colors: pink and red.
  • Percentiles which are calculated off unique set built of float64 numbers look weird, frankly, so not sure it was a good idea. I think the easier to understand thing is to base the coloring on the max value among of the source lines.
    • Color as pink values that are > 80% of the max value.
    • Color as red values that are > 95% of the max value.
    • Leave all other values as white.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have implemented these changes. The code now calculates the percentile on the array of cum values, instead of a set. I'm not using float64s anymore, instead I use int64. This last change meant that I had to implement the Sort Interface for int64.

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"
}
return ""
}

// calculate percentile expects cumSums to be sorted and
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doc should start with function name.

// to contain unique elements. It also expects the percentile to be between 0 and 99.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why 99 is the right bound, not 100? 100th ptile is the max value conceptually.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It depends on how you define percentile. 100th ptile would mean that the value is bigger than any value in the list, thus it would not exist in the list (this is my understading of ptile, correct me if I'm wrong).

func calculatePercentile(percentile float64, cumSums []float64) float64 {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

calcPtile

rank := percentile / 100 * float64(len(cumSums))
return cumSums[int64(rank)]
}

func getSetOfCumValues(fnodes graph.Nodes) []float64 {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should explain what this function is doing and why. E.g. the choice of deduplicating the different values before computing the percentile values is non-obvious, we should clearly explain the tactics and document the rationale.

mapOfCumValues := make(map[int64]bool)

for _, fn := range fnodes {
if _, ok := mapOfCumValues[fn.Cum]; !ok {
mapOfCumValues[fn.Cum] = true
}
}

setOfCumValues := make([]float64, 0, len(mapOfCumValues))
for key, _ := range mapOfCumValues {
setOfCumValues = append(setOfCumValues, float64(key))
}
return setOfCumValues
}

// calculatePercentiles 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 {
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)
}

return percentiles
}

// sourceCoordinates returns the lowest and highest line numbers from
// a set of assembly statements.
func sourceCoordinates(asm map[int][]assemblyInstruction) (start, end int) {
Expand Down Expand Up @@ -348,19 +408,21 @@ 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, rpt *Report) {
func printFunctionSourceLine(w io.Writer, fn *graph.Node, assembly []assemblyInstruction, reader *sourceReader, percentileString string, rpt *Report) {
if len(assembly) == 0 {
fmt.Fprintf(w,
"<span class=line> %6d</span> <span class=nop> %10s %10s %8s %s </span>\n",
"<span class=line> %6d</span> <span class=\"nop%s\"> %10s %10s %8s %s </span>\n",
fn.Info.Lineno,
percentileString,
valueOrDot(fn.Flat, rpt), valueOrDot(fn.Cum, rpt),
"", template.HTMLEscapeString(fn.Info.Name))
return
}

fmt.Fprintf(w,
"<span class=line> %6d</span> <span class=deadsrc> %10s %10s %8s %s </span>",
"<span class=line> %6d</span> <span class=\"deadsrc%s\"> %10s %10s %8s %s </span>",
fn.Info.Lineno,
percentileString,
valueOrDot(fn.Flat, rpt), valueOrDot(fn.Cum, rpt),
"", template.HTMLEscapeString(fn.Info.Name))
srcIndent := indentation(fn.Info.Name)
Expand Down
15 changes: 15 additions & 0 deletions internal/report/source_html.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,21 @@ background-color: #eeeeee;
color: #008800;
display: none;
}
.percentile_80 {
background-color: #fe5252
}
.percentile_60 {
background-color: #e09764
}
.percentile_40 {
background-color: #d1b96d
}
.percentile_20 {
background-color: #c3dc76
}
.percentile_10 {
background-color: #b3ff80
}
</style>`

const weblistPageScript = `<script type="text/javascript">
Expand Down
43 changes: 43 additions & 0 deletions internal/report/source_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,58 @@ import (
"bytes"
"os"
"path/filepath"
"reflect"
"regexp"
"runtime"
"strings"
"testing"

"github.com/google/pprof/internal/binutils"
"github.com/google/pprof/internal/graph"
"github.com/google/pprof/profile"
)

func TestCalculatePercentiles(t *testing.T) {
for _, testCase := range []struct {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there should be at least these additional test cases:

  • Duplicate values.
  • All equal values.

fnodes graph.Nodes
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does "f" in "fnodes" stand for here?

output map[float64]float64
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

output -> want

}{
{
fnodes: []*graph.Node{},
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prefer nil for empty slices - https://github.com/golang/go/wiki/CodeReviewComments#declaring-empty-slices which is also the zero value here.

output: nil,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nil is the zero value for reference types, no need to have the initializer for the field.

},
{
fnodes: []*graph.Node{
&graph.Node{
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

&graph.Node{Cum: 0},, using the multiline style here is unnecessarily sparse.

Cum: 0,
},
&graph.Node{
Cum: 8,
},
},
output: map[float64]float64{10: 0, 20: 0, 40: 0, 60: 8, 80: 8},
},
{
fnodes: []*graph.Node{
&graph.Node{
Cum: 10,
},
&graph.Node{
Cum: 5,
},
&graph.Node{
Cum: 8,
},
},
output: map[float64]float64{10: 5, 20: 5, 40: 8, 60: 8, 80: 10},
},
} {
if percentiles := calculatePercentiles(testCase.fnodes); !reflect.DeepEqual(percentiles, testCase.output) {
t.Errorf("%v is not equal to %v", percentiles, testCase.output)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/golang/go/wiki/CodeReviewComments#useful-test-failures

}
}
}

func TestWebList(t *testing.T) {
if runtime.GOOS != "linux" || runtime.GOARCH != "amd64" {
t.Skip("weblist only tested on x86-64 linux")
Expand Down