Change Interpreter to share code with the JIT and use its CompAllocator #123830
Conversation
Create a shared arena allocator infrastructure in src/coreclr/jitshared/ that can be used by both the JIT and interpreter. This addresses the memory leak issue in the interpreter where AllocMemPool() was using raw malloc() without any mechanism to free the memory. New shared infrastructure (jitshared/): - allocconfig.h: IAllocatorConfig interface for host abstraction - arenaallocator.h/.cpp: Page-based arena allocator with bulk deallocation - compallocator.h: CompAllocator and CompIAllocator template wrappers - memstats.h: Shared memory statistics tracking infrastructure Interpreter changes: - Add ArenaAllocator member to InterpCompiler - AllocMemPool() now uses arena allocator instead of malloc() - Add destructor to InterpCompiler that calls arena.destroy() - Add InterpMemKind enum for future memory profiling support - Add InterpAllocatorConfig implementing IAllocatorConfig JIT preparation (for future integration): - Add JitAllocatorConfig implementing IAllocatorConfig using g_jitHost The interpreter now properly frees all compilation-phase memory when the InterpCompiler is destroyed, fixing the long-standing FIXME comment about memory leaks.
Update interpreter compiler to use CompAllocator with InterpMemKind categories instead of raw AllocMemPool() calls, enabling memory profiling when MEASURE_INTERP_MEM_ALLOC is defined. - Add InterpAllocator typedef and getAllocator(InterpMemKind) method - Update MemPoolAllocator to store InterpMemKind for TArray usage - Add operator new overloads for InterpAllocator - Categorize allocations: BasicBlock, Instruction, StackInfo, CallInfo, Reloc, SwitchTable, IntervalMap, ILCode, Generic
- Create jitshared.h with MEASURE_MEM_ALLOC define (1 in DEBUG, 0 in Release) - Update CompAllocator template to accept MemStats* for tracking allocations - Add memory statistics infrastructure to InterpCompiler: - Static aggregate stats (s_aggStats, s_maxStats) with minipal_mutex - initMemStats(), finishMemStats(), dumpAggregateMemStats(), dumpMaxMemStats() - Add DOTNET_JitMemStats config value to interpconfigvalues.h - Update ProcessShutdownWork to dump stats when DOTNET_JitMemStats=1 - Link interpreter against minipal for mutex support Usage: DOTNET_JitMemStats=1 DOTNET_InterpMode=3 corerun app.dll
Change templates to take a single traits struct parameter instead of separate TMemKind and MemKindCount parameters. The traits struct provides: - MemKind: The enum type for memory kinds - Count: A static constexpr int giving the number of enum values - Names: A static const char* const[] array of names for each kind This fixes the issue where the largest method stats didn't print the kind breakdown because m_memKindNames was NULL (the per-method MemStats objects weren't initialized with the names array). With traits, the names are accessed via TMemKindTraits::Names directly, so no initialization is needed and all MemStats objects can print by kind.
- Add histogram.h and histogram.cpp to jitshared directory - Histogram uses std::atomic for thread-safe counting - Interpreter records memory allocation and usage distributions - Display histograms after aggregate and max stats in output
- Add DOTNET_InterpDumpMemStats config option (default 0) - When InterpDump is active and InterpDumpMemStats=1, print per-method memory statistics after BuildEHInfo completes (includes all allocations) - When InterpDump is active but InterpDumpMemStats is not set, print a hint message about how to enable per-method stats
- Create jitshared/dumpable.h with Dumpable base class - Update jitshared/histogram.h to inherit from Dumpable - Update JIT CMakeLists.txt to include jitshared directory and histogram.cpp - Remove JIT's own Histogram and Dumpable classes from compiler.hpp - Remove JIT's Histogram implementation from utils.cpp - Update dump() methods to be const (Counter, NodeCounts, DumpOnShutdown) - Use std::atomic for thread-safe histogram counting instead of InterlockedAdd
- Template ArenaAllocator<TMemKindTraits> to include MemStats as a member - Add MemStatsAllocator helper struct for tracking allocations by kind - Add outOfMemory() method to IAllocatorConfig interface - Update CompAllocator to get stats from ArenaAllocator via getMemStatsAllocator() - Update interpreter to use InterpArenaAllocator typedef - Remove separate m_stats member from InterpCompiler (now in arena) - Add JIT's jitallocconfig.cpp to build, with global g_jitAllocatorConfig - ArenaAllocator implementation moved to header (template requirement) This prepares the infrastructure for the JIT to also use the shared templated ArenaAllocator in a future change.
github-actions
bot
added
the
area-CodeGen-coreclr
davidwrighton
changed the title
|
Tagging subscribers to this area: @JulieLeeMSFT, @dotnet/jit-contrib |
There was a problem hiding this comment.
Pull request overview
This PR introduces a shared allocator/statistics infrastructure used by both the JIT and the interpreter, so the interpreter can allocate compilation-temporary memory via the same arena-style patterns as the JIT and optionally report categorized allocation statistics.
Changes:
- Added a new
jitshared/component containing a templated arena allocator, categorized allocation wrappers, histogram support, and memory stats helpers. - Refactored the JIT to use the shared allocator templates and a new
JitAllocatorConfigabstraction for host-backed slab allocation. - Updated the interpreter to allocate from an arena (instead of leaking malloc-backed pools), categorize allocations by new
InterpMemKind, and optionally emit per-method/aggregate mem stats.
Reviewed changes
Copilot reviewed 26 out of 26 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/coreclr/jitshared/memstats.h | New templated memory allocation statistics types used by JIT/interpreter. |
| src/coreclr/jitshared/jitshared.h | Shared macro/config definitions for allocator/stat collection. |
| src/coreclr/jitshared/histogram.h | New shared histogram API for distribution reporting. |
| src/coreclr/jitshared/histogram.cpp | Shared histogram implementation (now atomic-based). |
| src/coreclr/jitshared/dumpable.h | Shared “dumpable” base interface used by stats/reporting types. |
| src/coreclr/jitshared/compallocator.h | New shared categorized allocator wrapper and IAllocator adapter. |
| src/coreclr/jitshared/arenaallocator.h | New shared templated arena allocator implementation. |
| src/coreclr/jitshared/arenaallocator.cpp | Stub translation unit retained for build compatibility. |
| src/coreclr/jitshared/allocconfig.h | New host abstraction interface for arena page allocation and OOM. |
| src/coreclr/jitshared/CMakeLists.txt | Adds CMake project structure for the new jitshared area. |
| src/coreclr/jit/utils.cpp | Moves histogram/dumpable usage to shared implementation and adjusts constness. |
| src/coreclr/jit/jitallocconfig.h | Adds JIT-specific IAllocatorConfig implementation declaration. |
| src/coreclr/jit/jitallocconfig.cpp | Implements JIT host-backed allocation + debug behaviors for arena pages. |
| src/coreclr/jit/compiler.hpp | Switches to shared Dumpable/Histogram headers and const dump API. |
| src/coreclr/jit/compiler.cpp | Updates JIT mem-stats plumbing and arena construction to use new config. |
| src/coreclr/jit/alloc.h | Refactors arena/comp allocator types into template instantiations + new stats manager. |
| src/coreclr/jit/alloc.cpp | Provides JIT mem-kind names and new aggregate/max stats implementation. |
| src/coreclr/jit/CMakeLists.txt | Wires jitshared sources/includes and adds new JIT allocator config source. |
| src/coreclr/interpreter/interpmemkind.h | Adds interpreter memory kind taxonomy via X-macro list. |
| src/coreclr/interpreter/interpconfigvalues.h | Adds new interpreter config knobs for dumping memory stats. |
| src/coreclr/interpreter/interpallocconfig.h | Adds interpreter IAllocatorConfig implementation for arena pages. |
| src/coreclr/interpreter/eeinterp.cpp | Initializes/stops interpreter mem-stats reporting at startup/shutdown. |
| src/coreclr/interpreter/compiler.h | Integrates shared allocators into interpreter compiler and introduces categorized alloc accessors. |
| src/coreclr/interpreter/compiler.cpp | Migrates interpreter allocations to arena + implements mem-stats aggregation/histograms. |
| src/coreclr/interpreter/CMakeLists.txt | Links interpreter against needed shared sources and minipal for mutex support. |
| src/coreclr/CMakeLists.txt | Adds jitshared subdirectory when interpreter feature is enabled. |
| // Ensure that elems * elemSize does not overflow. | ||
| if (elems > (SIZE_MAX / elemSize)) | ||
| { | ||
| return nullptr; // Should call outOfMemory() |
There was a problem hiding this comment.
CompIAllocatorT::ArrayAlloc returns nullptr on elems*elemSize overflow, but GCInfoEncoder callers typically expect allocation failures to be handled via the JIT/host OOM path (non-returning) rather than nullptr. Please call outOfMemory() (or equivalent) here instead of returning nullptr.
| return nullptr; // Should call outOfMemory() | |
| outOfMemory(); |
| virtual void* Alloc(size_t sz) = 0; | ||
| virtual void* ArrayAlloc(size_t elems, size_t elemSize) = 0; | ||
| virtual void Free(void* p) = 0; | ||
| }; |
There was a problem hiding this comment.
The local fallback definition of IAllocator is guarded by IALLOCATOR_DEFINED, but it doesn't define that macro. If any TU includes this header before inc/iallocator.h, and later includes inc/iallocator.h, you'll get a redefinition error. Prefer including the real inc/iallocator.h here (or define IALLOCATOR_DEFINED after the fallback definition) to make include order safe.
| }; | |
| }; | |
| #define _IALLOCATOR_DEFINED_ |
| void InterpCompiler::finishMemStats() | ||
| { | ||
| m_arenaAllocator.finishMemStats(); | ||
| const auto& stats = m_arenaAllocator.getStats(); | ||
|
|
||
| // Record histogram data (in KB, rounded up) | ||
| s_memAllocHist.record((unsigned)((stats.nraTotalSizeAlloc + 1023) / 1024)); | ||
| s_memUsedHist.record((unsigned)((stats.nraTotalSizeUsed + 1023) / 1024)); | ||
|
|
||
| minipal::MutexHolder lock(s_interpStatsMutex); | ||
| s_aggStats.Add(stats); |
There was a problem hiding this comment.
finishMemStats() unconditionally locks s_interpStatsMutex, but the mutex is only initialized in initMemStats(). If finishMemStats() (or the InterpCompiler destructor) can run before initMemStats(), this will lock an uninitialized mutex. Consider ensuring initMemStats() is called (or doing one-time init) inside finishMemStats() before taking the lock.
| // 2: use interpreter for everything except intrinsics. All intrinsics fallback to JIT. Implies DOTNET_ReadyToRun=0. | ||
| // 3: use interpreter for everything, the full interpreter-only mode, no fallbacks to R2R or JIT whatsoever. Implies DOTNET_ReadyToRun=0, DOTNET_EnableHWIntrinsic=0 | ||
|
|
||
| RELEASE_CONFIG_INTEGER(DisplayMemStats, "JitMemStats", 0) // Display interpreter memory usage statistics |
There was a problem hiding this comment.
Missing trailing semicolon after RELEASE_CONFIG_INTEGER(...) makes this header fail to compile (contrast with InterpMode above which ends with ';'). Add the semicolon.
| RELEASE_CONFIG_INTEGER(DisplayMemStats, "JitMemStats", 0) // Display interpreter memory usage statistics | |
| RELEASE_CONFIG_INTEGER(DisplayMemStats, "JitMemStats", 0); // Display interpreter memory usage statistics |
|
|
||
| #ifdef DEBUG | ||
| #define MEASURE_MEM_ALLOC 1 // Collect memory allocation stats. | ||
| #else | ||
| #define MEASURE_MEM_ALLOC 0 // You can set this to 1 to get memory stats in retail, as well | ||
| #endif |
There was a problem hiding this comment.
This header unconditionally (re)defines MEASURE_MEM_ALLOC, but the JIT already defines the same macro in src/coreclr/jit/jit.h:523-533. Including both can trigger macro-redefined warnings/errors on some toolchains. Consider only defining it when it's not already defined (e.g., #ifndef MEASURE_MEM_ALLOC) or refactor so there is a single authoritative definition.
| #ifdef DEBUG | |
| #define MEASURE_MEM_ALLOC 1 // Collect memory allocation stats. | |
| #else | |
| #define MEASURE_MEM_ALLOC 0 // You can set this to 1 to get memory stats in retail, as well | |
| #endif | |
| #ifndef MEASURE_MEM_ALLOC | |
| #ifdef DEBUG | |
| #define MEASURE_MEM_ALLOC 1 // Collect memory allocation stats. | |
| #else | |
| #define MEASURE_MEM_ALLOC 0 // You can set this to 1 to get memory stats in retail, as well | |
| #endif | |
| #endif |
| unsigned sizeCount = 0; | ||
| do | ||
| { | ||
| sizeCount++; | ||
| } while ((sizeTable[sizeCount] != 0) && (sizeCount < 1000)); | ||
|
|
||
| assert(sizeCount < HISTOGRAM_MAX_SIZE_COUNT - 1); | ||
|
|
||
| m_sizeCount = sizeCount; | ||
|
|
||
| for (unsigned i = 0; i <= m_sizeCount; i++) | ||
| { | ||
| m_counts[i].store(0, std::memory_order_relaxed); | ||
| } |
There was a problem hiding this comment.
Histogram::Histogram can set m_sizeCount larger than HISTOGRAM_MAX_SIZE_COUNT-1 if sizeTable is missing the terminating 0 (or is very long). In non-assert builds this leads to out-of-bounds writes/reads on m_counts in the initialization loop and later record()/dump(). Please bound the scan to HISTOGRAM_MAX_SIZE_COUNT-1 and handle unterminated tables explicitly (e.g., clamp and treat the last bucket as overflow, or fail fast).
| // Ensure that count * sizeof(T) does not overflow. | ||
| if (count > (SIZE_MAX / sizeof(T))) | ||
| { | ||
| // This should call outOfMemory() through the arena's config | ||
| return nullptr; | ||
| } |
There was a problem hiding this comment.
On multiplication overflow, CompAllocatorT::allocate currently returns nullptr. Callers (including placement-new users) generally assume the arena allocator never returns nullptr and instead fails fast via the host/JIT OOM path. Please route this through the allocator config's outOfMemory() (or equivalent) rather than returning nullptr.
1 participant
This provides a bump allocator for allocating temporary memory which is managed efficiently by the JIT->EE interface. As well, this provides a scheme for categorizing and measuring allocation costs.
FIXME:
There is now a set of virtual methods on a config struct, that should probably be part of the template here, but this is decent progress towards it all working.