-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Disable TC for UnmanagedCallersOnly methods. #46550
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
Disable TC for UnmanagedCallersOnly methods. #46550
Conversation
|
After trying to repro locally, this is looking very odd. There's already some code in jithelpers.cpp to force jitted UnmanagedCallersOnly methods to switch to optimized code which works locally in what I thought would be a minimal repro, but for some reason the failing case (the dump from the tests) has the method QuickJitted. I'll make this a draft until I can actually get a local repro. |
|
I've found the issue. It's a bad interaction between JitStress and the way UnmanagedCallersOnly opts out of TieredCompilation. Working on a fix now. |
…hed to Optimized to switch back to MinOpts (was occuring in JitStress=2 and causing the crash).
… call counting for UnmanagedCallersOnly even if we had previously opted in.
…tzinsky/runtime into unmanagedcallersonly-no-tieredcomp
|
I tried out another new approach. This time we'll override the "should we do call counting" after we finish getting the R2R or jitted code if the method is UnmanagedCallersOnly. I left in the previous checks in since we still want to force the optimization level for the jitted code. |
|
/azp run runtime-coreclr libraries-jitstress |
|
Azure Pipelines successfully started running 1 pipeline(s). |
kouvel
left a comment
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.
I think this can be refactored a bit to simplify/improve the logic in various places and to reflect the state of the method more correctly:
- Move all of the following to the beginning of
MethodDesc::PrepareILBasedCode
runtime/src/coreclr/vm/prestub.cpp
Lines 444 to 466 in 0622fa0
#ifdef FEATURE_TIERED_COMPILATION bool shouldTier = pConfig->GetMethodDesc()->IsEligibleForTieredCompilation(); #if !defined(TARGET_X86) CallerGCMode callerGcMode = pConfig->GetCallerGCMode(); // If the method is eligible for tiering but is being // called from a Preemptive GC Mode thread or the method // has the UnmanagedCallersOnlyAttribute then the Tiered Compilation // should be disabled. if (shouldTier && (callerGcMode == CallerGCMode::Preemptive || (callerGcMode == CallerGCMode::Unknown && HasUnmanagedCallersOnlyAttribute()))) { NativeCodeVersion codeVersion = pConfig->GetCodeVersion(); if (codeVersion.IsDefaultVersion()) { pConfig->GetMethodDesc()->GetLoaderAllocator()->GetCallCountingManager()->DisableCallCounting(codeVersion); } codeVersion.SetOptimizationTier(NativeCodeVersion::OptimizationTierOptimized); shouldTier = false; } #endif // !TARGET_X86 #endif // FEATURE_TIERED_COMPILATION - Pass
shouldTierintoMethodDesc::GetPrecompiledCode - Delete all of
runtime/src/coreclr/vm/jitinterface.cpp
Lines 13027 to 13042 in 0622fa0
#ifdef FEATURE_TIERED_COMPILATION // Clearing all tier flags and mark as optimized if the reverse P/Invoke // flag is used and the function is eligible. if (flags.IsSet(CORJIT_FLAGS::CORJIT_FLAG_REVERSE_PINVOKE) && ftn->IsEligibleForTieredCompilation()) { _ASSERTE(config->GetCallerGCMode() != CallerGCMode::Coop); // Clear all possible states. flags.Clear(CORJIT_FLAGS::CORJIT_FLAG_TIER0); flags.Clear(CORJIT_FLAGS::CORJIT_FLAG_TIER1); config->SetJitSwitchedToOptimized(); } #endif // FEATURE_TIERED_COMPILATION
With call counting properly disabled, the JIT flags sent would request optimization, and the tier that shows up in profiles would be correct.
…o optimized code. We don't want to lock them at QuickJit.
|
/azp run runtime-coreclr libraries-jitstress |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Ah I think there is a bug currently in the optimization flags when call counting is disabled, so it wouldn't work the way I expected it to with the above changes I suggested. Please ignore my suggestions above, I'll fix the bug separately and update along with that. |
|
Also if you have a small test case can you please share it with me so that I can make sure it is working as intended? Or is it checked in somewhere? |
|
My test case is kind of hacky, but here it is: using System;
using System.Runtime.InteropServices;
var calendarDataType = Type.GetType("System.Globalization.CalendarData");
var method = calendarDataType.GetMethod("EnumCalendarInfoCallback", System.Reflection.BindingFlags.NonPublic | System.Reflection.BindingFlags.Static, null, System.Reflection.CallingConventions.Standard, new[] { typeof(char*), typeof(uint), typeof(IntPtr), typeof(void*)}, null);
var fnptr = method.MethodHandle.GetFunctionPointer();
unsafe
{
byte b = ((delegate* unmanaged<char*, uint, IntPtr, void*, byte>)fnptr)(null, 0, IntPtr.Zero, null);
}I validated that when running with no COMPlus flags, the JIT builds the code as untiered (optimized) and |
|
The System.Private.Uri.Functional.Tests dump looks like a GC hole which I believe is unrelated to this PR. I'll open a separate issue for it and merge this in. |
- Simplifies #46550 and fixes some issues - Disables call counting and adjusts the tier appropriately for `UnmanagedCallersOnly` methods - Fixes optimization flags sent to the JIT for the default code version when call counting is disabled, including when an `UnmanagedCallersOnly` method is attributed with `AggressiveOptimization`. On the default code version path previously it wasn't checking if call counting was disabled, and since that's an expensive check to add on that path, added a flag to `PrepareCodeConfig`. - Fixes some miscellaneous inconsistencies between call counting enablement, optimization tier, and jit flags
Tiered compilation uses call counting and the call counting mechanism today is incompatible with UnmanagedCallersOnly. Disabled tiered compilation for UnmanagedCallersOnly methods.
Fixes #46512