languages/go
languages/go
1
0
Fork 0
mirror of https://github.com/golang/go synced 2024-09-18 15:32:18 +00:00
Code Issues Packages Projects Releases Wiki Activity

cmd/compile/internal/pgo: remove ConvertLine2Int

Browse source 
Parts of package pgo fetch the line number of a node by parsing the
number out of the string returned from ir.Line().

This is indirect and inefficient, so it should be replaced with a more
direct lookup. It is also potentially buggy: ir.Line uses
ctxt.OutermostPos, i.e., the line number where an inlined node in
inlined. We want ctxt.InnermostPos, because that is the line number used
in pprof profiles that we are matching against (See comments on
OutermostPos and InnermostPos).

I'm not sure whether this was an active, as we use ir.Line before and
during inlining. I think we could see CALL nodes with OutermostPos !=
InnermostPos during midstack inlining, but I am not sure. Regardless,
explicitly using the desired position is clearer.

For #55022.

Change-Id: Ic640761c9e1d01cacbf91f3aaeaf284ad7e38dbd
Reviewed-on: https://go-review.googlesource.com/c/go/+/446302
Reviewed-by: Cherry Mui <cherryyz@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Michael Pratt <mpratt@google.com>
Auto-Submit: Michael Pratt <mpratt@google.com>
This commit is contained in:
Michael Pratt 2022-10-28 13:52:43 -04:00 committed by Gopher Robot
parent e8ec68edfa
commit ec0b540293
2 changed files with 45 additions and 18 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

14
src/cmd/compile/internal/inline/inl.go
View file

@ -441,10 +441,10 @@ func (v *hairyVisitor) doNode(n ir.Node) bool {
// Determine if the callee edge is a for hot callee or not.
if base.Flag.PgoProfile != "" && pgo.WeightedCG != nil && v.curFunc != nil {
if fn := inlCallee(n.X); fn != nil && typecheck.HaveInlineBody(fn) {
ln := pgo.ConvertLine2Int(ir.Line(n))
csi := pgo.CallSiteInfo{Line: ln, Caller: v.curFunc, Callee: fn}
line := int(base.Ctxt.InnermostPos(n.Pos()).RelLine())
csi := pgo.CallSiteInfo{Line: line, Caller: v.curFunc, Callee: fn}
if _, o := candHotEdgeMap[csi]; o {
pgo.ListOfHotCallSites[pgo.CallSiteInfo{Line: ln, Caller: v.curFunc}] = struct{}{}
pgo.ListOfHotCallSites[pgo.CallSiteInfo{Line: line, Caller: v.curFunc}] = struct{}{}
if base.Debug.PGOInline > 0 {
fmt.Printf("hot-callsite identified at line=%v for func=%v\n", ir.Line(n), ir.PkgFuncName(v.curFunc))
}
@ -873,8 +873,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.
ln := pgo.ConvertLine2Int(ir.Line(n))
csi := pgo.CallSiteInfo{Line: ln, Caller: ir.CurFunc}
line := int(base.Ctxt.InnermostPos(n.Pos()).RelLine())
csi := pgo.CallSiteInfo{Line: line, Caller: ir.CurFunc}
if _, ok := pgo.ListOfHotCallSites[csi]; ok {
if fn.Inl.Cost > inlineHotMaxBudget {
if logopt.Enabled() {
@ -1038,8 +1038,8 @@ func mkinlcall(n *ir.CallExpr, fn *ir.Func, maxCost int32, inlCalls *[]*ir.Inlin
}
if base.Debug.PGOInline > 0 {
ln := pgo.ConvertLine2Int(ir.Line(n))
csi := pgo.CallSiteInfo{Line: ln, Caller: ir.CurFunc}
line := int(base.Ctxt.InnermostPos(n.Pos()).RelLine())
csi := pgo.CallSiteInfo{Line: line, Caller: ir.CurFunc}
if _, ok := inlinedCallSites[csi]; !ok {
inlinedCallSites[csi] = struct{}{}
}

49
src/cmd/compile/internal/pgo/irgraph.go
View file

@ -4,9 +4,45 @@
// WORK IN PROGRESS
// 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()).
//
// 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,
// pprof profiles generated by the runtime always contain line numbers as
// adjusted by //line directives (because that is what we put in pclntab). Thus
// for the best behavior when attempting to match the source with the profile
// it makes sense to use the same line number space.
//
// Some of the effects of this to keep in mind:
//
// - For files without //line directives there is no impact, as RelLine() ==
// Line().
// - For functions entirely covered by the same //line directive (i.e., a
// directive before the function definition and no directives within the
// function), there should also be no impact, as line offsets within the
// function should be the same as the real line offsets.
// - Functions containing //line directives may be impacted. As fake line
// numbers need not be monotonic, we may compute negative line offsets. We
// should accept these and attempt to use them for best-effort matching, as
// these offsets should still match if the source is unchanged, and may
// continue to match with changed source depending on the impact of the
// changes on fake line numbers.
// - Functions containing //line directives may also contain duplicate lines,
// making it ambiguous which call the profile is referencing. This is a
// similar problem to multiple calls on a single real line, as we don't
// currently track column numbers.
//
// Long term it would be best to extend pprof profiles to include real line
// numbers. Until then, we have to live with these complexities. Luckily,
// //line directives that change line numbers in strange ways should be rare,
// and failing PGO matching on these files is not too big of a loss.
package pgo
import (
"cmd/compile/internal/base"
"cmd/compile/internal/ir"
"cmd/compile/internal/typecheck"
"cmd/compile/internal/types"
@ -14,8 +50,6 @@ import (
"internal/profile"
"log"
"os"
"strconv"
"strings"
)
// IRGraph is the key datastrcture that is built from profile. It is essentially a call graph with nodes pointing to IRs of functions and edges carrying weights and callsite information. The graph is bidirectional that helps in removing nodes efficiently.
@ -131,13 +165,6 @@ func BuildWeightedCallGraph() {
WeightedCG = createIRGraph()
}
// ConvertLine2Int converts ir.Line string to integer.
func ConvertLine2Int(line string) int {
splits := strings.Split(line, ":")
cs, _ := strconv.ParseInt(splits[len(splits)-2], 0, 64)
return int(cs)
}
// preprocessProfileGraph builds various maps from the profile-graph. It builds GlobalNodeMap and other information based on the name and callsite to compute node and edge weights which will be used later on to create edges for WeightedCG.
func preprocessProfileGraph() {
nFlat := make(map[string]int64)
@ -284,7 +311,7 @@ func (g *IRGraph) createIRGraphEdge(fn *ir.Func, callernode *IRNode, name string
ir.DoChildren(n, doNode)
case ir.OCALLFUNC:
call := n.(*ir.CallExpr)
line := ConvertLine2Int(ir.Line(n))
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 {
@ -294,7 +321,7 @@ func (g *IRGraph) createIRGraphEdge(fn *ir.Func, callernode *IRNode, name string
call := n.(*ir.CallExpr)
// Find the callee method from the call site and add the edge.
fn2 := ir.MethodExprName(call.X).Func
line := ConvertLine2Int(ir.Line(n))
line := int(base.Ctxt.InnermostPos(n.Pos()).RelLine())
g.addEdge(callernode, fn2, &n, name, line)
}
return false