-
Notifications
You must be signed in to change notification settings - Fork 5.2k
JIT: enable instrumentation for inlinees #119658
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
If we are doing an optimized+instrumented jit pass (like we do for R2R methods) allow the inlinees to be instrumented. This gives us a chance to collect profile data for methods that are always inlined. All inlinees currently share the same profile data segment with each other and with a root compilation of the method (if any). So this profile is "context-free". If there is a schema mismatch (say there is a stale pre-existing R2R schema) then subsequent instrumentation will fail. This is something we need to keep an eye on. Right now we can't distinguish this failure from other kinds of schema allocation failures. Closes dotnet#44372
This should address the long-standing regression in #91938. Local runs show it possibly helps but doesn't completely fix the regression. Still need to verify it is actually getting instrumentation data for the key inlinees. Generally speaking, this should help reduce the gap between running with R2R enabled (where we can lose profile data, or optimize based on stale data) and R2R disabled. |
I have a bunch more fixes on top of this, will PR them soon. One issue I'm seeing is IL mismatches. Some of it I've tracked down to IL differences between the PGO schema embedded into corelib and the current corelib IL; that's somewhat understandable as the PGO data likely goes stale, and should be (generally) a non-product issue. Would still be good to flag this case and allow the dynamic PGO to take over. I also suspect inlinee schemas may diverge from root schemas even for the same IL, but need to do more work to track this down. |
Still some failures to sort out. Also my local perf runs aren't as promising as I'd hoped. |
Ok, think I tracked down the last few issues. |
Local run. Regression fixed. TC now matches no-R2R perf here.
|
@EgorBot --intel --arm --filter System.Globalization.Tests.StringSearch.LastIndexOf_Word_NotFound(Options: (en-US, OrdinalIgnoreCase, False)) |
@EgorBot --intel --arm --filter |
@EgorBot --intel --arm --filter |
/azp run runtime-coreclr libraries-pgo, runtime-coreclr pgostress, runtime-coreclr pgo |
Azure Pipelines successfully started running 3 pipeline(s). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR enables instrumentation for inlined methods (inlinees) when performing optimized+instrumented JIT passes, such as R2R methods. This allows collection of profile data for methods that are always inlined, providing "context-free" profile data.
Key changes:
- Removes assertions and bailouts that prevented inlinee instrumentation
- Handles schema mismatches by discarding stale data and using new schemas
- Adds special handling for inlinee return expressions during instrumentation
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
src/coreclr/vm/pgo.cpp | Updates PGO allocation to handle schema mismatches by removing stale data and relaying schemas |
src/coreclr/vm/jitinterface.cpp | Changes PGO allocation to use method handle instead of method being compiled |
src/coreclr/jit/jitconfigvalues.h | Adds JitInstrumentIfOptimizing configuration option |
src/coreclr/jit/importercalls.cpp | Removes inlining restrictions for instrumentation and tail call handling |
src/coreclr/jit/fgprofile.cpp | Removes inlining restrictions and adds wrapper for inlinee return expression handling |
src/coreclr/jit/fginline.cpp | Removes clearing of instrumentation flags for inlinees |
src/coreclr/jit/compiler.h | Adds fgInstrumentMethodCore method declaration |
src/coreclr/jit/compiler.cpp | Removes inlining assertions and adds debug instrumentation forcing |
src/coreclr/inc/pgo_formatprocessing.h | Changes schema compatibility check to return match count instead of boolean |
@dotnet/jit-contrib PTAL There will be a sizable code size / tp increase in some collections. |
Libraries-pgo failure looks like a GC hole but suspect it is something pre-existing that we're just uncovering here. Pgo failure is in stackoverflow tester. Diffs. As promised large size increases, though the bulk of that is from libraries tests and (not surprisingly) mostly in Tier1-instr. A decent fraction of context misses too, so the accuracy of SPMI is a bit questionable. I am not really sure how to do a fair assessment here as the new data gathered by this instrumentation should have a big effect on Tier1. |
@EgorBot -amd -arm using System;
using System.Runtime.CompilerServices;
using System.Runtime.InteropServices;
using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Running;
public class Prog
{
static void Main(string[] args)
{
BenchmarkSwitcher.FromAssembly(typeof(Prog).Assembly).Run(args);
}
byte[] Src = new byte[32];
byte[] Dst = new byte[32];
[Benchmark]
public void Copy() => Src.AsSpan().CopyTo(Dst);
} |
@EgorBo PTAL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
If we are doing an optimized+instrumented jit pass (like we do for R2R methods) allow the inlinees to be instrumented. This gives us a chance to collect profile data for methods that are always inlined.
All inlinees currently share the same profile data segment with each other and with a root compilation of the method (if any). So this profile is "context-free".
If there is a schema mismatch (say there is a stale pre-existing R2R schema) then we disregard the old schema and use the new one.
This is mostly just removing assertions and bailouts and bad assumptions. However if the inlinee has an instrumentable call in its return expression we need to temporarily insert it as normal IR so it can get instrumented like anything else. We then undo this after instrumentation. Any residual impact from instrumentation will be left either in prior statements or commas inside the return expression.
Closes #44372. Closes #91938.