Make ETW rundown operate more effectively with JIT #116354

AaronRobinsonMSFT · 2025-06-05T22:41:11Z

Make the EECodeGenManager API surface narrower.
Rename some locals removing critical section notion and replace with lock.

Rework the CodeHeapIterator.
Rename members to more accurately reflect their purpose. Generally move to more canonical C++.

EECodeGenManager now has an iterator counter. This is used to track the number of outstanding iterators and can be used to defer deletes. Adds are still valid since the iterator will only operate on the state at the time the iterator was created.

Always emit UnwindInfo for methods. This was previously only enabled when the Runtime provider was enabled and a rundown triggered.

Rename some locals removing critical section notion and replace with lock. Rework the CodeHeapIterator. Rename members to more accurately reflect their purpose. Generally move to more canonical C++. EECodeGenManager now has a iterator counter. This is used to track the number of outstanding iterators and can be used to defer deletes. Adds are still valid since the iterator will only operate on the state at the time the iterator was created.

src/coreclr/vm/class.cpp

Copilot

Pull Request Overview

This PR refines the ETW rundown and JIT-related code by narrowing the EECodeGenManager API, renaming variables and functions for improved clarity, and reworking the CodeHeapIterator. It also updates logging formats, modifies type usage for iteration counters, and adjusts several low‐level components to support the new design.

Refactored API calls (e.g. AllocCode, AllocFromJitMetaHeap) and adapted braced initialization for RemoveJitData.
Renamed members (e.g. m_pChunks → m_pChunksBegin) and updated iteration variable types (USHORT → INT32) for consistency.
Improved log formatting and introduced move semantics in CrstHolder for better concurrency safety.

Reviewed Changes

