Split up loader heap implementations by davidwrighton · Pull Request #114246 · dotnet/runtime

davidwrighton · 2025-04-03T23:31:57Z

We have long had 3 rather different heaps embedded in one structure called a LoaderHeap. They have shared ... some infrastructure, but there is a mess of confusing flags and code paths which only work for one or the other type of heap.

This PR changes that to supporting the 3 different types of heaps with separate codebases, and keeps as much of the shared infrastructure as I could manage. Its, not what I would call a pretty separation, as there is a bit of a mess around the way the DAC apis can access the heaps, but I think this is an improvement. Notably, in a followon PR I'm intending to provide a new mechanism for the interleaved heap to work off of contents of a file instead of creating unique pages per stub block. (This should slighly improve the cache locality of .NET programs, and is a prerequisite for running on some heavily locked down platforms.) This PR prepares for that by making the Interleaved heap its own thing, so adding new behavior variants is a reviewable piece of work.

The only functional change that happens with this PR is the newly added capability of the interleaved heaps to support freeing memory. (It was always possible to free, but for some reason the re-allocation behavior was not actually supported.)

…_heap_implementations

…, the free block handling is NOT a part of the interleaved heap as it didn't work, and wasn't actually hooked up.

…ared anymore Add support for a free list that actually works in the stub heaps

dotnet-policy-service · 2025-04-03T23:32:43Z

Tagging subscribers to this area: @mangod9
See info in area-owners.md if you want to be subscribed.

…_heap_implementations

Copilot

Copilot reviewed 17 out of 18 changed files in this pull request and generated 1 comment.

Files not reviewed (1)

src/coreclr/utilcode/CMakeLists.txt: Language not supported

src/coreclr/vm/loaderallocator.hpp

src/coreclr/utilcode/loaderheap_shared.cpp

jkotas · 2025-04-09T02:25:09Z

src/coreclr/utilcode/loaderheap_shared.cpp

+                        "\nset the following registry DWORD value:"
+                        "\n"
+                        "\n    HKLM\\Software\\Microsoft\\.NETFramework\\LoaderHeapCallTracing = 1"


Suggested change

"\nset the following registry DWORD value:"

"\n"

"\n HKLM\\Software\\Microsoft\\.NETFramework\\LoaderHeapCallTracing = 1"

"\nset the following environment variable:"

"\n"

"\n DOTNET_LoaderHeapCallTracing=1"

davidwrighton · 2025-04-09T22:40:19Z

/ba-g the failing test is unrelated to this change and was fixed by Michal earlier today.

am11 · 2025-04-10T06:32:10Z

Copilot spotted a typo #114246 (comment).

Co-authored-by: Copilot <[email protected]>

janvorli

LGTM, thank you!

janvorli · 2025-04-10T12:42:57Z

src/coreclr/utilcode/interleavedloaderheap.cpp

+    // and notify the user to provide more reserved mem.
+    _ASSERTE((dwSizeToCommit <= dwSizeToReserve) && "Loaderheap tried to commit more memory than reserved by user");
+
+    if (!fReleaseMemory)


The fReleaseMemory is always set to TRUE at this point.

I'll fix that. Thanks.

janvorli · 2025-04-10T12:46:00Z

src/coreclr/utilcode/interleavedloaderheap.cpp

+
+        INDEBUG(m_dwDebugWastedBytes += unusedRemainder;)
+
+        // For interleaved heaps, further allocations will start from the newly committed page as they cannot


A nit - maybe remove the "For interleaved heap, " from multiple comments here, that was only usefull originally when the code for interleaved heaps stuff was intertwined with the other code.

I'll need to revisit these comments with the page remapping logic in my next PR. I'll address this then.

jkotas · 2025-04-12T03:20:50Z

src/coreclr/inc/loaderheap.h

-//
-// Caller is responsible for synchronization. ExplicitControlLoaderHeap is
-// not multithread safe.
+// The LoaderHeap is the black-box heap and has a Backout() method but none


This comment does not match InterleavedLoaderHeap

jkotas · 2025-04-12T03:23:54Z

src/coreclr/inc/loaderheap.h


 protected:
-    void *UnlockedAllocMemForCode_NoThrow(size_t dwHeaderSize, size_t dwCodeSize, DWORD dwCodeAlignment, size_t dwReserveForJumpStubs);
+    // This frees memory allocated by UnlockAllocMem. It's given this horrible name to emphasize


Suggested change

// This frees memory allocated by UnlockAllocMem. It's given this horrible name to emphasize

// This frees memory allocated by UnlockedAllocStub. It's given this horrible name to emphasize

jkotas · 2025-04-12T03:25:53Z

src/coreclr/inc/loaderheap.h

-    void SetReservedRegion(BYTE* dwReservedRegionAddress, SIZE_T dwReservedRegionSize, BOOL fReleaseMemory)
+
+public:
+    // This frees memory allocated by AllocMem. It's given this horrible name to emphasize


Suggested change

// This frees memory allocated by AllocMem. It's given this horrible name to emphasize

// This frees memory allocated by AllocStub. It's given this horrible name to emphasize

