Skip to content

Commit b5d37d1

Browse files
kgkg
authored
Fix issues with rejit profiler test (#117826)
* When replacing strings during rejit in the profiler rejit test, include the method name in the replacement string so it is easier to tell what is going on * Capture and validate actual output in the profiler rejit test * Re-enable rejit test on crossgen * Remove dead fIsRevert parameter in rejit C++ implementation * Make rejit test handle crossgen composite behavior where no inlining happens
1 parent 7721695 commit b5d37d1

File tree

5 files changed

+102
-41
lines changed

5 files changed

+102
-41
lines changed

‎src/coreclr/vm/rejit.cpp‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -548,7 +548,9 @@ HRESULT ReJitManager::UpdateActiveILVersions(
548548

549549
if ((flags & COR_PRF_REJIT_BLOCK_INLINING) == COR_PRF_REJIT_BLOCK_INLINING)
550550
{
551-
hr = UpdateNativeInlinerActiveILVersions(&mgrToCodeActivationBatch, pModule, rgMethodDefs[i], fIsRevert, flags);
551+
_ASSERTE(!fIsRevert);
552+
553+
hr = UpdateNativeInlinerActiveILVersions(&mgrToCodeActivationBatch, pModule, rgMethodDefs[i], flags);
552554
if (FAILED(hr))
553555
{
554556
return hr;
@@ -711,7 +713,6 @@ HRESULT ReJitManager::UpdateNativeInlinerActiveILVersions(
711713
SHash<CodeActivationBatchTraits> *pMgrToCodeActivationBatch,
712714
Module *pInlineeModule,
713715
mdMethodDef inlineeMethodDef,
714-
BOOL fIsRevert,
715716
COR_PRF_REJIT_FLAGS flags)
716717
{
717718
CONTRACTL
@@ -758,7 +759,7 @@ HRESULT ReJitManager::UpdateNativeInlinerActiveILVersions(
758759
}
759760
}
760761

761-
hr = UpdateActiveILVersion(pMgrToCodeActivationBatch, inliner.m_module, inliner.m_methodDef, fIsRevert, flags);
762+
hr = UpdateActiveILVersion(pMgrToCodeActivationBatch, inliner.m_module, inliner.m_methodDef, FALSE /* fIsRevert */, flags);
762763
if (FAILED(hr))
763764
{
764765
ReportReJITError(inliner.m_module, inliner.m_methodDef, NULL, hr);

‎src/coreclr/vm/rejit.h‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -166,7 +166,6 @@ class ReJitManager
166166
SHash<CodeActivationBatchTraits> *pMgrToCodeActivationBatch,
167167
Module *pInlineeModule,
168168
mdMethodDef inlineeMethodDef,
169-
BOOL fIsRevert,
170169
COR_PRF_REJIT_FLAGS flags);
171170

172171
static HRESULT UpdateJitInlinerActiveILVersions(

‎src/tests/issues.targets‎

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -570,9 +570,6 @@
570570
<ExcludeList Include="$(XunitTestBinBase)/baseservices/multidimmarray/rank1array/**">
571571
<Issue>https://github.com/dotnet/runtime/issues/108255</Issue>
572572
</ExcludeList>
573-
<ExcludeList Include="$(XunitTestBinBase)/profiler/rejit/rejit/**">
574-
<Issue>https://github.com/dotnet/runtime/issues/109310</Issue>
575-
</ExcludeList>
576573
<ExcludeList Include="$(XunitTestBinBase)/JIT/Methodical/Arrays/misc/arrres_il_r/**">
577574
<Issue>https://github.com/dotnet/runtime/issues/109311</Issue>
578575
</ExcludeList>

‎src/tests/profiler/native/rejitprofiler/rejitprofiler.cpp‎

Lines changed: 48 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -95,13 +95,19 @@ HRESULT ReJITProfiler::Shutdown()
9595
_profInfo10 = nullptr;
9696
}
9797

98-
int expectedRejitCount = -1;
98+
int expectedRejitCount;
9999
auto it = _inlinings.find(_targetFuncId);
100100
if (it != _inlinings.end())
101101
{
102102
// The number of inliners are expected to ReJIT, plus the method itself
103103
expectedRejitCount = (int)((*it).second->size() + 1);
104104
}
105+
else
106+
{
107+
// No inlinings happened, which can occur in composite R2R mode on some targets.
108+
// This is fine as long as we rejitted the target method itself.
109+
expectedRejitCount = 1;
110+
}
105111

106112
INFO(L" rejit count=" << _rejits << L" expected rejit count=" << expectedRejitCount);
107113

@@ -320,7 +326,8 @@ HRESULT STDMETHODCALLTYPE ReJITProfiler::GetReJITParameters(ModuleID moduleId, m
320326
{
321327
SHUTDOWNGUARD();
322328

323-
INFO(L"Starting to build IL for method " << GetFunctionIDName(GetFunctionIDFromToken(moduleId, methodId, false)));
329+
String functionName = GetFunctionIDName(GetFunctionIDFromToken(moduleId, methodId, false));
330+
INFO(L"Starting to build IL for method " << functionName);
324331
COMPtrHolder<IUnknown> pUnk;
325332
HRESULT hr = _profInfo10->GetModuleMetaData(moduleId, ofWrite, IID_IMetaDataEmit2, &pUnk);
326333
if (FAILED(hr))
@@ -340,10 +347,18 @@ HRESULT STDMETHODCALLTYPE ReJITProfiler::GetReJITParameters(ModuleID moduleId, m
340347
}
341348

342349

343-
const WCHAR *wszNewUserDefinedString = WCHAR("Hello from profiler rejit!");
350+
String newUserDefinedString = String(WCHAR("Hello from profiler rejit method '"));
351+
newUserDefinedString += functionName;
352+
newUserDefinedString += WCHAR("'! ");
344353
mdString tokmdsUserDefined = mdTokenNil;
345-
hr = pTargetEmit->DefineUserString(wszNewUserDefinedString,
346-
(ULONG)wcslen(wszNewUserDefinedString),
354+
355+
// There's no portable way to convert a String to LPCWSTR so just make a manual copy on the stack.
356+
char16_t buf[4096] = { 0 };
357+
for (size_t i = 0, c = newUserDefinedString.Length(); i < c; i++)
358+
buf[i] = (char16_t)newUserDefinedString[i];
359+
360+
hr = pTargetEmit->DefineUserString((LPCWSTR)(void *)buf,
361+
(ULONG)newUserDefinedString.Length(),
347362
&tokmdsUserDefined);
348363
if (FAILED(hr))
349364
{
@@ -427,28 +442,39 @@ HRESULT STDMETHODCALLTYPE ReJITProfiler::ReJITError(ModuleID moduleId, mdMethodD
427442

428443
void ReJITProfiler::AddInlining(FunctionID inliner, FunctionID inlinee)
429444
{
430-
shared_ptr<unordered_set<FunctionID>> inliners;
431-
auto result = _inlinings.find(inlinee);
432-
if (result == _inlinings.end())
445+
String calleeName = GetFunctionIDName(inlinee);
446+
String moduleName = GetModuleIDName(GetModuleIDForFunction(inlinee));
447+
448+
// Depending on various things it's possible the JIT will inline code during our test run that isn't part of the
449+
// rejit test module. For example if part of the BCL didn't get crossgen'd or the crossgen'd code isn't used.
450+
// We don't care about those inlinings, so we won't track them. This makes the test more reliable.
451+
if (EndsWith(moduleName, String(WCHAR("rejit.dll"))))
433452
{
434-
auto p = make_pair(inlinee, make_shared<unordered_set<FunctionID>>());
435-
inliners = p.second;
436-
_inlinings.insert(p);
453+
shared_ptr<unordered_set<FunctionID>> inliners;
454+
auto result = _inlinings.find(inlinee);
455+
if (result == _inlinings.end())
456+
{
457+
auto p = make_pair(inlinee, make_shared<unordered_set<FunctionID>>());
458+
inliners = p.second;
459+
_inlinings.insert(p);
460+
}
461+
else
462+
{
463+
inliners = (*result).second;
464+
}
465+
466+
auto it = inliners->find(inliner);
467+
if (it == inliners->end())
468+
{
469+
inliners->insert(inliner);
470+
}
471+
472+
INFO(L"Inlining in test module! Inliner=" << GetFunctionIDName(inliner) << L" Inlinee=" << calleeName << L" module=" << moduleName);
437473
}
438474
else
439475
{
440-
inliners = (*result).second;
476+
INFO(L"Inlining in non-test module! Inliner=" << GetFunctionIDName(inliner) << L" Inlinee=" << calleeName << L" module=" << moduleName);
441477
}
442-
443-
auto it = inliners->find(inliner);
444-
if (it == inliners->end())
445-
{
446-
inliners->insert(inliner);
447-
}
448-
449-
String calleeName = GetFunctionIDName(inlinee);
450-
String moduleName = GetModuleIDName(GetModuleIDForFunction(inlinee));
451-
INFO(L"Inlining occurred! Inliner=" << GetFunctionIDName(inliner) << L" Inlinee=" << calleeName << L" Inlinee module name=" << moduleName);
452478
}
453479

454480
FunctionID ReJITProfiler::GetFunctionIDFromToken(ModuleID module, mdMethodDef token, bool invalidArgNotFailure)

‎src/tests/profiler/rejit/rejit.cs‎

Lines changed: 50 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,15 @@ class RejitWithInlining //: ProfilerTest
1010
{
1111
static readonly Guid ReJitProfilerGuid = new Guid("66F7A9DF-8858-4A32-9CFF-3AD0787E0186");
1212

13+
static System.Text.StringBuilder OutputBuilder = new ();
14+
15+
[MethodImplAttribute(MethodImplOptions.NoInlining)]
16+
public static void TestWriteLine(string s)
17+
{
18+
OutputBuilder.AppendLine(s);
19+
Console.WriteLine(s);
20+
}
21+
1322
[MethodImplAttribute(MethodImplOptions.NoInlining)]
1423
private static int MaxInlining()
1524
{
@@ -20,18 +29,41 @@ private static int MaxInlining()
2029

2130
TriggerReJIT();
2231

32+
OutputBuilder.Clear();
33+
2334
// TriggerInliningChain triggers a ReJIT, now this time we should call
2435
// in to the ReJITted code
2536
TriggerDirectInlining();
2637
CallMethodWithoutInlining();
2738
TriggerInliningChain();
2839

40+
string matchString = "Hello from profiler rejit method 'InlineeTarget'!";
41+
int numRejittedTargets = OutputBuilder.ToString().Split(matchString).Length;
42+
if (numRejittedTargets != 4)
43+
{
44+
Console.WriteLine("ReJIT did not update all instances of InlineeTarget!");
45+
return 1234;
46+
}
47+
2948
TriggerRevert();
3049

50+
OutputBuilder.Clear();
51+
3152
TriggerDirectInlining();
3253
CallMethodWithoutInlining();
3354
TriggerInliningChain();
3455

56+
if (OutputBuilder.ToString().Contains(matchString))
57+
{
58+
Console.WriteLine("ReJIT revert of InlineeTarget was not complete!");
59+
return 1235;
60+
}
61+
62+
// Currently there will still be some 'Hello from profiler rejit method' messages left
63+
// in the output in certain configurations. This is because the test profiler does not revert all
64+
// the methods that it modified - reverts are not symmetric with rejits.
65+
// See https://github.com/dotnet/runtime/issues/117823
66+
3567
return 100;
3668
}
3769

@@ -50,42 +82,48 @@ private static void TriggerRevert()
5082
[MethodImplAttribute(MethodImplOptions.NoInlining)]
5183
private static void TriggerInliningChain()
5284
{
53-
Console.WriteLine("TriggerInlining");
85+
TestWriteLine("TriggerInliningChain");
5486
// Test Inlining through another method
5587
InlineeChain1();
5688
}
5789

5890
[MethodImplAttribute(MethodImplOptions.NoInlining)]
5991
private static void TriggerDirectInlining()
6092
{
61-
Console.WriteLine("TriggerDirectInlining");
93+
TestWriteLine("TriggerDirectInlining");
6294
InlineeTarget();
6395
}
6496

6597
[MethodImplAttribute(MethodImplOptions.NoInlining)]
6698
private static void CallMethodWithoutInlining()
6799
{
68-
Console.WriteLine("CallMethodWithoutInlining");
69-
Action callMethod = InlineeTarget;
70-
callMethod();
100+
TestWriteLine("CallMethodWithoutInlining");
101+
Action<string> callMethod = InlineeTarget;
102+
callMethod("CallMethodWithoutInlining");
71103
}
72104

73105
[MethodImplAttribute(MethodImplOptions.AggressiveInlining)]
74106
private static void InlineeChain1()
75107
{
76-
Console.WriteLine("Inline.InlineeChain1");
108+
TestWriteLine(" Inline.InlineeChain1");
77109
InlineeTarget();
78110
}
79111

80112
[MethodImplAttribute(MethodImplOptions.AggressiveInlining)]
81-
public static void InlineeTarget()
113+
public static void InlineeTarget([CallerMemberName] string callerMemberName = null)
82114
{
83-
Console.WriteLine("Inline.InlineeTarget");
115+
var sb = new System.Text.StringBuilder();
116+
sb.Append(" Inline.InlineeTarget");
117+
sb.Append(' ');
118+
sb.Append('(');
119+
sb.Append(callerMemberName);
120+
sb.Append(')');
121+
TestWriteLine(sb.ToString());
84122
}
85123

86124
public static int RunTest(string[] args)
87125
{
88-
Console.WriteLine("maxinlining");
126+
TestWriteLine("maxinlining");
89127
return MaxInlining();
90128
}
91129

@@ -108,22 +146,22 @@ public class SeparateClassNeverLoaded
108146
[MethodImplAttribute(MethodImplOptions.NoInlining)]
109147
private static void TriggerInliningChain()
110148
{
111-
Console.WriteLine("TriggerInlining");
149+
RejitWithInlining.TestWriteLine("TriggerInlining");
112150
// Test Inlining through another method
113151
InlineeChain1();
114152
}
115153

116154
[MethodImplAttribute(MethodImplOptions.NoInlining)]
117155
private static void TriggerDirectInlining()
118156
{
119-
Console.WriteLine("TriggerDirectInlining");
157+
RejitWithInlining.TestWriteLine("TriggerDirectInlining");
120158
RejitWithInlining.InlineeTarget();
121159
}
122160

123161
[MethodImplAttribute(MethodImplOptions.AggressiveInlining)]
124162
private static void InlineeChain1()
125163
{
126-
Console.WriteLine("Inline.InlineeChain1");
164+
RejitWithInlining.TestWriteLine("Inline.InlineeChain1");
127165
RejitWithInlining.InlineeTarget();
128166
}
129167
}

0 commit comments

Comments
 (0)