Conversation
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
src/coreclr/vm/jitinterface.cpp
Outdated
|
|
||
| if (pMD->IsIL() && pMD->HasILHeader()) | ||
| { | ||
| COR_ILMETHOD_DECODER header(pMD->GetILHeader()); |
There was a problem hiding this comment.
I'm not sure it's cheap, though..
|
@EgorBot -arm -linux_arm -profiler using BenchmarkDotNet.Attributes;
using System.Buffers;
namespace System.Text.Encodings.Web.Tests
{
public class Perf_Encoders
{
public IEnumerable<object> GetEncoderArguments()
{
foreach (int size in new[] { 16 })
{
yield return new EncoderArguments("&lorem ipsum=dolor sit amet", size, UrlEncoder.Default);
}
}
[Benchmark]
[ArgumentsSource(nameof(GetEncoderArguments))]
public OperationStatus EncodeUtf16(EncoderArguments arguments) => arguments.EncodeUtf16();
public class EncoderArguments
{
private readonly string _sourceString;
// pads the string with a pseudorandom number of non-escapable characters
private readonly int _paddingSize;
private readonly TextEncoder _encoder;
private readonly string _sourceBufferUtf16;
private readonly char[] _destinationBufferUtf16;
private readonly byte[] _sourceBufferUtf8;
private readonly byte[] _destinationBufferUtf8;
public EncoderArguments(string sourceString, int paddingSize, TextEncoder encoder)
{
_sourceString = sourceString;
_paddingSize = paddingSize;
_encoder = encoder;
_sourceBufferUtf16 = BuildSourceString();
_destinationBufferUtf16 = new char[paddingSize + 10 * sourceString.Length];
_sourceBufferUtf8 = Encoding.UTF8.GetBytes(_sourceBufferUtf16);
_destinationBufferUtf8 = new byte[paddingSize + 10 * sourceString.Length];
string BuildSourceString()
{
var sb = new StringBuilder();
// pad the string with `paddingSize` non-escapable ascii characters
var random = new Random(42);
for (int i = 0; i < paddingSize; i++)
{
sb.Append((char)random.Next('a', 'z' + 1));
}
sb.Append(sourceString);
return sb.ToString();
}
}
public override string ToString() => $"{GetShortEncoderName()},{_sourceString},{_paddingSize}";
// the name is displayed in the results in console, we want it as short as possible
private string GetShortEncoderName()
{
if (_encoder.Equals(JavaScriptEncoder.Default))
return "JavaScript";
if (_encoder.Equals(JavaScriptEncoder.UnsafeRelaxedJsonEscaping))
return "UnsafeRelaxed";
if (_encoder.Equals(UrlEncoder.Default))
return "Url";
throw new NotSupportedException("Unknown encoder.");
}
public OperationStatus EncodeUtf8() => _encoder.EncodeUtf8(_sourceBufferUtf8, _destinationBufferUtf8, out int _, out int _);
public OperationStatus EncodeUtf16() => _encoder.Encode(_sourceBufferUtf16, _destinationBufferUtf16, out int _, out int _);
}
}
} |
There was a problem hiding this comment.
Pull request overview
This PR introduces a new JIT/EE interface helper (tryGetMethodILSize) so the JIT can query a callee’s IL size plus whether it’s marked AggressiveInlining, and then factors the cumulative size of such force-inline callees into the inliner’s estimated IL size to add “friction” and reduce inliner-budget cascades.
Changes:
- Add
ICorStaticInfo::tryGetMethodILSizeand implement it across CoreCLR EE, JIT wrappers, NativeAOT JIT interface, and SuperPMI record/replay. - Teach the JIT’s opcode scan to record a new inline observation for calls to
AggressiveInliningmethods, and adjustExtendedDefaultPolicy’s estimated IL size using this data. - Bump the JIT/EE versioning GUID to reflect the interface change.
Reviewed changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/coreclr/inc/corinfo.h | Adds the new tryGetMethodILSize API to the JIT/EE contract. |
| src/coreclr/vm/jitinterface.cpp | Implements CEEInfo::tryGetMethodILSize in the EE. |
| src/coreclr/inc/icorjitinfoimpl_generated.h | Updates the EE-side generated interface implementation declarations. |
| src/coreclr/inc/jiteeversionguid.h | Updates the JIT/EE version GUID for the contract change. |
| src/coreclr/jit/compiler.h | Declares the new Compiler::eeTryGetMethodILSize helper. |
| src/coreclr/jit/ee_il_dll.hpp | Adds the JIT-to-EE call-through wrapper for tryGetMethodILSize. |
| src/coreclr/jit/fgbasic.cpp | During precise inline scanning, queries callee IL size + AggressiveInlining and records a new observation. |
| src/coreclr/jit/inline.def | Adds a new inline observation for force-inline calls. |
| src/coreclr/jit/inlinepolicy.h | Tracks cumulative force-inline callee IL size in ExtendedDefaultPolicy. |
| src/coreclr/jit/inlinepolicy.cpp | Accumulates the new observation and adjusts estimated IL size accordingly. |
| src/coreclr/jit/ICorJitInfo_wrapper_generated.hpp | Adds wrapper support for the new API. |
| src/coreclr/jit/ICorJitInfo_names_generated.h | Adds the API name for generated thunking/logging. |
| src/coreclr/tools/Common/JitInterface/ThunkGenerator/ThunkInput.txt | Adds the API to the thunk-generator input. |
| src/coreclr/tools/Common/JitInterface/CorInfoImpl_generated.cs | Wires up the managed callback for the new API (NativeAOT). |
| src/coreclr/tools/Common/JitInterface/CorInfoImpl.cs | Implements the managed-side logic to compute IL size + AggressiveInlining. |
| src/coreclr/tools/aot/jitinterface/jitinterface_generated.h | Extends NativeAOT JIT interface callback table/wrapper. |
| src/coreclr/tools/superpmi/superpmi/icorjitinfo.cpp | Adds SuperPMI ICJI forwarding for the new API. |
| src/coreclr/tools/superpmi/superpmi-shim-simple/icorjitinfo_generated.cpp | Adds shim forwarding for the new API. |
| src/coreclr/tools/superpmi/superpmi-shim-counter/icorjitinfo_generated.cpp | Adds shim call counting for the new API. |
| src/coreclr/tools/superpmi/superpmi-shim-collector/icorjitinfo.cpp | Records/replays tryGetMethodILSize data in SuperPMI collection. |
| src/coreclr/tools/superpmi/superpmi-shared/lwmlist.h | Adds a new LWM map slot for recording TryGetMethodILSize. |
| src/coreclr/tools/superpmi/superpmi-shared/methodcontext.h | Declares record/replay methods and adds packet IDs for the new data. |
| src/coreclr/tools/superpmi/superpmi-shared/methodcontext.cpp | Implements record/dump/replay for TryGetMethodILSize. |
|
Would it be possible to inline "bottom up" and then each inline decision has a true picture of the cost of the code it's going to potentially inline? It also means there's no heuristics or guessing? |
For AOT - yes, for the JIT - no |
Thanks for the reply! I was wondering if it was something OSR could do incrementally but I guess it would lead to a lot of expensive compiling.... |
- Use raw COR_ILMETHOD_TINY/FAT instead of COR_ILMETHOD_DECODER to avoid decoder overhead and tighten contract to GC_NOTRIGGER - Remove stale Packet_IsAggressiveInlining enum value Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
An attempt to "fix" the "exceed inliner budget" problem by avoiding inlining methods with call opcodes where call opcodes are big AggressiveInline methods, JIT doesn't take into account that by agreeing to inline A it automatically inlines all small (below ALLWAYS_INLINE) and AggressiveInlining entities. It should have some fricition.
I hit this problem too often as part of the Reduce-Unsafe effort, where I replace unsafe code with safe and it suddenly starts regressing due to inliner budge, a good example is this recent issue: #125781
Typical scenario is: benefit-multiplier motivates us to inline a large method, and we quickly run out of the budget after inlining callees inside it and end up giving up on e.g.
System.ReadOnlySpan'1[ulong]:.ctor(byref,int):thisetc