Copilot reviewed 23 out of 23 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
src/coreclr/vm/jitinterface.cpp	Updated API calls and braced initialization for RemoveJitData.
src/coreclr/vm/excep.cpp	Changed iteration counter types from USHORT to INT32.
src/coreclr/vm/eventtrace.cpp	Modified event logging flow and adjusted the code heap iterator usage.
src/coreclr/vm/dynamicmethod.{h,cpp}	Added new setters and updated LOG formatting.
src/coreclr/vm/crst.h	Added move semantics to CrstHolder for improved lock management.
src/coreclr/vm/codeman.h	Renamed members and updated virtual destructors to default.
src/coreclr/vm/class.{inl,h,cpp}	Renamed chunk pointers to improve clarity of the method list.
src/coreclr/utilcode/{pedecoder.cpp,pedecoder.h}	Adjusted MethodSectionIterator to use non-const pointers.
src/coreclr/inc/utilcode.h	Updated the type for Count() from USHORT to INT32.
src/coreclr/inc/contract.inl	Revised error message text for tracking failures.
src/coreclr/debug/daccess/*	Updated references to use the new m_pAllCodeHeaps naming.

Comments suppressed due to low confidence (6)

src/coreclr/vm/dynamicmethod.cpp:876

The updated LOG formatting now uses '%zx' for size_t values; please verify that this format specifier is supported on all target platforms.

LOG((LF_BCL, LL_INFO1000, "Level3 - CodeHeap released [%p, vt(0x%zx)] - ref count %d\n", this, *(size_t*)this, m_AllocationCount));

src/coreclr/utilcode/pedecoder.cpp:2581

The removal of const qualifiers from the MethodSectionIterator parameters changes the method's contract; please confirm that allowing mutation on these pointers is intentional and safe for the overall design.

MethodSectionIterator::MethodSectionIterator(void *code, SIZE_T codeSize, void *codeTable, SIZE_T codeTableSize)

src/coreclr/inc/contract.inl:588

The updated error message provides clearer information regarding missing tracking data; please ensure that this change is consistently reflected in related documentation and error handling conventions.

strcat_s(Buf,ARRAY_SIZE(Buf), "Missing tracking information. Look for data structures that manipulate contract state (i.e., CrstHolder).\n");

src/coreclr/vm/jitinterface.cpp:11044

The use of braced initialization for RemoveJitData is nontraditional; please double-check that overload resolution works as intended across all build configurations.

jitMgr->RemoveJitData({pCodeHeader, m_GCinfo_len, m_EHinfo_len});

src/coreclr/inc/utilcode.h:1095

Changing the return type of Count() from USHORT to INT32 is fine, but please ensure that all dependent functions and loops consuming this value are updated to handle INT32 appropriately.

INT32 Count()

src/coreclr/vm/crst.h:371

The introduction of move semantics for CrstHolder is a good improvement; please verify that the new move constructor and assignment operator correctly preserve lock invariants in all multi-threaded scenarios.

CrstHolder(CrstHolder&& other)

jkotas

Test failures look related:

20:03:09.305 Running test: tracing/runtimeeventsource/nativeruntimeeventsource/nativeruntimeeventsource.cmd
ASSERT FAILED
	Expression: codeLength > 0
	Location:   line 1866 in /Users/runner/work/1/s/src/coreclr/vm/eetwain.cpp

src/coreclr/vm/profilingenumerators.cpp

src/coreclr/vm/crst.h

Narrow contract

AaronRobinsonMSFT · 2025-06-08T23:20:26Z

Test failures look related:

20:03:09.305 Running test: tracing/runtimeeventsource/nativeruntimeeventsource/nativeruntimeeventsource.cmd
ASSERT FAILED
	Expression: codeLength > 0
	Location:   line 1866 in /Users/runner/work/1/s/src/coreclr/vm/eetwain.cpp

Yep. I'm trying to root it out on macOS. I tried on Windows and did find a contract violation that I needed to fix. I pushed up that fix and will see where we are.

src/coreclr/vm/runtimehandles.cpp

src/coreclr/vm/codeman.cpp

possible due to MethodDesc iteration. Rework how DynamicMethodDesc and LCGMethodResolver destruction works. This was needed due to Crst lock inversion. Remove CrstIbcProfile

src/coreclr/vm/class.h

src/coreclr/vm/codeman.cpp

src/coreclr/vm/threads.cpp

Fixup Crst relationship of WrapperTemplate and ExecutableAllocatorLock.

AaronRobinsonMSFT · 2025-06-14T15:59:03Z

@jkotas I think this should be ready to review now.

src/coreclr/vm/ceemain.cpp

src/coreclr/vm/codeman.cpp

jkotas · 2025-06-28T03:52:42Z

src/coreclr/vm/finalizerthread.cpp

+    Thread* finalizerThread = GetFinalizerThread();
+    return finalizerThread->RequireSyncBlockCleanup()
+        || SystemDomain::System()->RequireAppDomainCleanup()
+        || (finalizerThread->m_DetachCount > 0)


Suggested change

|| (finalizerThread->m_DetachCount > 0)

|| (Thread::m_DetachCount > 0)

m_DetachCount is static

jkotas · 2025-06-28T03:52:54Z

src/coreclr/vm/finalizerthread.cpp

+        SystemDomain::System()->ProcessDelayedUnloadLoaderAllocators();
+    }
+
+    if (finalizerThread->m_DetachCount > 0


Suggested change

if (finalizerThread->m_DetachCount > 0

if (Thread::m_DetachCount > 0

jkotas · 2025-06-28T04:08:09Z

src/coreclr/vm/threads.cpp

@@ -6790,70 +6790,13 @@ T_CONTEXT *Thread::GetFilterContext(void)

 void Thread::ClearContext()
 {
-    CONTRACTL {


(not strictly related to this PR) Thread::ClearContext can be deleted. I do not see why we need to set `m_fDisableComObjectEagerCleanup = false; in the two places where it is called: One is Thread destructor - clearly useless there, and the other is dead thread during shutdown - no RCWs should be created in that case either.

jkotas · 2025-06-28T04:12:37Z

src/coreclr/vm/codeman.h

-// On non X86 platforms, the OS defined UnwindInfo (accessed from RUNTIME_FUNCTION
-// structures) to  support the ability unwind the stack.   Unfortunatey the pre-Win8
+typedef DPTR(class UnwindInfoTable) PTR_UnwindInfoTable;
+// On Windows non-x64 platforms, the OS defined UnwindInfo (accessed from RUNTIME_FUNCTION


Suggested change

// On Windows non-x64 platforms, the OS defined UnwindInfo (accessed from RUNTIME_FUNCTION

// On Windows x64, publish OS defined UnwindInfo (accessed from RUNTIME_FUNCTION

jkotas · 2025-06-28T14:32:59Z

src/coreclr/vm/finalizerthread.cpp

+        STANDARD_VM_CONTRACT;
+        _ASSERTE(pDMD != NULL);
+
+        s_delayDestroyDynamicMethodList.LinkHead(new SimpleList<DynamicMethodDesc*>::Node(pDMD));


Allocating to destroy an item tends to be cause trouble in corner-cases.

Would it be better to maintain this linked list without allocations? There is convenient LCGMethodResolver::m_next that we can reuse for this link list.

jkotas · 2025-06-28T15:18:43Z

src/coreclr/vm/profilingenumerators.cpp

+            // On AMD64 JumpStub is used to call functions that is 2GB away.  JumpStubs have a CodeHeader
+            // with NULL MethodDesc, are stored in code heap and are reported by EEJitManager::EnumCode.


Suggested change

// On AMD64 JumpStub is used to call functions that is 2GB away. JumpStubs have a CodeHeader

// with NULL MethodDesc, are stored in code heap and are reported by EEJitManager::EnumCode.

// Stubs (see StubCodeBlockKind) have no MethodDesc. Skip them.

jkotas · 2025-06-28T15:29:05Z

src/coreclr/vm/codeman.cpp


-#ifndef TARGET_UNIX
-    if (!RtlUnwindFtnsInited)
+#ifdef TARGET_WINDOWS


Suggested change

#ifdef TARGET_WINDOWS

Redundant ifdef

jkotas · 2025-06-28T15:29:15Z

src/coreclr/vm/codeman.cpp

    }
+#endif // TARGET_WINDOWS


Suggested change

#endif // TARGET_WINDOWS

jkotas · 2025-06-28T15:41:17Z

src/coreclr/vm/codeman.h

+    static void Initialize();
+
+#if defined(TARGET_AMD64) && defined(TARGET_WINDOWS)
+private:
    // These are lower level functions that assume you have found the list of UnwindInfoTable entries
    // These are used by the stublinker and the high-level method functions above


Suggested change

// These are used by the stublinker and the high-level method functions above

// These are used by the high-level method functions above

Stublinker does not use these anymore

jkotas · 2025-06-28T16:24:19Z

src/coreclr/vm/codeman.cpp

+        EX_CATCH
+        {
+            MethodDesc* pMD = arg.GetMethodDesc();
+            LOG((LF_JIT, LL_ERROR, "Failed to delay RemoveJitData for method, %s::%s\n", pMD->m_pszDebugClassName, pMD->m_pszDebugMethodName));


I am not sure whether dealing with RemoveJitData is worth the trouble:

For !IsLCGMethod case, the backout for GetJitMetaHeap looks like overengineering given that we are not backing out the code itself that is by far the largest chunk of memory. I think it would be fine to delete GetJitMetaHeap backout logic to keep this simpler.

It leaves is with the IsLCGMethod case. If we hit this OOM path, we will end up with code allocated for DynamicMethod method, but that DynamicMethod will be eventually destroyed. I am not sure whether all parts of the system will be able to handle this stray piece of code heap memory attached to destroyed DynamicMethod gracefully. It may be better to turn LCGMethodResolver::m_recordCodePointer into a linked list, same way as e.g. LCGMethodResolver::m_DynamicStringLiterals are done, and do cleanup of all code blocks as part of the regular DynamicMethod cleanup. As side effect, it would prepare us for enabling tiered compilation for DynamicMethods. DynamicMethods do not support tiered compilation because of it is not possible to attach multiple pieces of code to a single DynamicMethod.

If we do both of these, the entire RemoveJitData can be deleted.

AaronRobinsonMSFT added this to the 10.0.0 milestone Jun 5, 2025

AaronRobinsonMSFT requested a review from jkotas June 5, 2025 22:41

AaronRobinsonMSFT added the area-VM-coreclr label Jun 5, 2025

dotnet-policy-service bot assigned AaronRobinsonMSFT Jun 5, 2025

Fix GCC build.

a43c6d2

AaronRobinsonMSFT commented Jun 6, 2025

View reviewed changes

src/coreclr/vm/class.cpp Outdated Show resolved Hide resolved

Fix contracts

9d74732

AaronRobinsonMSFT marked this pull request as ready for review June 6, 2025 17:43

Fix contracts

d7fbfcc

AaronRobinsonMSFT requested a review from Copilot June 6, 2025 22:39

Copilot AI reviewed Jun 6, 2025

View reviewed changes

Narrow contract

2c934d7

jkotas reviewed Jun 8, 2025

View reviewed changes

src/coreclr/vm/profilingenumerators.cpp Show resolved Hide resolved

src/coreclr/vm/crst.h Show resolved Hide resolved

jkotas added the tenet-performance Performance related issue label Jun 8, 2025

Fix thread mode on DynamicMethod clean-up

b839ad9

Narrow contract

jkotas reviewed Jun 9, 2025

View reviewed changes

src/coreclr/vm/runtimehandles.cpp Outdated Show resolved Hide resolved

Feedback

ced737a

jkotas reviewed Jun 9, 2025

View reviewed changes

src/coreclr/vm/codeman.cpp Outdated Show resolved Hide resolved

Fix clean-up of delay structures.

5e94fc3

This was referenced Jun 10, 2025

Build on Windows Fails sometimes with fatal error C1090: PDB API call failed #48070

Open

Interop/COM/NativeClients/Dispatch test failure: 0x80131516 = dispatchCoerceTesting->Invoke #116393

Open

Move DynamicMethodDesc destruction to finalizer queue if not

3f4e1f7

possible due to MethodDesc iteration. Rework how DynamicMethodDesc and LCGMethodResolver destruction works. This was needed due to Crst lock inversion. Remove CrstIbcProfile

build-analysis bot mentioned this pull request Jun 12, 2025

Intermittent build failure in AfterSourceBuild: "Could not write state file" #76488

Open

AaronRobinsonMSFT requested a review from jkotas June 12, 2025 21:02

Fix contract

9c11801

jkotas reviewed Jun 12, 2025

View reviewed changes

src/coreclr/vm/class.h Outdated Show resolved Hide resolved

jkotas reviewed Jun 13, 2025

View reviewed changes

src/coreclr/vm/class.h Outdated Show resolved Hide resolved

jkotas reviewed Jun 13, 2025

View reviewed changes

src/coreclr/vm/codeman.cpp Outdated Show resolved Hide resolved

jkotas reviewed Jun 13, 2025

View reviewed changes

src/coreclr/vm/threads.cpp Outdated Show resolved Hide resolved

build-analysis bot mentioned this pull request Jun 13, 2025

LibraryImportGenerator.Unit.Tests crashing on linux-x64 mono interpreter #100800

Open

AaronRobinsonMSFT added 6 commits June 13, 2025 10:46

Remove GetChunk optimization.

6e662e6

Move finalizer related work functions to finalizerthread.cpp.

5d726df

Alter HostCodeHeap memory clean up.

aa27e44

Merge remote-tracking branch 'upstream/main' into runtime_102858

c1509c6

Nits

4cb0434

Unconditionally publish UnwindInfo.

ae3881c

AaronRobinsonMSFT requested a review from jkotas June 14, 2025 00:39

AaronRobinsonMSFT added 2 commits June 13, 2025 17:53

Missed function rename.

b28c538

Contract violation fix

7c1e074

Fixup Crst relationship of WrapperTemplate and ExecutableAllocatorLock.

build-analysis bot mentioned this pull request Jun 14, 2025

browser-wasm windows Debug AllSubsets_CoreCLR builds failing in emcc seemingly unrelated to any code issues #116647

Open

Merge remote-tracking branch 'upstream/main' into runtime_102858

5ab23b9

Merge remote-tracking branch 'upstream/main' into runtime_102858

976cfdf

build-analysis bot mentioned this pull request Jun 17, 2025

Occasional failure in "browser-wasm windows Release LibraryTests: Build Product" #116671

Open

jkotas reviewed Jun 17, 2025

View reviewed changes

src/coreclr/vm/ceemain.cpp Outdated Show resolved Hide resolved

jkotas reviewed Jun 17, 2025

View reviewed changes

src/coreclr/vm/codeman.cpp Outdated Show resolved Hide resolved

jkotas reviewed Jun 17, 2025

View reviewed changes

src/coreclr/vm/codeman.cpp Outdated Show resolved Hide resolved

Feedback

1de0975

AaronRobinsonMSFT requested a review from jkotas June 17, 2025 21:23

build-analysis bot mentioned this pull request Jun 18, 2025

browser-wasm Windows build error #116746

Open

jkotas mentioned this pull request Jun 22, 2025

Override type load limit in AssemblyBinder::GetNameForDiagnosticsFromManagedALC #116900

Merged

Merge branch 'main' into runtime_102858

ee38616

This was referenced Jun 22, 2025

slow macOS - "##[error]The job running on agent Azure Pipelines 9 ran longer than the maximum time of 60 minutes." dotnet/dnceng#1883

Open

The Operation will be canceled. The next steps may not contain expected logs. dotnet/dnceng#3008

Open

jkotas reviewed Jun 28, 2025

View reviewed changes

	\|\| (finalizerThread->m_DetachCount > 0)
	\|\| (Thread::m_DetachCount > 0)

	if (finalizerThread->m_DetachCount > 0
	if (Thread::m_DetachCount > 0

	// On Windows non-x64 platforms, the OS defined UnwindInfo (accessed from RUNTIME_FUNCTION
	// On Windows x64, publish OS defined UnwindInfo (accessed from RUNTIME_FUNCTION

		// On AMD64 JumpStub is used to call functions that is 2GB away. JumpStubs have a CodeHeader
		// with NULL MethodDesc, are stored in code heap and are reported by EEJitManager::EnumCode.

	// On AMD64 JumpStub is used to call functions that is 2GB away. JumpStubs have a CodeHeader
	// with NULL MethodDesc, are stored in code heap and are reported by EEJitManager::EnumCode.
	// Stubs (see StubCodeBlockKind) have no MethodDesc. Skip them.

	// These are used by the stublinker and the high-level method functions above
	// These are used by the high-level method functions above

Make ETW rundown operate more effectively with JIT #116354

Are you sure you want to change the base?

Make ETW rundown operate more effectively with JIT #116354

Uh oh!

Conversation

AaronRobinsonMSFT commented Jun 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

jkotas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

AaronRobinsonMSFT commented Jun 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AaronRobinsonMSFT commented Jun 14, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

AaronRobinsonMSFT commented Jun 5, 2025 •

edited

Loading

AaronRobinsonMSFT commented Jun 8, 2025 •

edited

Loading