-
Notifications
You must be signed in to change notification settings - Fork 618
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.
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
Colorize weblist output #271
Changes from 5 commits
b64b875
3c1cc88
f20b0fa
54536e0
2393a99
7f5dfe6
f8123b3
3e3a1c0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -24,6 +24,7 @@ import ( | |
"io" | ||
"os" | ||
"path/filepath" | ||
"sort" | ||
"strconv" | ||
"strings" | ||
|
||
|
@@ -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) | ||
} | ||
printFunctionClosing(w) | ||
} | ||
return nil | ||
} | ||
|
||
func getPercentileString(cumSum float64, percentiles map[float64]float64) string { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Using float64 as the map key is quite unusual. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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]: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
rank := percentile / 100 * float64(len(cumSums)) | ||
return cumSums[int64(rank)] | ||
} | ||
|
||
func getSetOfCumValues(fnodes graph.Nodes) []float64 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) { | ||
|
@@ -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) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think there should be at least these additional test cases:
|
||
fnodes graph.Nodes | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What does "f" in "fnodes" stand for here? |
||
output map[float64]float64 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
}{ | ||
{ | ||
fnodes: []*graph.Node{}, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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{ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
} | ||
} | ||
} | ||
|
||
func TestWebList(t *testing.T) { | ||
if runtime.GOOS != "linux" || runtime.GOARCH != "amd64" { | ||
t.Skip("weblist only tested on x86-64 linux") | ||
|
There was a problem hiding this comment.
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?