jkotas · 2025-04-12T03:27:20Z

src/coreclr/utilcode/allocmemtracker.cpp

+#define DONOT_DEFINE_ETW_CALLBACK
+#include "eventtracebase.h"
+
+#ifndef DACCESS_COMPILE


Move the file to (non-DAC) UTILCODE_SOURCES in CMakeLists.txt instead of end-to-end ifdef

jkotas · 2025-04-12T03:28:51Z

src/coreclr/utilcode/interleavedloaderheap.cpp

+    m_codePageGenerator = codePageGenerator;
+}
+
+// ~LoaderHeap is not synchronised (obviously)


Suggested change

// ~LoaderHeap is not synchronised (obviously)

jkotas · 2025-04-12T03:30:45Z

src/coreclr/utilcode/loaderheap.cpp

+
+    size_t dwSize = dwRequestedSize;

+    // Interleaved heap cannot ad any extra to the requested size


Suggested change

// Interleaved heap cannot ad any extra to the requested size

davidwrighton added 13 commits January 26, 2024 13:38

Split explicit control heap from normal loader heaps

6d51df4

Finish splitting up loader heap code

a4d4dad

More splitting up of LoaderHeap and ExplicitControlLoaderHeap

f8962a0

Break LoaderHeap codebase into multiple files

08d4d3b

Merge branch 'main' of github.com:dotnet/runtime into split_up_loader…

2ae1527

…_heap_implementations

Follow change from merge

15d776a

Current Progress

332d992

It builds now

e2c216a

Removed unnecessary ExplicitControlLoaderHeapBlock concept

a362f38

Move to having the right set of methods in the various files. Notably…

d00bb82

…, the free block handling is NOT a part of the interleaved heap as it didn't work, and wasn't actually hooked up.

Move field initialization up into the base classes as much as possible

5165ea5

Change stub allocation api from AllocAlignedMem to AllocStub

cdb66ef

Move all of the shared code back into LoaderHeap.cpp where its not sh…

1631405

…ared anymore Add support for a free list that actually works in the stub heaps

ghost added the area-VM-coreclr label Apr 3, 2025

dotnet-policy-service bot assigned davidwrighton Apr 3, 2025

davidwrighton added 2 commits April 3, 2025 16:33

Merge branch 'main' of github.com:dotnet/runtime into split_up_loader…

1647326

…_heap_implementations

Fix build breaks

d53ed49

build-analysis bot mentioned this pull request Apr 5, 2025

NativeAOT failure in System.Reflection.Tests.GetTypeTests.GetType_InvalidAssemblyName #114288

Closed

Update comments and fix at least 1 of the musl build breaks

cf91d74

build-analysis bot mentioned this pull request Apr 7, 2025

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

Open

3 tasks

davidwrighton changed the title ~~[DRAFT] Split up loader heap implementations~~ Split up loader heap implementations Apr 8, 2025

davidwrighton marked this pull request as ready for review April 8, 2025 21:57

Copilot AI review requested due to automatic review settings April 8, 2025 21:57

Copilot AI reviewed Apr 8, 2025

View reviewed changes

src/coreclr/vm/loaderallocator.hpp Outdated Show resolved Hide resolved

jkotas reviewed Apr 9, 2025

View reviewed changes

src/coreclr/utilcode/loaderheap_shared.cpp Show resolved Hide resolved

jkotas reviewed Apr 9, 2025

View reviewed changes

Address feedback

14df448

davidwrighton requested a review from janvorli April 9, 2025 22:23

davidwrighton mentioned this pull request Apr 9, 2025

Create a single copy of stub templates #114462

Merged

davidwrighton requested a review from jkotas April 10, 2025 00:02

Update src/coreclr/vm/loaderallocator.hpp

fbba5a0

Co-authored-by: Copilot <[email protected]>

janvorli approved these changes Apr 11, 2025

View reviewed changes

Remove nothion of non-released blocks from the InterleavedLoaderHeap

a0a4cdc

jkotas reviewed Apr 12, 2025

View reviewed changes

davidwrighton merged commit e6f38d7 into dotnet:main Apr 12, 2025
98 checks passed

github-actions bot locked and limited conversation to collaborators May 13, 2025


		INDEBUG(m_dwDebugWastedBytes += unusedRemainder;)

		// For interleaved heaps, further allocations will start from the newly committed page as they cannot

	// This frees memory allocated by UnlockAllocMem. It's given this horrible name to emphasize
	// This frees memory allocated by UnlockedAllocStub. It's given this horrible name to emphasize

	// This frees memory allocated by AllocMem. It's given this horrible name to emphasize
	// This frees memory allocated by AllocStub. It's given this horrible name to emphasize


		size_t dwSize = dwRequestedSize;

		// Interleaved heap cannot ad any extra to the requested size

Conversation

davidwrighton commented Apr 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dotnet-policy-service bot commented Apr 3, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidwrighton commented Apr 9, 2025

Uh oh!

am11 commented Apr 10, 2025

Uh oh!

janvorli left a comment

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

davidwrighton commented Apr 3, 2025 •

edited

Loading