Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
Profiler RequestReJITWithInliners did not work properly if all method…
…s in the module were not loaded (#69634)

- Add unused functions into rejit.cs test to expose the problematic behavior
- Fix by using tokens throughout the handling of R2R inlining information instead of converting to MethodDescs at times
  • Loading branch information
davidwrighton committed May 21, 2022
1 parent c190f6b commit 9a5387b
Show file tree
Hide file tree
Showing 5 changed files with 81 additions and 39 deletions.
53 changes: 27 additions & 26 deletions src/coreclr/vm/rejit.cpp
Expand Up @@ -420,7 +420,6 @@ COR_IL_MAP* ProfilerFunctionControl::GetInstrumentedMapEntries()
#ifndef DACCESS_COMPILE
NativeImageInliningIterator::NativeImageInliningIterator() :
m_pModule(NULL),
m_pInlinee(NULL),
m_dynamicBuffer(NULL),
m_dynamicBufferSize(0),
m_dynamicAvailable(0),
Expand All @@ -429,21 +428,21 @@ NativeImageInliningIterator::NativeImageInliningIterator() :

}

HRESULT NativeImageInliningIterator::Reset(Module *pModule, MethodDesc *pInlinee)
HRESULT NativeImageInliningIterator::Reset(Module *pInlinerModule, MethodInModule inlinee)
{
_ASSERTE(pModule != NULL);
_ASSERTE(pInlinee != NULL);
_ASSERTE(pInlinerModule != NULL);
_ASSERTE(inlinee.m_module != NULL);

m_pModule = pModule;
m_pInlinee = pInlinee;
m_pModule = pInlinerModule;
m_inlinee = inlinee;

HRESULT hr = S_OK;
EX_TRY
{
// Trying to use the existing buffer
BOOL incompleteData;
Module *inlineeModule = m_pInlinee->GetModule();
mdMethodDef mdInlinee = m_pInlinee->GetMemberDef();
Module *inlineeModule = m_inlinee.m_module;
mdMethodDef mdInlinee = m_inlinee.m_methodDef;
COUNT_T methodsAvailable = m_pModule->GetReadyToRunInliners(inlineeModule, mdInlinee, m_dynamicBufferSize, m_dynamicBuffer, &incompleteData);

// If the existing buffer is not large enough, reallocate.
Expand Down Expand Up @@ -484,19 +483,16 @@ BOOL NativeImageInliningIterator::Next()
return m_currentPos < m_dynamicAvailable;
}

MethodDesc *NativeImageInliningIterator::GetMethodDesc()
MethodInModule NativeImageInliningIterator::GetMethod()
{
// this evaluates true when m_currentPos == s_failurePos or m_currentPos == (COUNT_T)-1
// m_currentPos is an unsigned type
if (m_currentPos >= m_dynamicAvailable)
{
return NULL;
return MethodInModule();
}

MethodInModule mm = m_dynamicBuffer[m_currentPos];
Module *pModule = mm.m_module;
mdMethodDef mdInliner = mm.m_methodDef;
return pModule->LookupMethodDef(mdInliner);
return m_dynamicBuffer[m_currentPos];
}

//---------------------------------------------------------------------------------------
Expand Down Expand Up @@ -623,16 +619,20 @@ HRESULT ReJitManager::UpdateActiveILVersions(

if ((flags & COR_PRF_REJIT_BLOCK_INLINING) == COR_PRF_REJIT_BLOCK_INLINING)
{
hr = UpdateNativeInlinerActiveILVersions(&mgrToCodeActivationBatch, pMD, fIsRevert, flags);
hr = UpdateNativeInlinerActiveILVersions(&mgrToCodeActivationBatch, pModule, rgMethodDefs[i], fIsRevert, flags);
if (FAILED(hr))
{
return hr;
}

hr = UpdateJitInlinerActiveILVersions(&mgrToCodeActivationBatch, pMD, fIsRevert, flags);
if (FAILED(hr))
if (pMD != NULL)
{
return hr;
// If pMD is not null, then the method may have already been inlined somewhere. Go check.
hr = UpdateJitInlinerActiveILVersions(&mgrToCodeActivationBatch, pMD, fIsRevert, flags);
if (FAILED(hr))
{
return hr;
}
}
}
} // for (ULONG i = 0; i < cFunctions; i++)
Expand Down Expand Up @@ -780,7 +780,8 @@ HRESULT ReJitManager::UpdateActiveILVersion(
// static
HRESULT ReJitManager::UpdateNativeInlinerActiveILVersions(
SHash<CodeActivationBatchTraits> *pMgrToCodeActivationBatch,
MethodDesc *pInlinee,
Module *pInlineeModule,
mdMethodDef inlineeMethodDef,
BOOL fIsRevert,
COR_PRF_REJIT_FLAGS flags)
{
Expand All @@ -794,7 +795,8 @@ HRESULT ReJitManager::UpdateNativeInlinerActiveILVersions(
CONTRACTL_END;

_ASSERTE(pMgrToCodeActivationBatch != NULL);
_ASSERTE(pInlinee != NULL);
_ASSERTE(pInlineeModule != NULL);
_ASSERTE(RidFromToken(inlineeMethodDef) != 0);

HRESULT hr = S_OK;

Expand All @@ -812,16 +814,15 @@ HRESULT ReJitManager::UpdateNativeInlinerActiveILVersions(
Module * pModule = pDomainAssembly->GetModule();
if (pModule->HasReadyToRunInlineTrackingMap())
{
inlinerIter.Reset(pModule, pInlinee);
inlinerIter.Reset(pModule, MethodInModule(pInlineeModule, inlineeMethodDef));

MethodDesc *pInliner = NULL;
while (inlinerIter.Next())
{
pInliner = inlinerIter.GetMethodDesc();
MethodInModule inliner = inlinerIter.GetMethod();
{
CodeVersionManager *pCodeVersionManager = pModule->GetCodeVersionManager();
CodeVersionManager::LockHolder codeVersioningLockHolder;
ILCodeVersion ilVersion = pCodeVersionManager->GetActiveILCodeVersion(pInliner);
ILCodeVersion ilVersion = pCodeVersionManager->GetActiveILCodeVersion(inliner.m_module, inliner.m_methodDef);
if (!ilVersion.HasDefaultIL())
{
// This method has already been ReJITted, no need to request another ReJIT at this point.
Expand All @@ -830,10 +831,10 @@ HRESULT ReJitManager::UpdateNativeInlinerActiveILVersions(
}
}

hr = UpdateActiveILVersion(pMgrToCodeActivationBatch, pInliner->GetModule(), pInliner->GetMemberDef(), fIsRevert, flags);
hr = UpdateActiveILVersion(pMgrToCodeActivationBatch, inliner.m_module, inliner.m_methodDef, fIsRevert, flags);
if (FAILED(hr))
{
ReportReJITError(pInliner->GetModule(), pInliner->GetMemberDef(), NULL, hr);
ReportReJITError(inliner.m_module, inliner.m_methodDef, NULL, hr);
}
}
}
Expand Down
9 changes: 5 additions & 4 deletions src/coreclr/vm/rejit.h
Expand Up @@ -75,13 +75,13 @@ class NativeImageInliningIterator
public:
NativeImageInliningIterator();

HRESULT Reset(Module *pInlineeModule, MethodDesc *pInlinee);
HRESULT Reset(Module* pInlinerModule, MethodInModule inlinee);
BOOL Next();
MethodDesc *GetMethodDesc();
MethodInModule GetMethod();

private:
Module *m_pModule;
MethodDesc *m_pInlinee;
MethodInModule m_inlinee;
NewArrayHolder<MethodInModule> m_dynamicBuffer;
COUNT_T m_dynamicBufferSize;
COUNT_T m_dynamicAvailable;
Expand Down Expand Up @@ -189,7 +189,8 @@ class ReJitManager

static HRESULT UpdateNativeInlinerActiveILVersions(
SHash<CodeActivationBatchTraits> *pMgrToCodeActivationBatch,
MethodDesc *pInlinee,
Module *pInlineeModule,
mdMethodDef inlineeMethodDef,
BOOL fIsRevert,
COR_PRF_REJIT_FLAGS flags);

Expand Down
29 changes: 22 additions & 7 deletions src/tests/profiler/native/rejitprofiler/rejitprofiler.cpp
Expand Up @@ -287,13 +287,20 @@ HRESULT STDMETHODCALLTYPE ReJITProfiler::JITCachedFunctionSearchFinished(Functio
COR_PRF_METHOD method;
while (pEnum->Next(1, &method, NULL) == S_OK)
{
FunctionID inlinerFuncId = GetFunctionIDFromToken(method.moduleId, method.methodId);
FunctionID inlinerFuncId = GetFunctionIDFromToken(method.moduleId, method.methodId, true);

// GetFunctionIDFromToken doesn't handle generics, will return NULL
// GetFunctionIDFromToken doesn't handle generics or not yet loaded methods, will return NULL
if (inlinerFuncId != mdTokenNil)
{
AddInlining(inlinerFuncId, functionId);
}
else
{
String calleeName = GetFunctionIDName(functionId);
String moduleName = GetModuleIDName(GetModuleIDForFunction(functionId));
String inlinerModuleId = GetModuleIDName(method.moduleId);
INFO(L"Inlining occurred, but name could not be resolved! Inliner=ModuleId=" << inlinerModuleId << L" Token=" << std::hex << method.methodId << L", Inlinee=" << calleeName << L" Inlinee module name=" << moduleName);
}
}
}

Expand All @@ -313,7 +320,7 @@ HRESULT STDMETHODCALLTYPE ReJITProfiler::GetReJITParameters(ModuleID moduleId, m
{
SHUTDOWNGUARD();

INFO(L"Starting to build IL for method " << GetFunctionIDName(GetFunctionIDFromToken(moduleId, methodId)));
INFO(L"Starting to build IL for method " << GetFunctionIDName(GetFunctionIDFromToken(moduleId, methodId, false)));
COMPtrHolder<IUnknown> pUnk;
HRESULT hr = _profInfo10->GetModuleMetaData(moduleId, ofWrite, IID_IMetaDataEmit2, &pUnk);
if (FAILED(hr))
Expand Down Expand Up @@ -444,13 +451,21 @@ void ReJITProfiler::AddInlining(FunctionID inliner, FunctionID inlinee)
INFO(L"Inlining occurred! Inliner=" << GetFunctionIDName(inliner) << L" Inlinee=" << calleeName << L" Inlinee module name=" << moduleName);
}

FunctionID ReJITProfiler::GetFunctionIDFromToken(ModuleID module, mdMethodDef token)
FunctionID ReJITProfiler::GetFunctionIDFromToken(ModuleID module, mdMethodDef token, bool invalidArgNotFailure)
{
HRESULT hr = S_OK;
FunctionID functionId;
if (FAILED(hr = pCorProfilerInfo->GetFunctionFromToken(module,
token,
&functionId)))
hr = pCorProfilerInfo->GetFunctionFromToken(module,
token,
&functionId);

if (invalidArgNotFailure && hr == E_INVALIDARG)
{
printf("Call to GetFunctionFromToken failed with E_INVALIDARG, this may be caused by the method not yet being loaded\n");
return mdTokenNil;
}

if (FAILED(hr))
{
printf("Call to GetFunctionFromToken failed with hr=0x%x\n", hr);
_failures++;
Expand Down
2 changes: 1 addition & 1 deletion src/tests/profiler/native/rejitprofiler/rejitprofiler.h
Expand Up @@ -48,7 +48,7 @@ class ReJITProfiler : public Profiler

bool FunctionSeen(FunctionID func);

FunctionID GetFunctionIDFromToken(ModuleID module, mdMethodDef token);
FunctionID GetFunctionIDFromToken(ModuleID module, mdMethodDef token, bool invalidArgNotFailure);
mdMethodDef GetMethodDefForFunction(FunctionID functionId);
ModuleID GetModuleIDForFunction(FunctionID functionId);

Expand Down
27 changes: 26 additions & 1 deletion src/tests/profiler/rejit/rejit.cs
Expand Up @@ -78,7 +78,7 @@ private static void InlineeChain1()
}

[MethodImplAttribute(MethodImplOptions.AggressiveInlining)]
private static void InlineeTarget()
public static void InlineeTarget()
{
Console.WriteLine("Inline.InlineeTarget");
}
Expand All @@ -102,4 +102,29 @@ public static int Main(string[] args)
profileeOptions: ProfileeOptions.OptimizationSensitive);
}
}

public class SeparateClassNeverLoaded
{
[MethodImplAttribute(MethodImplOptions.NoInlining)]
private static void TriggerInliningChain()
{
Console.WriteLine("TriggerInlining");
// Test Inlining through another method
InlineeChain1();
}

[MethodImplAttribute(MethodImplOptions.NoInlining)]
private static void TriggerDirectInlining()
{
Console.WriteLine("TriggerDirectInlining");
RejitWithInlining.InlineeTarget();
}

[MethodImplAttribute(MethodImplOptions.AggressiveInlining)]
private static void InlineeChain1()
{
Console.WriteLine("Inline.InlineeChain1");
RejitWithInlining.InlineeTarget();
}
}
}

0 comments on commit 9a5387b

Please sign in to comment.