]> Cypherpunks.ru repositories - gostls13.git/commitdiff
cmd/compile/internal/pgo: remove ConvertLine2Int
authorMichael Pratt <mpratt@google.com>
Fri, 28 Oct 2022 17:52:43 +0000 (13:52 -0400)
committerGopher Robot <gobot@golang.org>
Mon, 31 Oct 2022 21:00:25 +0000 (21:00 +0000)
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>

src/cmd/compile/internal/inline/inl.go
src/cmd/compile/internal/pgo/irgraph.go

index 4d942d037fc8f71c4b56684c937157af97c487ba..7e1a9adae87b5a44936b67e10784b50471b35065 100644 (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{}{}
                }
index 98238823f7f98bd5a4a5a8623769f5a1b5c277ad..e6e183cdd22b3a6cf543e8586ab2049d54aa3785 100644 (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