From: Michael Pratt Date: Wed, 2 Nov 2022 15:11:03 +0000 (-0400) Subject: cmd/compile/internal/pgo: match on call line offsets X-Git-Tag: go1.20rc1~412 X-Git-Url: http://www.git.cypherpunks.su/?a=commitdiff_plain;h=bdd1e283a914d4d161589adce4f2ad79767d029d;p=gostls13.git cmd/compile/internal/pgo: match on call line offsets Rather than matching calls to edges in the profile based directly on line number in the source file, use the line offset from the start of the function. This makes matching robust to changes in the source file above the function containing the call. The start line in the profile comes from Function.start_line, which is included in Go pprof output since CL 438255. Currently it is an error if no samples set start_line to help users detect profiles missing this information. In the future, we should fallback to using absolute lines, which is better than nothing. For #55022. Change-Id: Ie621950cfee1fef8fb200907a2a3f1ded41d04fa Reviewed-on: https://go-review.googlesource.com/c/go/+/447315 Reviewed-by: Cherry Mui TryBot-Result: Gopher Robot Run-TryBot: Michael Pratt Auto-Submit: Michael Pratt --- diff --git a/src/cmd/compile/internal/inline/inl.go b/src/cmd/compile/internal/inline/inl.go index c7f56d360d..2260a90d50 100644 --- a/src/cmd/compile/internal/inline/inl.go +++ b/src/cmd/compile/internal/inline/inl.go @@ -112,7 +112,7 @@ func pgoInlinePrologue(p *pgo.Profile) { if e.Weight != 0 { edgeweightpercent := pgo.WeightInPercentage(e.Weight, p.TotalEdgeWeight) if edgeweightpercent > inlineHotCallSiteThresholdPercent { - csi := pgo.CallSiteInfo{Line: e.CallSite, Caller: n.AST} + csi := pgo.CallSiteInfo{LineOffset: e.CallSiteOffset, Caller: n.AST} if _, ok := candHotEdgeMap[csi]; !ok { candHotEdgeMap[csi] = struct{}{} } @@ -150,7 +150,7 @@ func computeThresholdFromCDF(p *pgo.Profile) (float64, []pgo.NodeMapKey) { if ni.CalleeName != nj.CalleeName { return ni.CalleeName < nj.CalleeName } - return ni.CallSite < nj.CallSite + return ni.CallSiteOffset < nj.CallSiteOffset }) cum := int64(0) for i, n := range nodes { @@ -488,8 +488,8 @@ func (v *hairyVisitor) doNode(n ir.Node) bool { // Determine if the callee edge is for an inlinable hot callee or not. if v.profile != nil && v.curFunc != nil { if fn := inlCallee(n.X, v.profile); fn != nil && typecheck.HaveInlineBody(fn) { - line := int(base.Ctxt.InnermostPos(n.Pos()).RelLine()) - csi := pgo.CallSiteInfo{Line: line, Caller: v.curFunc} + lineOffset := pgo.NodeLineOffset(n, fn) + csi := pgo.CallSiteInfo{LineOffset: lineOffset, Caller: v.curFunc} if _, o := candHotEdgeMap[csi]; o { if base.Debug.PGOInline > 0 { fmt.Printf("hot-callsite identified at line=%v for func=%v\n", ir.Line(n), ir.PkgFuncName(v.curFunc)) @@ -919,8 +919,8 @@ func mkinlcall(n *ir.CallExpr, fn *ir.Func, maxCost int32, inlCalls *[]*ir.Inlin } if fn.Inl.Cost > maxCost { // If the callsite is hot and it is under the inlineHotMaxBudget budget, then try to inline it, or else bail. - line := int(base.Ctxt.InnermostPos(n.Pos()).RelLine()) - csi := pgo.CallSiteInfo{Line: line, Caller: ir.CurFunc} + lineOffset := pgo.NodeLineOffset(n, fn) + csi := pgo.CallSiteInfo{LineOffset: lineOffset, Caller: ir.CurFunc} if _, ok := candHotEdgeMap[csi]; ok { if fn.Inl.Cost > inlineHotMaxBudget { if logopt.Enabled() { @@ -1084,8 +1084,7 @@ func mkinlcall(n *ir.CallExpr, fn *ir.Func, maxCost int32, inlCalls *[]*ir.Inlin } if base.Debug.PGOInline > 0 { - line := int(base.Ctxt.InnermostPos(n.Pos()).RelLine()) - csi := pgo.CallSiteInfo{Line: line, Caller: ir.CurFunc} + csi := pgo.CallSiteInfo{LineOffset: pgo.NodeLineOffset(n, fn), Caller: ir.CurFunc} if _, ok := inlinedCallSites[csi]; !ok { inlinedCallSites[csi] = struct{}{} } diff --git a/src/cmd/compile/internal/pgo/irgraph.go b/src/cmd/compile/internal/pgo/irgraph.go index f0932b51ea..528c27645d 100644 --- a/src/cmd/compile/internal/pgo/irgraph.go +++ b/src/cmd/compile/internal/pgo/irgraph.go @@ -6,7 +6,8 @@ // A note on line numbers: when working with line numbers, we always use the // binary-visible relative line number. i.e., the line number as adjusted by -// //line directives (ctxt.InnermostPos(ir.Node.Pos()).RelLine()). +// //line directives (ctxt.InnermostPos(ir.Node.Pos()).RelLine()). Use +// NodeLineOffset to compute line offsets. // // If you are thinking, "wait, doesn't that just make things more complex than // using the real line number?", then you are 100% correct. Unfortunately, @@ -80,17 +81,17 @@ type IREdgeMap map[*IRNode][]*IREdge // weight, callsite, and line number information. type IREdge struct { // Source and destination of the edge in IRNode. - Src, Dst *IRNode - Weight int64 - CallSite int + Src, Dst *IRNode + Weight int64 + CallSiteOffset int // Line offset from function start line. } // NodeMapKey represents a hash key to identify unique call-edges in profile // and in IR. Used for deduplication of call edges found in profile. type NodeMapKey struct { - CallerName string - CalleeName string - CallSite int + CallerName string + CalleeName string + CallSiteOffset int // Line offset from function start line. } // Weights capture both node weight and edge weight. @@ -102,17 +103,14 @@ type Weights struct { // CallSiteInfo captures call-site information and its caller/callee. type CallSiteInfo struct { - Line int - Caller *ir.Func - Callee *ir.Func + LineOffset int // Line offset from function start line. + Caller *ir.Func + Callee *ir.Func } // Profile contains the processed PGO profile and weighted call graph used for // PGO optimizations. type Profile struct { - // Original profile-graph. - ProfileGraph *Graph - // Aggregated NodeWeights and EdgeWeights across the profile. This // helps us determine the percentage threshold for hot/cold // partitioning. @@ -148,15 +146,14 @@ func New(profileFile string) *Profile { }) p := &Profile{ - NodeMap: make(map[NodeMapKey]*Weights), - ProfileGraph: g, + NodeMap: make(map[NodeMapKey]*Weights), WeightedCG: &IRGraph{ IRNodes: make(map[string]*IRNode), }, } // Build the node map and totals from the profile graph. - p.preprocessProfileGraph() + p.processprofileGraph(g) // Create package-level call graph with weights from profile and IR. p.initializeIRGraph() @@ -164,31 +161,34 @@ func New(profileFile string) *Profile { return p } -// preprocessProfileGraph builds various maps from the profile-graph. +// processprofileGraph builds various maps from the profile-graph. // // It initializes NodeMap and Total{Node,Edge}Weight based on the name and // callsite to compute node and edge weights which will be used later on to // create edges for WeightedCG. -func (p *Profile) preprocessProfileGraph() { +func (p *Profile) processprofileGraph(g *Graph) { nFlat := make(map[string]int64) nCum := make(map[string]int64) + seenStartLine := false // Accummulate weights for the same node. - for _, n := range p.ProfileGraph.Nodes { + for _, n := range g.Nodes { canonicalName := n.Info.Name nFlat[canonicalName] += n.FlatValue() nCum[canonicalName] += n.CumValue() } - // Process ProfileGraph and build various node and edge maps which will + // Process graph and build various node and edge maps which will // be consumed by AST walk. - for _, n := range p.ProfileGraph.Nodes { + for _, n := range g.Nodes { + seenStartLine = seenStartLine || n.Info.StartLine != 0 + p.TotalNodeWeight += n.FlatValue() canonicalName := n.Info.Name - // Create the key to the NodeMapKey. + // Create the key to the nodeMapKey. nodeinfo := NodeMapKey{ - CallerName: canonicalName, - CallSite: n.Info.Lineno, + CallerName: canonicalName, + CallSiteOffset: n.Info.Lineno - n.Info.StartLine, } for _, e := range n.Out { @@ -205,6 +205,13 @@ func (p *Profile) preprocessProfileGraph() { } } } + + if !seenStartLine { + // TODO(prattic): If Function.start_line is missing we could + // fall back to using absolute line numbers, which is better + // than nothing. + log.Fatal("PGO profile missing Function.start_line data") + } } // initializeIRGraph builds the IRGraph by visting all the ir.Func in decl list @@ -240,9 +247,9 @@ func (p *Profile) VisitIR(fn *ir.Func, recursive bool) { } // Create the key for the NodeMapKey. nodeinfo := NodeMapKey{ - CallerName: name, - CalleeName: "", - CallSite: -1, + CallerName: name, + CalleeName: "", + CallSiteOffset: 0, } // If the node exists, then update its node weight. if weights, ok := p.NodeMap[nodeinfo]; ok { @@ -254,9 +261,17 @@ func (p *Profile) VisitIR(fn *ir.Func, recursive bool) { p.createIRGraphEdge(fn, g.IRNodes[name], name) } +// NodeLineOffset returns the line offset of n in fn. +func NodeLineOffset(n ir.Node, fn *ir.Func) int { + // See "A note on line numbers" at the top of the file. + line := int(base.Ctxt.InnermostPos(n.Pos()).RelLine()) + startLine := int(base.Ctxt.InnermostPos(fn.Pos()).RelLine()) + return line - startLine +} + // addIREdge adds an edge between caller and new node that points to `callee` // based on the profile-graph and NodeMap. -func (p *Profile) addIREdge(caller *IRNode, callee *ir.Func, n *ir.Node, callername string, line int) { +func (p *Profile) addIREdge(caller *IRNode, callername string, call ir.Node, callee *ir.Func) { g := p.WeightedCG // Create an IRNode for the callee. @@ -266,18 +281,18 @@ func (p *Profile) addIREdge(caller *IRNode, callee *ir.Func, n *ir.Node, callern // Create key for NodeMapKey. nodeinfo := NodeMapKey{ - CallerName: callername, - CalleeName: calleename, - CallSite: line, + CallerName: callername, + CalleeName: calleename, + CallSiteOffset: NodeLineOffset(call, caller.AST), } // Create the callee node with node weight. if g.IRNodes[calleename] == nil { g.IRNodes[calleename] = calleenode nodeinfo2 := NodeMapKey{ - CallerName: calleename, - CalleeName: "", - CallSite: -1, + CallerName: calleename, + CalleeName: "", + CallSiteOffset: 0, } if weights, ok := p.NodeMap[nodeinfo2]; ok { g.IRNodes[calleename].Flat = weights.NFlat @@ -290,20 +305,20 @@ func (p *Profile) addIREdge(caller *IRNode, callee *ir.Func, n *ir.Node, callern caller.Cum = weights.NCum // Add edge in the IRGraph from caller to callee. - info := &IREdge{Src: caller, Dst: g.IRNodes[calleename], Weight: weights.EWeight, CallSite: line} + info := &IREdge{Src: caller, Dst: g.IRNodes[calleename], Weight: weights.EWeight, CallSiteOffset: nodeinfo.CallSiteOffset} g.OutEdges[caller] = append(g.OutEdges[caller], info) g.InEdges[g.IRNodes[calleename]] = append(g.InEdges[g.IRNodes[calleename]], info) } else { nodeinfo.CalleeName = "" - nodeinfo.CallSite = -1 + nodeinfo.CallSiteOffset = 0 if weights, ok := p.NodeMap[nodeinfo]; ok { caller.Flat = weights.NFlat caller.Cum = weights.NCum - info := &IREdge{Src: caller, Dst: g.IRNodes[calleename], Weight: 0, CallSite: line} + info := &IREdge{Src: caller, Dst: g.IRNodes[calleename], Weight: 0, CallSiteOffset: nodeinfo.CallSiteOffset} g.OutEdges[caller] = append(g.OutEdges[caller], info) g.InEdges[g.IRNodes[calleename]] = append(g.InEdges[g.IRNodes[calleename]], info) } else { - info := &IREdge{Src: caller, Dst: g.IRNodes[calleename], Weight: 0, CallSite: line} + info := &IREdge{Src: caller, Dst: g.IRNodes[calleename], Weight: 0, CallSiteOffset: nodeinfo.CallSiteOffset} g.OutEdges[caller] = append(g.OutEdges[caller], info) g.InEdges[g.IRNodes[calleename]] = append(g.InEdges[g.IRNodes[calleename]], info) } @@ -319,18 +334,16 @@ func (p *Profile) createIRGraphEdge(fn *ir.Func, callernode *IRNode, name string ir.DoChildren(n, doNode) case ir.OCALLFUNC: call := n.(*ir.CallExpr) - line := int(base.Ctxt.InnermostPos(n.Pos()).RelLine()) // Find the callee function from the call site and add the edge. - f := inlCallee(call.X) - if f != nil { - p.addIREdge(callernode, f, &n, name, line) + callee := inlCallee(call.X) + if callee != nil { + p.addIREdge(callernode, name, n, callee) } case ir.OCALLMETH: call := n.(*ir.CallExpr) // Find the callee method from the call site and add the edge. - fn2 := ir.MethodExprName(call.X).Func - line := int(base.Ctxt.InnermostPos(n.Pos()).RelLine()) - p.addIREdge(callernode, fn2, &n, name, line) + callee := ir.MethodExprName(call.X).Func + p.addIREdge(callernode, name, n, callee) } return false } @@ -419,18 +432,17 @@ func (p *Profile) RedirectEdges(cur *IRNode, inlinedCallSites map[CallSiteInfo]s g := p.WeightedCG for i, outEdge := range g.OutEdges[cur] { - if _, found := inlinedCallSites[CallSiteInfo{Line: outEdge.CallSite, Caller: cur.AST}]; !found { + if _, found := inlinedCallSites[CallSiteInfo{LineOffset: outEdge.CallSiteOffset, Caller: cur.AST}]; !found { for _, InEdge := range g.InEdges[cur] { - if _, ok := inlinedCallSites[CallSiteInfo{Line: InEdge.CallSite, Caller: InEdge.Src.AST}]; ok { + if _, ok := inlinedCallSites[CallSiteInfo{LineOffset: InEdge.CallSiteOffset, Caller: InEdge.Src.AST}]; ok { weight := g.calculateWeight(InEdge.Src, cur) g.redirectEdge(InEdge.Src, cur, outEdge, weight, i) } } } else { - g.remove(cur, i, outEdge.Dst.AST.Nname) + g.remove(cur, i) } } - g.removeall(cur) } // redirectEdges deletes the cur node out-edges and redirect them so now these @@ -449,28 +461,19 @@ func (g *IRGraph) redirectEdge(parent *IRNode, cur *IRNode, outEdge *IREdge, wei outEdge.Src = parent outEdge.Weight = weight * outEdge.Weight g.OutEdges[parent] = append(g.OutEdges[parent], outEdge) - g.remove(cur, idx, outEdge.Dst.AST.Nname) + g.remove(cur, idx) } // remove deletes the cur-node's out-edges at index idx. -func (g *IRGraph) remove(cur *IRNode, idx int, name *ir.Name) { +func (g *IRGraph) remove(cur *IRNode, i int) { if len(g.OutEdges[cur]) >= 2 { - g.OutEdges[cur][idx] = &IREdge{CallSite: -1} + g.OutEdges[cur][i] = g.OutEdges[cur][len(g.OutEdges[cur])-1] + g.OutEdges[cur] = g.OutEdges[cur][:len(g.OutEdges[cur])-1] } else { delete(g.OutEdges, cur) } } -// removeall deletes all cur-node's out-edges that marked to be removed . -func (g *IRGraph) removeall(cur *IRNode) { - for i := len(g.OutEdges[cur]) - 1; i >= 0; i-- { - if g.OutEdges[cur][i].CallSite == -1 { - g.OutEdges[cur][i] = g.OutEdges[cur][len(g.OutEdges[cur])-1] - g.OutEdges[cur] = g.OutEdges[cur][:len(g.OutEdges[cur])-1] - } - } -} - // calculateWeight calculates the weight of the new redirected edge. func (g *IRGraph) calculateWeight(parent *IRNode, cur *IRNode) int64 { sum := int64(0) diff --git a/src/cmd/compile/internal/test/pgo_inl_test.go b/src/cmd/compile/internal/test/pgo_inl_test.go index cbf14415c7..d0737f76a1 100644 --- a/src/cmd/compile/internal/test/pgo_inl_test.go +++ b/src/cmd/compile/internal/test/pgo_inl_test.go @@ -8,6 +8,7 @@ import ( "bufio" "fmt" "internal/testenv" + "io" "os" "os/exec" "path/filepath" @@ -16,12 +17,20 @@ import ( "testing" ) -// TestPGOIntendedInlining tests that specific functions are inlined. -func TestPGOIntendedInlining(t *testing.T) { +// testPGOIntendedInlining tests that specific functions are inlined. +func testPGOIntendedInlining(t *testing.T, dir string) { testenv.MustHaveGoRun(t) t.Parallel() - const pkg = "cmd/compile/internal/test/testdata/pgo/inline" + const pkg = "example.com/pgo/inline" + + // Add a go.mod so we have a consistent symbol names in this temp dir. + goMod := fmt.Sprintf(`module %s +go 1.19 +`, pkg) + if err := os.WriteFile(filepath.Join(dir, "go.mod"), []byte(goMod), 0644); err != nil { + t.Fatalf("error writing go.mod: %v", err) + } want := []string{ "(*BS).NS", @@ -58,14 +67,12 @@ func TestPGOIntendedInlining(t *testing.T) { expectedNotInlinedList[fullName] = struct{}{} } - // go test -c -o /tmp/test.exe -cpuprofile testdata/pgo/inline/inline_hot.pprof cmd/compile/internal/test/testdata/pgo/inline - curdir, err := os.Getwd() - if err != nil { - t.Fatalf("error getting wd: %v", err) - } - gcflag := fmt.Sprintf("-gcflags=-m -m -pgoprofile %s/testdata/pgo/inline/inline_hot.pprof", curdir) - out := filepath.Join(t.TempDir(), "test.exe") - cmd := testenv.CleanCmdEnv(exec.Command(testenv.GoToolPath(t), "test", "-c", "-o", out, gcflag, pkg)) + // go test -c -o /tmp/test.exe -cpuprofile inline_hot.pprof + pprof := filepath.Join(dir, "inline_hot.pprof") + gcflag := fmt.Sprintf("-gcflags=-m -m -pgoprofile %s", pprof) + out := filepath.Join(dir, "test.exe") + cmd := testenv.CleanCmdEnv(exec.Command(testenv.GoToolPath(t), "test", "-c", "-o", out, gcflag, ".")) + cmd.Dir = dir pr, pw, err := os.Pipe() if err != nil { @@ -136,3 +143,89 @@ func TestPGOIntendedInlining(t *testing.T) { t.Errorf("%s was expected not inlined", fullName) } } + +// TestPGOIntendedInlining tests that specific functions are inlined when PGO +// is applied to the exact source that was profiled. +func TestPGOIntendedInlining(t *testing.T) { + wd, err := os.Getwd() + if err != nil { + t.Fatalf("error getting wd: %v", err) + } + srcDir := filepath.Join(wd, "testdata/pgo/inline") + + // Copy the module to a scratch location so we can add a go.mod. + dir := t.TempDir() + + for _, file := range []string{"inline_hot.go", "inline_hot_test.go", "inline_hot.pprof"} { + if err := copyFile(filepath.Join(dir, file), filepath.Join(srcDir, file)); err != nil { + t.Fatalf("error copying %s: %v", file, err) + } + } + + testPGOIntendedInlining(t, dir) +} + +// TestPGOIntendedInlining tests that specific functions are inlined when PGO +// is applied to the modified source. +func TestPGOIntendedInliningShiftedLines(t *testing.T) { + wd, err := os.Getwd() + if err != nil { + t.Fatalf("error getting wd: %v", err) + } + srcDir := filepath.Join(wd, "testdata/pgo/inline") + + // Copy the module to a scratch location so we can modify the source. + dir := t.TempDir() + + // Copy most of the files unmodified. + for _, file := range []string{"inline_hot_test.go", "inline_hot.pprof"} { + if err := copyFile(filepath.Join(dir, file), filepath.Join(srcDir, file)); err != nil { + t.Fatalf("error copying %s : %v", file, err) + } + } + + // Add some comments to the top of inline_hot.go. This adjusts the line + // numbers of all of the functions without changing the semantics. + src, err := os.Open(filepath.Join(srcDir, "inline_hot.go")) + if err != nil { + t.Fatalf("error opening src inline_hot.go: %v", err) + } + defer src.Close() + + dst, err := os.Create(filepath.Join(dir, "inline_hot.go")) + if err != nil { + t.Fatalf("error creating dst inline_hot.go: %v", err) + } + defer dst.Close() + + if _, err := io.WriteString(dst, `// Autogenerated +// Lines +`); err != nil { + t.Fatalf("error writing comments to dst: %v", err) + } + + if _, err := io.Copy(dst, src); err != nil { + t.Fatalf("error copying inline_hot.go: %v", err) + } + + dst.Close() + + testPGOIntendedInlining(t, dir) +} + +func copyFile(dst, src string) error { + s, err := os.Open(src) + if err != nil { + return err + } + defer s.Close() + + d, err := os.Create(dst) + if err != nil { + return err + } + defer d.Close() + + _, err = io.Copy(d, s) + return err +} diff --git a/src/cmd/compile/internal/test/testdata/pgo/inline/inline_hot.go b/src/cmd/compile/internal/test/testdata/pgo/inline/inline_hot.go index c1d2a53983..9a462fdfd9 100644 --- a/src/cmd/compile/internal/test/testdata/pgo/inline/inline_hot.go +++ b/src/cmd/compile/internal/test/testdata/pgo/inline/inline_hot.go @@ -2,7 +2,11 @@ // Use of this source code is governed by a BSD-style // license that can be found in the LICENSE file. -// WARNING: Please avoid updating this file. If this file needs to be updated, then a new inline_hot.pprof file should be generated via "go test -bench=. -cpuprofile testdata/pgo/inline/inline_hot.pprof cmd/compile/internal/test/testdata/pgo/inline". +// WARNING: Please avoid updating this file. If this file needs to be updated, +// then a new inline_hot.pprof file should be generated: +// +// $ cd $GOROOT/src/cmd/compile/internal/test/testdata/pgo/inline/ +// $ go test -bench=. -cpuprofile ./inline_hot.pprof package main import ( diff --git a/src/cmd/compile/internal/test/testdata/pgo/inline/inline_hot.pprof b/src/cmd/compile/internal/test/testdata/pgo/inline/inline_hot.pprof index 45ccb6132b..1b55ed1233 100644 Binary files a/src/cmd/compile/internal/test/testdata/pgo/inline/inline_hot.pprof and b/src/cmd/compile/internal/test/testdata/pgo/inline/inline_hot.pprof differ diff --git a/src/cmd/compile/internal/test/testdata/pgo/inline/inline_hot_test.go b/src/cmd/compile/internal/test/testdata/pgo/inline/inline_hot_test.go index 024d340785..2725c57053 100644 --- a/src/cmd/compile/internal/test/testdata/pgo/inline/inline_hot_test.go +++ b/src/cmd/compile/internal/test/testdata/pgo/inline/inline_hot_test.go @@ -2,7 +2,11 @@ // Use of this source code is governed by a BSD-style // license that can be found in the LICENSE file. -// WARNING: Please avoid updating this file. If this file needs to be updated, then a new inline_hot.pprof file should be generated via "go test -bench=. -cpuprofile testdata/pgo/inline/inline_hot.pprof cmd/compile/internal/test/testdata/pgo/inline". +// WARNING: Please avoid updating this file. If this file needs to be updated, +// then a new inline_hot.pprof file should be generated: +// +// $ cd $GOROOT/src/cmd/compile/internal/test/testdata/pgo/inline/ +// $ go test -bench=. -cpuprofile ./inline_hot.pprof package main import "testing"