This repository has been archived by the owner on Jan 23, 2023. It is now read-only.
WIP - Tiered Jitting Part Deux #12193
There are no files selected for viewing
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,29 @@ | ||
| # Code Versioning Profiler Breaking Changes # | ||
|
|
||
| The runtime changes done as part of the code versioning feature will cause some (hopefully minor) breaking changes to be visible via the profiler API. My goal is to advertise these coming changes and solicit feedback about what will be easiest for profiler writers to absorb. Currently this feature is only under development in the .Net Core version of the runtime. If you solely support a profiler on full .Net Framework this change doesn't affect you. | ||
|
|
||
| ## Underlying issue ## | ||
|
|
||
| Code versioning, and in particular its use for tiered compilation means that the runtime will be invoking the JIT more than it did in the past. Historically there was a 1:1 relationship between a FunctionID and a single JITCompilationFinished event. For those profilers using ReJIT APIs the 1:1 relationship was between a (FunctionID,ReJITID) pair and a single [Re]JITCompilationFinished event. [Re]JitCompilationStarted events were usually 1:1 as well, but not guaranteed in all cases. Tiered compilation will break these invariants by invoking the JIT potentially multiple times per method. | ||
|
|
||
| ## Likely ways you will see behavior change ## | ||
|
|
||
| 1. There will be more JITCompilation events, and potentially more ReJIT compilation events than there were before. | ||
|
There was a problem hiding this comment. If there are multiple JITCompilation events for the same method, is there any way of knowing the last if these (per method)? We currently pre-compute IL-code at ModuleLoadStarted, then keep it in memory until JITCompilationStarted, then exchange it and then delete it. If there will be subsequent JITCompilationStarted events, we cannot do this optimization anymore. Is it possible to know when there is the last-stage JIT event, after which there won't be any more. There was a problem hiding this comment. I assume you mean that you call ICorProfilerInfo::SetILFunctionBody to do that exchange? If you called this method the first time a method JITs (as you do today), the effect of this would persist automatically for all future jittings of that same method. If I understand you correctly the only thing you might need to change is to check in your JITCompilation handler if the IL code pointer is already NULL. If it is that means you already set the IL and deleted your copy in a previous event so you should avoid calling SetILFunctionBody again with your now deleted IL pointer. There was a problem hiding this comment. You assumed exactly right. Sounds like we will be fine. |
||
|
|
||
| 2. These JIT events may originate from a background worker thread that may be different from the thread which ultimately runs the jitted code. | ||
|
|
||
| 3. Calls to ICorProfilerInfo4::GetCodeInfo3 will only return information about the first jitted code body for a given FunctionID,rejitID pair. We'll need to create a new API to handle code bodies after the first. | ||
|
|
||
| 4. Calls to ICorProfilerInfo4::GetILToNativeMapping2 will only return information about the first jitted code body for a given FunctionID,rejitID pair. We'll need to create a new API to handle code bodies after the first. | ||
|
|
||
| 5. IL supplied during the JITCompilationStarted callback is now verified the same as if you had provided it during ModuleLoadFinished. | ||
|
|
||
|
|
||
| ## Obscure ways you might see behavior change ## | ||
|
|
||
| 1. If tiered compilation fails to publish an updated code body on a method that has already been instrumented with RequestReJIT and jitted, the profiler could receive a rejit error callback reporting the problem. This should only occur on OOM or process memory corruption. | ||
|
|
||
| 2. The timing of ReJITCompilationFinished has been adjusted to be slightly earlier (after the new code body is generated, but prior to updating the previous jitted code to modify control flow). This raises a slim possibility for a ReJIT error to be reported after ReJITCompilationFinished in the case of OOM or process memory corruption. | ||
|
|
||
|
|
||
| There are likely some other variations of the changed behavior I haven't thought of yet, but if further testing, code review, or discussion brings it to the surface I'll add it here. Feel free to get in touch on github (@noahfalk), or if you have anything you want to discuss in private you can email me at noahfalk AT microsoft.com | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -89,7 +89,7 @@ else(WIN32) | |
| add_definitions(-DFEATURE_ARRAYSTUB_AS_IL) | ||
| add_definitions(-DFEATURE_MULTICASTSTUB_AS_IL) | ||
| endif(WIN32) | ||
| add_definitions(-DFEATURE_CODE_VERSIONING) | ||
| add_definitions(-DFEATURE_COLLECTIBLE_TYPES) | ||
|
|
||
| if(WIN32) | ||
|
|
@@ -119,7 +119,6 @@ endif(CLR_CMAKE_PLATFORM_LINUX) | |
| if(CLR_CMAKE_PLATFORM_UNIX) | ||
| add_definitions(-DFEATURE_EVENTSOURCE_XPLAT=1) | ||
| endif(CLR_CMAKE_PLATFORM_UNIX) | ||
| # NetBSD doesn't implement this feature | ||
| if(NOT CMAKE_SYSTEM_NAME STREQUAL NetBSD) | ||
| add_definitions(-DFEATURE_HIJACK) | ||
|
|
@@ -136,6 +135,9 @@ if(FEATURE_INTERPRETER) | |
| add_definitions(-DFEATURE_INTERPRETER) | ||
| endif(FEATURE_INTERPRETER) | ||
| add_definitions(-DFEATURE_ISYM_READER) | ||
| if(CLR_CMAKE_TARGET_ARCH_AMD64 OR CLR_CMAKE_TARGET_ARCH_I386) | ||
| add_definitions(-DFEATURE_JUMPSTAMP) | ||
| endif(CLR_CMAKE_TARGET_ARCH_AMD64 OR CLR_CMAKE_TARGET_ARCH_I386) | ||
| add_definitions(-DFEATURE_LOADER_OPTIMIZATION) | ||
| if (CLR_CMAKE_PLATFORM_LINUX OR WIN32) | ||
| add_definitions(-DFEATURE_MANAGED_ETW) | ||
|
|
@@ -175,6 +177,7 @@ if (CLR_CMAKE_PLATFORM_UNIX OR CLR_CMAKE_TARGET_ARCH_ARM64) | |
| endif () | ||
| add_definitions(-DFEATURE_SVR_GC) | ||
| add_definitions(-DFEATURE_SYMDIFF) | ||
| add_definitions(-DFEATURE_TIERED_COMPILATION) | ||
|
There was a problem hiding this comment. Does it matter that FEATURE_REJIT is conditionally defined, but FEATURE_TIERED_COMPILATION is unconditionally defined? #Resolved There was a problem hiding this comment. Nope, this one is by design. ReJit depends on jumpstamps and jumpstamps aren't available on ARM. Tiered compilation on the other hand solely uses Precode so it should work on all architectures. In reply to: 127331986 [](ancestors = 127331986) |
||
| if (CLR_CMAKE_PLATFORM_ARCH_AMD64) | ||
| # Enable the AMD64 Unix struct passing JIT-EE interface for all AMD64 platforms, to enable altjit. | ||
| add_definitions(-DFEATURE_UNIX_AMD64_STRUCT_PASSING_ITF) | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -903,14 +903,15 @@ void DacDbiInterfaceImpl::GetNativeVarData(MethodDesc * pMethodDesc, | |
| // pEntryCount is the number of valid entries in nativeMap, and it may be adjusted downwards | ||
| // as part of the composition. | ||
| //----------------------------------------------------------------------------- | ||
| void DacDbiInterfaceImpl::ComposeMapping(const InstrumentedILOffsetMapping * pProfilerILMap, ICorDebugInfo::OffsetMapping nativeMap[], ULONG32* pEntryCount) | ||
| { | ||
| // Translate the IL offset if the profiler has provided us with a mapping. | ||
| // The ICD public API should always expose the original IL offsets, but GetBoundaries() | ||
| // directly accesses the debug info, which stores the instrumented IL offsets. | ||
|
|
||
| ULONG32 entryCount = *pEntryCount; | ||
| // The map pointer could be NULL or there could be no entries in the map, in either case no work to do | ||
| if (pProfilerILMap && !pProfilerILMap->IsNull()) | ||
| { | ||
| // If we did instrument, then we can't have any sequence points that | ||
| // are "in-between" the old-->new map that the profiler gave us. | ||
|
|
@@ -925,7 +926,7 @@ void DacDbiInterfaceImpl::ComposeMapping(InstrumentedILOffsetMapping profilerILM | |
| ULONG32 prevILOffset = (ULONG32)(ICorDebugInfo::MAX_ILNUM); | ||
| for (ULONG32 i = 0; i < entryCount; i++) | ||
| { | ||
| ULONG32 origILOffset = TranslateInstrumentedILOffsetToOriginal(nativeMap[i].ilOffset, pProfilerILMap); | ||
|
|
||
| if (origILOffset == prevILOffset) | ||
| { | ||
|
|
@@ -1003,20 +1004,20 @@ void DacDbiInterfaceImpl::GetSequencePoints(MethodDesc * pMethodDesc, | |
|
|
||
| // if there is a rejit IL map for this function, apply that in preference to load-time mapping | ||
| #ifdef FEATURE_REJIT | ||
| CodeVersionManager * pCodeVersionManager = pMethodDesc->GetCodeVersionManager(); | ||
| NativeCodeVersion nativeCodeVersion = pCodeVersionManager->GetNativeCodeVersion(dac_cast<PTR_MethodDesc>(pMethodDesc), (PCODE)startAddr); | ||
| if (!nativeCodeVersion.IsNull()) | ||
| { | ||
| const InstrumentedILOffsetMapping * pRejitMapping = nativeCodeVersion.GetILCodeVersion().GetInstrumentedILMap(); | ||
| ComposeMapping(pRejitMapping, mapCopy, &entryCount); | ||
| } | ||
| else | ||
| { | ||
| #endif | ||
| // if there is a profiler load-time mapping and not a rejit mapping, apply that instead | ||
| InstrumentedILOffsetMapping loadTimeMapping = | ||
| pMethodDesc->GetModule()->GetInstrumentedILOffsetMapping(pMethodDesc->GetMemberDef()); | ||
| ComposeMapping(&loadTimeMapping, mapCopy, &entryCount); | ||
| #ifdef FEATURE_REJIT | ||
| } | ||
| #endif | ||
|
|
@@ -7149,73 +7150,113 @@ HRESULT DacDbiInterfaceImpl::GetPEFileMDInternalRW(VMPTR_PEFile vmPEFile, OUT TA | |
| HRESULT DacDbiInterfaceImpl::GetReJitInfo(VMPTR_Module vmModule, mdMethodDef methodTk, OUT VMPTR_ReJitInfo* pvmReJitInfo) | ||
| { | ||
| DD_ENTER_MAY_THROW; | ||
| _ASSERTE(!"You shouldn't be calling this - use GetActiveRejitILCodeVersionNode instead"); | ||
| return S_OK; | ||
| } | ||
|
|
||
| HRESULT DacDbiInterfaceImpl::GetActiveRejitILCodeVersionNode(VMPTR_Module vmModule, mdMethodDef methodTk, OUT VMPTR_ILCodeVersionNode* pVmILCodeVersionNode) | ||
| { | ||
| DD_ENTER_MAY_THROW; | ||
| if (pVmILCodeVersionNode == NULL) | ||
| return E_INVALIDARG; | ||
| #ifdef FEATURE_REJIT | ||
| PTR_Module pModule = vmModule.GetDacPtr(); | ||
| CodeVersionManager * pCodeVersionManager = pModule->GetCodeVersionManager(); | ||
| // Be careful, there are two different definitions of 'active' being used here | ||
| // For the CodeVersionManager, the active IL version is whatever one should be used in the next invocation of the method | ||
| // 'rejit active' narrows that to only include rejit IL bodies where the profiler has already provided the definition | ||
| // for the new IL (ilCodeVersion.GetRejitState()==ILCodeVersion::kStateActive). It is possible that the code version | ||
| // manager's active IL version hasn't yet asked the profiler for the IL body to use, in which case we want to filter it | ||
| // out from the return in this method. | ||
| ILCodeVersion activeILVersion = pCodeVersionManager->GetActiveILCodeVersion(pModule, methodTk); | ||
| if (activeILVersion.IsNull() || activeILVersion.GetRejitState() != ILCodeVersion::kStateActive) | ||
|
There was a problem hiding this comment. Out of curiosity, do we actually need to differentiate between rejit and non-rejit IL? I like the plan that you're on here of taking the rejit code and making it more general purpose, but would like it even more if we didn't have to have a concept of rejit specific storage. If I think about this a bit more, perhaps you're using the term rejit here to represent the fact that we might not be talking about IL that is in an assembly? #Resolved There was a problem hiding this comment. ICorDebug does have 1st class knowledge of ReJIT IL via APIs like ICorDebugFunction3::GetActiveReJitRequestILCode, so we do need to distinguish. At the moment ReJit is the only thing that can change the IL from its original state in the assembly, but if we wanted to add other IL modifying capabilities in the future I think we would need rethink whether treating each case specially was the right thing to do. The reason the debugger cares about ReJit IL is that the debugger API can be operated by the same component that is operating the profiler API. It is legal to have the profiler create new IL code that has new local variables, then the debugger views a dump of that code and assigns special meaning to profiler added locals. In reply to: 127300842 [](ancestors = 127300842) |
||
| { | ||
| pVmILCodeVersionNode->SetDacTargetPtr(0); | ||
| } | ||
| else | ||
| { | ||
| pVmILCodeVersionNode->SetDacTargetPtr(PTR_TO_TADDR(activeILVersion.AsNode())); | ||
| } | ||
| #else | ||
| _ASSERTE(!"You shouldn't be calling this - rejit is not supported in this build"); | ||
| pVmILCodeVersionNode->SetDacTargetPtr(0); | ||
| #endif | ||
| return S_OK; | ||
| } | ||
|
|
||
| HRESULT DacDbiInterfaceImpl::GetReJitInfo(VMPTR_MethodDesc vmMethod, CORDB_ADDRESS codeStartAddress, OUT VMPTR_ReJitInfo* pvmReJitInfo) | ||
| { | ||
| DD_ENTER_MAY_THROW; | ||
| _ASSERTE(!"You shouldn't be calling this - use GetNativeCodeVersionNode instead"); | ||
| return S_OK; | ||
| } | ||
|
|
||
| HRESULT DacDbiInterfaceImpl::GetNativeCodeVersionNode(VMPTR_MethodDesc vmMethod, CORDB_ADDRESS codeStartAddress, OUT VMPTR_NativeCodeVersionNode* pVmNativeCodeVersionNode) | ||
| { | ||
| DD_ENTER_MAY_THROW; | ||
| if (pVmNativeCodeVersionNode == NULL) | ||
| return E_INVALIDARG; | ||
| #ifdef FEATURE_REJIT | ||
| PTR_MethodDesc pMD = vmMethod.GetDacPtr(); | ||
| CodeVersionManager * pCodeVersionManager = pMD->GetCodeVersionManager(); | ||
| NativeCodeVersion codeVersion = pCodeVersionManager->GetNativeCodeVersion(pMD, (PCODE)codeStartAddress); | ||
| pVmNativeCodeVersionNode->SetDacTargetPtr(PTR_TO_TADDR(codeVersion.AsNode())); | ||
| #else | ||
| pVmNativeCodeVersionNode->SetDacTargetPtr(0); | ||
| #endif | ||
| return S_OK; | ||
| } | ||
|
|
||
| HRESULT DacDbiInterfaceImpl::GetSharedReJitInfo(VMPTR_ReJitInfo vmReJitInfo, OUT VMPTR_SharedReJitInfo* pvmSharedReJitInfo) | ||
| { | ||
| DD_ENTER_MAY_THROW; | ||
| _ASSERTE(!"You shouldn't be calling this - use GetLCodeVersionNode instead"); | ||
| return S_OK; | ||
| } | ||
|
|
||
| HRESULT DacDbiInterfaceImpl::GetILCodeVersionNode(VMPTR_NativeCodeVersionNode vmNativeCodeVersionNode, VMPTR_ILCodeVersionNode* pVmILCodeVersionNode) | ||
| { | ||
| DD_ENTER_MAY_THROW; | ||
| if (pVmILCodeVersionNode == NULL) | ||
| return E_INVALIDARG; | ||
| #ifdef FEATURE_REJIT | ||
| NativeCodeVersionNode* pNativeCodeVersionNode = vmNativeCodeVersionNode.GetDacPtr(); | ||
| pVmILCodeVersionNode->SetDacTargetPtr(PTR_TO_TADDR(pNativeCodeVersionNode->GetILCodeVersion().AsNode())); | ||
| #else | ||
| _ASSERTE(!"You shouldn't be calling this - rejit is not supported in this build"); | ||
| pVmILCodeVersionNode->SetDacTargetPtr(0); | ||
| #endif | ||
| return S_OK; | ||
| } | ||
|
|
||
| HRESULT DacDbiInterfaceImpl::GetSharedReJitInfoData(VMPTR_SharedReJitInfo vmSharedReJitInfo, DacSharedReJitInfo* pData) | ||
| { | ||
| DD_ENTER_MAY_THROW; | ||
| _ASSERTE(!"You shouldn't be calling this - use GetILCodeVersionNodeData instead"); | ||
| return S_OK; | ||
| } | ||
|
|
||
| HRESULT DacDbiInterfaceImpl::GetILCodeVersionNodeData(VMPTR_ILCodeVersionNode vmILCodeVersionNode, DacSharedReJitInfo* pData) | ||
| { | ||
| DD_ENTER_MAY_THROW; | ||
| #ifdef FEATURE_REJIT | ||
| ILCodeVersionNode* pILCodeVersionNode = vmILCodeVersionNode.GetDacPtr(); | ||
| pData->m_state = pILCodeVersionNode->GetRejitState(); | ||
| pData->m_pbIL = PTR_TO_CORDB_ADDRESS(dac_cast<ULONG_PTR>(pILCodeVersionNode->GetIL())); | ||
| pData->m_dwCodegenFlags = pILCodeVersionNode->GetJitFlags(); | ||
| const InstrumentedILOffsetMapping* pMapping = pILCodeVersionNode->GetInstrumentedILMap(); | ||
| if (pMapping) | ||
| { | ||
| pData->m_cInstrumentedMapEntries = (ULONG)pMapping->GetCount(); | ||
| pData->m_rgInstrumentedMapEntries = PTR_TO_CORDB_ADDRESS(dac_cast<ULONG_PTR>(pMapping->GetOffsets())); | ||
| } | ||
| else | ||
| { | ||
| pData->m_cInstrumentedMapEntries = 0; | ||
| pData->m_rgInstrumentedMapEntries = 0; | ||
| } | ||
| #else | ||
| _ASSERTE(!"You shouldn't be calling this - rejit isn't supported in this build"); | ||
| #endif | ||
| return S_OK; | ||
| } | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Can you mention code map? Are you fixing those for reJIT cases?
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.
Thanks, I had missed this one, but yes the issue is similar to GetCodeInfo3 where the API only expects one code body. As you point out the API is already somewhat broken because it doesn't handle ReJIT code bodies either.
As a tentative proposal what if we create GetILToNativeMapping3 which takes a code start address rather than a FunctionID? It will take a little research to confirm, but I don't think there should be any significant obstacle to making it work correctly for all code bodies.
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.
So you saying that code map is broken after tiered re jit? Adding method is fine I think
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.
Yes, the end to end workflow would be broken. GetILToNativeMapping will give you a valid IL to Native map for the first code body which is jitted. However if the same method JITs a second time, the second code body may have a different native to il mapping than the first. The current API provides you no mechanism to access the native to il map that corresponds to that second code body.
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.
it is actually two questions in one. First, just want to confirm that exception stack will show the proper line number after tiered re-JIT optimized the method body. Second, can fixing of code map for re-JITed by profiler methods can be included in scope of this work.
For the second one - if it requires a new method - it is fine. Just allow to do it.
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.
For the Exception.StackTrace issue that you mentioned I'm keeping it separate from this work. If you only use tiered jitting and never call RequestReJIT in the profiler then exception stack traces will work properly. If you do call RequestReJIT then the pre-existing bug may give you incorrect StackTrace results and that might still exist after this work is complete.
Yes, I will include fixing GetILToNativeMapping in this work. It may not be in this checkin, but I'll get a solution in place before tiered jitting is ever enabled by default.