Skip to content

Commit 2d428cf

Browse files
prattmicgopherbot
authored andcommitted
cmd/compile: fix unstable selection of hottest edge
When selecting the hottest edge to use for PGO-based devirtualization, edges are order by: 1. Edge weight 2. If weights are equal, prefer the edge with IR available in the package. 3. Otherwise, simply sort lexicographically. The existing logic for (2) is incomplete. If the hottest edge so far is missing IR, but the new edge has IR, then it works as expected and selects the new edge. But if the hottest edge so far has IR and the new edge is missing IR, we want to always keep the hottest edge so far, but this logic will fall through and use lexicographical ordering instead. Adjust the check to always make an explicit choice when IR availability differs. Change-Id: Ia7fcc286aa9a62ac209fd978cfce60463505f4cd Reviewed-on: https://go-review.googlesource.com/c/go/+/539475 Auto-Submit: Michael Pratt <[email protected]> Reviewed-by: Cherry Mui <[email protected]> LUCI-TryBot-Result: Go LUCI <[email protected]>
1 parent 547eb8d commit 2d428cf

File tree

2 files changed

+157
-3
lines changed

2 files changed

+157
-3
lines changed

src/cmd/compile/internal/devirtualize/pgo.go

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -457,9 +457,12 @@ func findHotConcreteCallee(p *pgo.Profile, caller *ir.Func, call *ir.CallExpr) (
457457
// Now e.Weight == hottest.Weight, we must select on other
458458
// criteria.
459459

460-
if hottest.Dst.AST == nil && e.Dst.AST != nil {
461-
// Prefer the edge with IR available.
462-
return true
460+
// If only one edge has IR, prefer that one.
461+
if (hottest.Dst.AST == nil) != (e.Dst.AST == nil) {
462+
if e.Dst.AST != nil {
463+
return true
464+
}
465+
return false
463466
}
464467

465468
// Arbitrary, but the callee names will always differ. Select
Lines changed: 151 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,151 @@
1+
// Copyright 2023 The Go Authors. All rights reserved.
2+
// Use of this source code is governed by a BSD-style
3+
// license that can be found in the LICENSE file.
4+
5+
package devirtualize
6+
7+
import (
8+
"cmd/compile/internal/base"
9+
"cmd/compile/internal/ir"
10+
"cmd/compile/internal/pgo"
11+
"cmd/compile/internal/types"
12+
"cmd/compile/internal/typecheck"
13+
"cmd/internal/obj"
14+
"cmd/internal/src"
15+
"testing"
16+
)
17+
18+
func init() {
19+
// These are the few constants that need to be initialized in order to use
20+
// the types package without using the typecheck package by calling
21+
// typecheck.InitUniverse() (the normal way to initialize the types package).
22+
types.PtrSize = 8
23+
types.RegSize = 8
24+
types.MaxWidth = 1 << 50
25+
typecheck.InitUniverse()
26+
base.Ctxt = &obj.Link{}
27+
base.Debug.PGODebug = 3
28+
}
29+
30+
func makePos(b *src.PosBase, line, col uint) src.XPos {
31+
return base.Ctxt.PosTable.XPos(src.MakePos(b, line, col))
32+
}
33+
34+
func TestFindHotConcreteCallee(t *testing.T) {
35+
// findHotConcreteCallee only uses pgo.Profile.WeightedCG, so we're
36+
// going to take a shortcut and only construct that.
37+
p := &pgo.Profile{
38+
WeightedCG: &pgo.IRGraph{
39+
IRNodes: make(map[string]*pgo.IRNode),
40+
},
41+
}
42+
43+
// Create a new IRNode and add it to p.
44+
//
45+
// fn may be nil, in which case the node will set LinkerSymbolName.
46+
newNode := func(name string, fn *ir.Func) *pgo.IRNode {
47+
n := &pgo.IRNode{
48+
OutEdges: make(map[pgo.NamedCallEdge]*pgo.IREdge),
49+
}
50+
if fn != nil {
51+
n.AST = fn
52+
} else {
53+
n.LinkerSymbolName = name
54+
}
55+
p.WeightedCG.IRNodes[name] = n
56+
return n
57+
}
58+
59+
// Add a new call edge from caller to callee.
60+
addEdge := func(caller, callee *pgo.IRNode, offset int, weight int64) {
61+
namedEdge := pgo.NamedCallEdge{
62+
CallerName: caller.Name(),
63+
CalleeName: callee.Name(),
64+
CallSiteOffset: offset,
65+
}
66+
irEdge := &pgo.IREdge{
67+
Src: caller,
68+
Dst: callee,
69+
CallSiteOffset: offset,
70+
Weight: weight,
71+
}
72+
caller.OutEdges[namedEdge] = irEdge
73+
}
74+
75+
pkgFoo := types.NewPkg("example.com/foo", "foo")
76+
basePos := src.NewFileBase("foo.go", "/foo.go")
77+
78+
// Create a new struct type named structName with a method named methName and
79+
// return the method.
80+
makeStructWithMethod := func(structName, methName string) *ir.Func {
81+
// type structName struct{}
82+
structType := types.NewStruct(nil)
83+
84+
// func (structName) methodName()
85+
recv := types.NewField(src.NoXPos, typecheck.Lookup(structName), structType)
86+
sig := types.NewSignature(recv, nil, nil)
87+
fn := ir.NewFunc(src.NoXPos, src.NoXPos, pkgFoo.Lookup(structName + "." + methName), sig)
88+
89+
// Add the method to the struct.
90+
structType.SetMethods([]*types.Field{types.NewField(src.NoXPos, typecheck.Lookup(methName), sig)})
91+
92+
return fn
93+
}
94+
95+
const (
96+
// Caller start line.
97+
callerStart = 42
98+
99+
// The line offset of the call we care about.
100+
callOffset = 1
101+
102+
// The line offset of some other call we don't care about.
103+
wrongCallOffset = 2
104+
)
105+
106+
// type IFace interface {
107+
// Foo()
108+
// }
109+
fooSig := types.NewSignature(types.FakeRecv(), nil, nil)
110+
method := types.NewField(src.NoXPos, typecheck.Lookup("Foo"), fooSig)
111+
iface := types.NewInterface([]*types.Field{method})
112+
113+
callerFn := ir.NewFunc(makePos(basePos, callerStart, 1), src.NoXPos, pkgFoo.Lookup("Caller"), types.NewSignature(nil, nil, nil))
114+
115+
hotCalleeFn := makeStructWithMethod("HotCallee", "Foo")
116+
coldCalleeFn := makeStructWithMethod("ColdCallee", "Foo")
117+
wrongLineCalleeFn := makeStructWithMethod("WrongLineCallee", "Foo")
118+
wrongMethodCalleeFn := makeStructWithMethod("WrongMethodCallee", "Bar")
119+
120+
callerNode := newNode("example.com/foo.Caller", callerFn)
121+
hotCalleeNode := newNode("example.com/foo.HotCallee.Foo", hotCalleeFn)
122+
coldCalleeNode := newNode("example.com/foo.ColdCallee.Foo", coldCalleeFn)
123+
wrongLineCalleeNode := newNode("example.com/foo.WrongCalleeLine.Foo", wrongLineCalleeFn)
124+
wrongMethodCalleeNode := newNode("example.com/foo.WrongCalleeMethod.Foo", wrongMethodCalleeFn)
125+
126+
hotMissingCalleeNode := newNode("example.com/bar.HotMissingCallee.Foo", nil)
127+
128+
addEdge(callerNode, wrongLineCalleeNode, wrongCallOffset, 100) // Really hot, but wrong line.
129+
addEdge(callerNode, wrongMethodCalleeNode, callOffset, 100) // Really hot, but wrong method type.
130+
addEdge(callerNode, hotCalleeNode, callOffset, 10)
131+
addEdge(callerNode, coldCalleeNode, callOffset, 1)
132+
133+
// Equal weight, but IR missing.
134+
//
135+
// N.B. example.com/bar sorts lexicographically before example.com/foo,
136+
// so if the IR availability of hotCalleeNode doesn't get precedence,
137+
// this would be mistakenly selected.
138+
addEdge(callerNode, hotMissingCalleeNode, callOffset, 10)
139+
140+
// IFace.Foo()
141+
sel := typecheck.NewMethodExpr(src.NoXPos, iface, typecheck.Lookup("Foo"))
142+
call := ir.NewCallExpr(makePos(basePos, callerStart+callOffset, 1), ir.OCALLINTER, sel, nil)
143+
144+
gotFn, gotWeight := findHotConcreteCallee(p, callerFn, call)
145+
if gotFn != hotCalleeFn {
146+
t.Errorf("findHotConcreteInterfaceCallee func got %v want %v", gotFn, hotCalleeFn)
147+
}
148+
if gotWeight != 10 {
149+
t.Errorf("findHotConcreteInterfaceCallee weight got %v want 10", gotWeight)
150+
}
151+
}

0 commit comments

Comments
 (0)