[go] runtime/pprof: consume tag for first CPU record
0 views
Skip to first unread message
Michael Pratt (Gerrit)
Dec 7, 2021, 5:33:38 PM12/7/21
to Michael Pratt, goph...@pubsubhelper.golang.org, golang-...@googlegroups.com, Gopher Robot, Austin Clements, Cherry Mui, Michael Knyszek, Felix Geisendörfer, golang-co...@googlegroups.com
Michael Pratt submitted this change.
2 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: src/runtime/pprof/proto.go
Insertions: 2, Deletions: 0.
@@ -283,6 +283,8 @@
b.period = 1e9 / int64(data[2])
b.havePeriod = true
data = data[3:]
+ // Consume tag slot. Note that there isn't a meaningful tag
+ // value for this record.
tags = tags[1:]
}
```
Approvals:
Austin Clements: Looks good to me, approved
Michael Pratt: Trusted; Run TryBots
Gopher Robot: TryBots succeeded
runtime/pprof: consume tag for first CPU record
profBuf.write uses an index in b.tags for each entry, even if that entry
has no tag (that slice entry just remains 0). profBuf.read similarly
returns a tags slice with exactly as many entries as there are records
in data.
profileBuilder.addCPUData iterates through the tags in lockstep with the
data records. Except in the special case of the first record, where it
forgets to increment tags. Thus the first read of profiling data has all
tags off-by-one.
To help avoid regressions, addCPUData is changed to assert that tags
contains exactly the correct number of tags.
For #50007.
Change-Id: I5f32f93003297be8d6e33ad472c185d924a63256
Reviewed-on: https://go-review.googlesource.com/c/go/+/369741
Reviewed-by: Austin Clements <aus...@google.com>
Trust: Michael Pratt <mpr...@google.com>
Run-TryBot: Michael Pratt <mpr...@google.com>
TryBot-Result: Gopher Robot <go...@golang.org>
---
M src/runtime/pprof/pprof_test.go
M src/runtime/pprof/proto_test.go
M src/runtime/pprof/proto.go
M src/runtime/cpuprof.go
4 files changed, 66 insertions(+), 13 deletions(-)
diff --git a/src/runtime/cpuprof.go b/src/runtime/cpuprof.go
index 6076564..48cef46 100644
--- a/src/runtime/cpuprof.go
+++ b/src/runtime/cpuprof.go
@@ -200,6 +200,8 @@
// If profiling is turned off and all the profile data accumulated while it was
// on has been returned, readProfile returns eof=true.
// The caller must save the returned data and tags before calling readProfile again.
+// The returned data contains a whole number of records, and tags contains
+// exactly one entry per record.
//
//go:linkname runtime_pprof_readProfile runtime/pprof.readProfile
func runtime_pprof_readProfile() ([]uint64, []unsafe.Pointer, bool) {
diff --git a/src/runtime/pprof/pprof_test.go b/src/runtime/pprof/pprof_test.go
index e32928b..913f899 100644
--- a/src/runtime/pprof/pprof_test.go
+++ b/src/runtime/pprof/pprof_test.go
@@ -1608,6 +1608,7 @@
testCases := []struct {
name string
input []uint64 // following the input format assumed by profileBuilder.addCPUData.
+ count int // number of records in input.
wantLocs [][]string // ordered location entries with function names.
wantSamples []*profile.Sample // ordered samples, we care only about Value and the profile location IDs.
}{{
@@ -1617,6 +1618,7 @@
3, 0, 500, // hz = 500. Must match the period.
5, 0, 50, inlinedCallerStack[0], inlinedCallerStack[1],
},
+ count: 2,
wantLocs: [][]string{
{"runtime/pprof.inlinedCalleeDump", "runtime/pprof.inlinedCallerDump"},
},
@@ -1633,6 +1635,7 @@
7, 0, 10, inlinedCallerStack[0], inlinedCallerStack[1], inlinedCallerStack[0], inlinedCallerStack[1],
5, 0, 20, inlinedCallerStack[0], inlinedCallerStack[1],
},
+ count: 3,
wantLocs: [][]string{{"runtime/pprof.inlinedCalleeDump", "runtime/pprof.inlinedCallerDump"}},
wantSamples: []*profile.Sample{
{Value: []int64{10, 10 * period}, Location: []*profile.Location{{ID: 1}, {ID: 1}}},
@@ -1646,6 +1649,7 @@
// entry. The "stk" entry is actually the count.
4, 0, 0, 4242,
},
+ count: 2,
wantLocs: [][]string{{"runtime/pprof.lostProfileEvent"}},
wantSamples: []*profile.Sample{
{Value: []int64{4242, 4242 * period}, Location: []*profile.Location{{ID: 1}}},
@@ -1664,6 +1668,7 @@
5, 0, 30, inlinedCallerStack[0], inlinedCallerStack[0],
4, 0, 40, inlinedCallerStack[0],
},
+ count: 3,
// inlinedCallerDump shows up here because
// runtime_expandFinalInlineFrame adds it to the stack frame.
wantLocs: [][]string{{"runtime/pprof.inlinedCalleeDump"}, {"runtime/pprof.inlinedCallerDump"}},
@@ -1677,6 +1682,7 @@
3, 0, 500, // hz = 500. Must match the period.
9, 0, 10, recursionStack[0], recursionStack[1], recursionStack[2], recursionStack[3], recursionStack[4], recursionStack[5],
},
+ count: 2,
wantLocs: [][]string{
{"runtime/pprof.recursionChainBottom"},
{
@@ -1700,6 +1706,7 @@
5, 0, 50, inlinedCallerStack[0], inlinedCallerStack[1],
4, 0, 60, inlinedCallerStack[0],
},
+ count: 3,
wantLocs: [][]string{{"runtime/pprof.inlinedCalleeDump", "runtime/pprof.inlinedCallerDump"}},
wantSamples: []*profile.Sample{
{Value: []int64{50, 50 * period}, Location: []*profile.Location{{ID: 1}}},
@@ -1712,6 +1719,7 @@
4, 0, 70, inlinedCallerStack[0],
5, 0, 80, inlinedCallerStack[0], inlinedCallerStack[1],
},
+ count: 3,
wantLocs: [][]string{{"runtime/pprof.inlinedCalleeDump", "runtime/pprof.inlinedCallerDump"}},
wantSamples: []*profile.Sample{
{Value: []int64{70, 70 * period}, Location: []*profile.Location{{ID: 1}}},
@@ -1724,6 +1732,7 @@
3, 0, 500, // hz = 500. Must match the period.
4, 0, 70, inlinedCallerStack[0],
},
+ count: 2,
wantLocs: [][]string{{"runtime/pprof.inlinedCalleeDump", "runtime/pprof.inlinedCallerDump"}},
wantSamples: []*profile.Sample{
{Value: []int64{70, 70 * period}, Location: []*profile.Location{{ID: 1}}},
@@ -1739,6 +1748,7 @@
// from getting merged into above.
5, 0, 80, inlinedCallerStack[1], inlinedCallerStack[0],
},
+ count: 3,
wantLocs: [][]string{
{"runtime/pprof.inlinedCalleeDump", "runtime/pprof.inlinedCallerDump"},
{"runtime/pprof.inlinedCallerDump"},
@@ -1751,7 +1761,7 @@
for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
- p, err := translateCPUProfile(tc.input)
+ p, err := translateCPUProfile(tc.input, tc.count)
if err != nil {
t.Fatalf("translating profile: %v", err)
}
diff --git a/src/runtime/pprof/proto.go b/src/runtime/pprof/proto.go
index 54e7a80..073a076 100644
--- a/src/runtime/pprof/proto.go
+++ b/src/runtime/pprof/proto.go
@@ -266,8 +266,9 @@
}
// addCPUData adds the CPU profiling data to the profile.
-// The data must be a whole number of records,
-// as delivered by the runtime.
+//
+// The data must be a whole number of records, as delivered by the runtime.
+// len(tags) must be equal to the number of records in data.
func (b *profileBuilder) addCPUData(data []uint64, tags []unsafe.Pointer) error {
if !b.havePeriod {
// first record is period
@@ -282,6 +283,9 @@
b.period = 1e9 / int64(data[2])
b.havePeriod = true
data = data[3:]
+ // Consume tag slot. Note that there isn't a meaningful tag
+ // value for this record.
+ tags = tags[1:]
}
// Parse CPU samples from the profile.
@@ -306,14 +310,14 @@
if data[0] < 3 || tags != nil && len(tags) < 1 {
return fmt.Errorf("malformed profile")
}
+ if len(tags) < 1 {
+ return fmt.Errorf("mismatched profile records and tags")
+ }
count := data[2]
stk := data[3:data[0]]
data = data[data[0]:]
- var tag unsafe.Pointer
- if tags != nil {
- tag = tags[0]
- tags = tags[1:]
- }
+ tag := tags[0]
+ tags = tags[1:]
if count == 0 && len(stk) == 1 {
// overflow record
@@ -327,6 +331,10 @@
}
b.m.lookup(stk, tag).count += int64(count)
}
+
+ if len(tags) != 0 {
+ return fmt.Errorf("mismatched profile records and tags")
+ }
return nil
}
diff --git a/src/runtime/pprof/proto_test.go b/src/runtime/pprof/proto_test.go
index 4a9749a..ea0ed9a 100644
--- a/src/runtime/pprof/proto_test.go
+++ b/src/runtime/pprof/proto_test.go
@@ -17,16 +17,20 @@
"runtime"
"strings"
"testing"
+ "unsafe"
)
// translateCPUProfile parses binary CPU profiling stack trace data
// generated by runtime.CPUProfile() into a profile struct.
// This is only used for testing. Real conversions stream the
// data into the profileBuilder as it becomes available.
-func translateCPUProfile(data []uint64) (*profile.Profile, error) {
+//
+// count is the number of records in data.
+func translateCPUProfile(data []uint64, count int) (*profile.Profile, error) {
var buf bytes.Buffer
b := newProfileBuilder(&buf)
- if err := b.addCPUData(data, nil); err != nil {
+ tags := make([]unsafe.Pointer, count)
+ if err := b.addCPUData(data, tags); err != nil {
return nil, err
}
b.build()
@@ -46,7 +50,7 @@
var buf bytes.Buffer
b := []uint64{3, 0, 500} // empty profile at 500 Hz (2ms sample period)
- p, err := translateCPUProfile(b)
+ p, err := translateCPUProfile(b, 1)
if err != nil {
t.Fatalf("translateCPUProfile: %v", err)
}
@@ -120,7 +124,7 @@
5, 0, 40, uint64(addr2 + 1), uint64(addr2 + 2), // 40 samples in addr2
5, 0, 10, uint64(addr1 + 1), uint64(addr1 + 2), // 10 samples in addr1
}
- p, err := translateCPUProfile(b)
+ p, err := translateCPUProfile(b, 4)
if err != nil {
t.Fatalf("translating profile: %v", err)
}
@@ -429,7 +433,7 @@
3, 0, 500, // hz = 500
3, 0, 10, // 10 samples with an empty stack trace
}
- _, err := translateCPUProfile(b)
+ _, err := translateCPUProfile(b, 2)
if err != nil {
t.Fatalf("translating profile: %v", err)
}
To view, visit change 369741. To unsubscribe, or for help writing mail filters, visit settings.
Reply all
Reply to author
Forward
0 new messages