From ce528719487080f4797e1ed45671bd509cbc2d10 Mon Sep 17 00:00:00 2001 From: Michael Pratt Date: Tue, 7 Apr 2020 15:17:25 -0400 Subject: [PATCH] runtime/pprof: clarify recursive inline heuristic Following CL 226818, the compiler will allow inlining a single cycle in an inline chain. Immediately-recursive functions are still disallowed, which is what this heuristic refers to. Add a regression test for this case. Note that in addition to this check, if the compiler were to inline multiple cycles via a loop (i.e., rather than appending duplicate code), much more work would be required here to handle a single address appearing in multiple different inline frames. Updates #29737 Change-Id: I88de15cfbeabb9c04381e1c12cc36778623132a5 Reviewed-on: https://go-review.googlesource.com/c/go/+/227346 Run-TryBot: Michael Pratt TryBot-Result: Gobot Gobot Reviewed-by: Dan Scales Reviewed-by: Hyang-Ah Hana Kim --- src/runtime/pprof/pprof_test.go | 75 +++++++++++++++++++++++++++++---- src/runtime/pprof/proto.go | 2 +- 2 files changed, 68 insertions(+), 9 deletions(-) diff --git a/src/runtime/pprof/pprof_test.go b/src/runtime/pprof/pprof_test.go index 52b51ee60d..c3e253eb41 100644 --- a/src/runtime/pprof/pprof_test.go +++ b/src/runtime/pprof/pprof_test.go @@ -227,6 +227,25 @@ func recursionCallee(n, x int) int { return y * recursionCallee(n-1, x) } +func recursionChainTop(x int, pcs []uintptr) { + if x < 0 { + return + } + recursionChainMiddle(x, pcs) +} + +func recursionChainMiddle(x int, pcs []uintptr) { + recursionChainBottom(x, pcs) +} + +func recursionChainBottom(x int, pcs []uintptr) { + // This will be called each time, we only care about the last. We + // can't make this conditional or this function won't be inlined. + dumpCallers(pcs) + + recursionChainTop(x - 1, pcs) +} + func parseProfile(t *testing.T, valBytes []byte, f func(uintptr, []*profile.Location, map[string][]string)) *profile.Profile { p, err := profile.Parse(bytes.NewReader(valBytes)) if err != nil { @@ -1158,11 +1177,12 @@ func TestTracebackAll(t *testing.T) { } } -// TestTryAdd tests the cases that's hard to test with real program execution. -// For example, the current go compilers may not inline functions involved in recursion -// but that may not be true in the future compilers. This tests such cases by -// using fake call sequences and forcing the profile build utilizing -// translateCPUProfile defined in proto_test.go +// TestTryAdd tests the cases that are hard to test with real program execution. +// +// For example, the current go compilers may not always inline functions +// involved in recursion but that may not be true in the future compilers. This +// tests such cases by using fake call sequences and forcing the profile build +// utilizing translateCPUProfile defined in proto_test.go func TestTryAdd(t *testing.T) { if _, found := findInlinedCall(inlinedCallerDump, 4<<10); !found { t.Skip("Can't determine whether anything was inlined into inlinedCallerDump.") @@ -1177,6 +1197,23 @@ func TestTryAdd(t *testing.T) { inlinedCallerStack[i] = uint64(pcs[i]) } + if _, found := findInlinedCall(recursionChainBottom, 4<<10); !found { + t.Skip("Can't determine whether anything was inlined into recursionChainBottom.") + } + + // recursionChainTop + // recursionChainMiddle + // recursionChainBottom + // recursionChainTop + // recursionChainMiddle + // recursionChainBottom + pcs = make([]uintptr, 6) + recursionChainTop(1, pcs) + recursionStack := make([]uint64, len(pcs)) + for i := range pcs { + recursionStack[i] = uint64(pcs[i]) + } + period := int64(2000 * 1000) // 1/500*1e9 nanosec. testCases := []struct { @@ -1225,14 +1262,14 @@ func TestTryAdd(t *testing.T) { {Value: []int64{4242, 4242 * period}, Location: []*profile.Location{{ID: 1}}}, }, }, { - // If a function is called recursively then it must not be - // inlined in the caller. + // If a function is directly called recursively then it must + // not be inlined in the caller. // // N.B. We're generating an impossible profile here, with a // recursive inlineCalleeDump call. This is simulating a non-Go // function that looks like an inlined Go function other than // its recursive property. See pcDeck.tryAdd. - name: "recursive_func_is_not_inlined", + name: "directly_recursive_func_is_not_inlined", input: []uint64{ 3, 0, 500, // hz = 500. Must match the period. 5, 0, 30, inlinedCallerStack[0], inlinedCallerStack[0], @@ -1245,6 +1282,28 @@ func TestTryAdd(t *testing.T) { {Value: []int64{30, 30 * period}, Location: []*profile.Location{{ID: 1}, {ID: 1}, {ID: 2}}}, {Value: []int64{40, 40 * period}, Location: []*profile.Location{{ID: 1}, {ID: 2}}}, }, + }, { + name: "recursion_chain_inline", + input: []uint64{ + 3, 0, 500, // hz = 500. Must match the period. + 9, 0, 10, recursionStack[0], recursionStack[1], recursionStack[2], recursionStack[3], recursionStack[4], recursionStack[5], + }, + wantLocs: [][]string{ + {"runtime/pprof.recursionChainBottom"}, + { + "runtime/pprof.recursionChainMiddle", + "runtime/pprof.recursionChainTop", + "runtime/pprof.recursionChainBottom", + }, + { + "runtime/pprof.recursionChainMiddle", + "runtime/pprof.recursionChainTop", + "runtime/pprof.TestTryAdd", // inlined into the test. + }, + }, + wantSamples: []*profile.Sample{ + {Value: []int64{10, 10 * period}, Location: []*profile.Location{{ID: 1}, {ID: 2}, {ID: 3}}}, + }, }, { name: "truncated_stack_trace_later", input: []uint64{ diff --git a/src/runtime/pprof/proto.go b/src/runtime/pprof/proto.go index bb63153a70..f3d8ac38bf 100644 --- a/src/runtime/pprof/proto.go +++ b/src/runtime/pprof/proto.go @@ -461,7 +461,7 @@ func (b *profileBuilder) appendLocsForStack(locs []uint64, stk []uintptr) (newLo // have the following properties: // Frame's Func is nil (note: also true for non-Go functions), and // Frame's Entry matches its entry function frame's Entry (note: could also be true for recursive calls and non-Go functions), and -// Frame's Name does not match its entry function frame's name (note: inlined functions cannot be recursive). +// Frame's Name does not match its entry function frame's name (note: inlined functions cannot be directly recursive). // // As reading and processing the pcs in a stack trace one by one (from leaf to the root), // we use pcDeck to temporarily hold the observed pcs and their expanded frames -- 2.50.0