Currently locations are stored in a map and looked up by ID from
the map. The IDs are usually small sequential integers (the Go
pprof CPU profiles are so). Using a slice is more efficient (with
a fallback map to handle weirdly large IDs).
Change-Id: I9e20d5cebca3a5239636413e1bf2f0b273038031
Reviewed-on: https://go-review.googlesource.com/c/go/+/447803
Run-TryBot: Cherry Mui <cherryyz@google.com>
Reviewed-by: Michael Pratt <mpratt@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
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 <cherryyz@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Michael Pratt <mpratt@google.com>
Auto-Submit: Michael Pratt <mpratt@google.com>
Currently, with PGO, the inliner uses node weights to decide if a
function is inlineable (with a larger budget). But the actual
inlining is determined by the weight of the call edge. There is a
discrepancy that, if a callee node is hot but the call edge is not,
it would not inlined, and marking the callee inlineable would of
no use.
Instead of using two kinds of weights, we just use the edge
weights to decide inlineability. If a function is the callee of a
hot call edge, its inlineability is determined with a larger
threshold. For a function that exceeds the regular inlining budget,
it is still inlined only when the call edge is hot, as it would
exceed the regular inlining cost for non-hot call sites, even if
it is marked inlineable.
For #55022.
Change-Id: I93fa9919fc6bcbb394e6cfe54ec96a96eede08f7
Reviewed-on: https://go-review.googlesource.com/c/go/+/447015
Run-TryBot: Cherry Mui <cherryyz@google.com>
Reviewed-by: Michael Pratt <mpratt@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
The global ListOfHotCallSites set is used to communicate between
CanInline and InlineCalls the set of call sites that InlineCalls may
increase the budget for.
CanInline clears this map on each call, thus assuming that
InlineCalls(x) is called immediately after CanInline(x). This assumption
is false, as CanInline (among other cases) is recursive (CanInline ->
hairyVisitor.doNode -> inlCallee -> CanInline).
When this assumption proves false, we will lose the opportunity to
inline hot calls.
This CL is the least invasive fix for this. ListOfHotCallSites is
actually just a subset of the candHotEdgeMap, with CallSiteInfo.Callee
cleared. candHotEdgeMap doesn't actually need to distinguish based on
Callee, so we can drop callee from candHotEdgeMap as well and just use
that directly [1].
Later CLs should do more work to remove the globals entirely.
For cmd/compile, this inceases the number of PGO inlined functions by
~50% for one set of PGO parameters. I have no evaluated performance
impact.
[1] This is something that we likely want to change in the future.
For #55022.
Change-Id: I57735958d651f6dfa9bd296499841213d20e1706
Reviewed-on: https://go-review.googlesource.com/c/go/+/446755
Auto-Submit: Michael Pratt <mpratt@google.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
Run-TryBot: Michael Pratt <mpratt@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Since pgo is a new package, it is reasonably straightforward to
encapsulate its state into a non-global object that we pass around,
which will help keep it isolated.
There are no functional changes in this CL, just packaging up the
globals into a new object.
There are two major pieces of cleanup remaining:
1. reflectdata and noder have separate InlineCalls calls for method
wrappers. The Profile is not plumbed there yet, but this is not a
regression as the globals were previously set only right around the
main inlining pass in gc.Main.
2. pgo.ListOfHotCallSites is still global, as it will require more work
to clean up. It is effectively a local variable in InlinePackage,
except that it assumes that InlineCalls is immediately preceded by a
CanInline call for the same function. This is not necessarily true
due to the recursive nature of CanInline. This also means that some
InlineCalls calls may be missing the list of hot callsites right now.
For #55022.
Change-Id: Ic1fe41f73df96861c65f8bfeecff89862b367290
Reviewed-on: https://go-review.googlesource.com/c/go/+/446303
Reviewed-by: Cherry Mui <cherryyz@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Michael Pratt <mpratt@google.com>
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>
