On Thu, 20 Feb 2025 13:14:34 GMT, Thomas Stuefe <[email protected]> wrote:
>> Greetings,
>>
>> This is a rewrite of the Compiler Memory Statistic. The primary new feature
>> is the capability to track allocations by C2 phases. This will allow for a
>> much faster, more thorough analysis of footprint issues.
>>
>> Tracking Arena memory movement is not trivial since one needs to follow the
>> ebb and flow of allocations over nested C2 phases. A phase typically
>> allocates more than it releases, accruing new nodes and resource area. A
>> phase can also release more than allocated when Arenas carried over from
>> other phases go out of scope in this phase. Finally, it can have high
>> temporary peaks that vanish before the phase ends.
>>
>> I wanted to track that information correctly and display it clearly in a way
>> that is easy to understand.
>>
>> The patch implements per-phase tracking by instrumenting the `TracePhase`
>> stack object (thanks to @rwestrel for this idea).
>>
>> The nice thing with this technique is that it also allows for quick analysis
>> of a suspected hot spot (eg, the inside of a loop): drop a TracePhase in
>> there with a speaking name, and you can see the allocations inside that
>> phase.
>>
>> The statistic gives us two new forms of output:
>>
>> 1) At the moment the compilation memory *peaked*, we now get a detailed
>> breakdown of that peak usage per phase:
>>
>>
>> Arena Usage by Arena Type and compilation phase, at arena usage peak of
>> 58817816:
>> Phase Total ra node comp
>> type index reglive regsplit cienv other
>> none 1205512 155104 982984 33712
>> 0 0 0 0 0 33712
>> parse 11685376 720016 6578728 1899064
>> 0 0 0 0 1832888 654680
>> optimizer 916584 0 556416 0
>> 0 0 0 0 0 360168
>> escapeAnalysis 1983400 0 1276392 707008
>> 0 0 0 0 0 0
>> connectionGraph 720016 0 0 621832
>> 0 0 0 0 98184 0
>> macroEliminate 196448 0 196448 0
>> 0 0 0 0 0 0
>> iterGVN 327440 0 196368 131072
>> 0 0 0 0 0 0
>> incrementalInline 3992816 0 3043704 62...
>
> Thomas Stuefe has refreshed the contents of this pull request, and previous
> commits have been removed. The incremental views will show differences
> compared to the previous content of the PR. The pull request contains one new
> commit since the last revision:
>
> avoid Thread::current in high traffic chunk alloc path
A few naming/documentation comments so far.
src/hotspot/share/compiler/compilationMemoryStatistic.cpp line 1093:
> 1091: Compile::TracePhase tp(Phase::_t_testTimer1);
> 1092: Arena ar(MemTag::mtCompiler, Arena::Tag::tag_reglive);
> 1093: ar.Amalloc(2 * M); // phase-local peak, should show up at
> MY-TESTPHASE-2
The reference to `MY-TESTPHASE-2` seems obsolete.
test/hotspot/jtreg/compiler/print/CompileCommandMemLimit.java line 105:
> 103: // by phase end. So, in the phase timeline these 2MB must show
> up as "significant temporary peak".
> 104: // In testPhase2, we allocate 32MB from resource area, which is
> leaked until the end of the compilation. This
> 105: // means that these 32MB will show up as permanent memory
> increase in the phasetimeline.
The references to `testPhase` seem obsolete, do you mean `Phase::_t_testTimer1`
and `Phase::_t_testTimer2`?
test/hotspot/jtreg/compiler/print/CompileCommandMemLimit.java line 105:
> 103: // by phase end. So, in the phase timeline these 2MB must show
> up as "significant temporary peak".
> 104: // In testPhase2, we allocate 32MB from resource area, which is
> leaked until the end of the compilation. This
> 105: // means that these 32MB will show up as permanent memory
> increase in the phasetimeline.
Suggestion:
// means that these 32MB will show up as permanent memory increase in
the phase timeline.
test/hotspot/jtreg/compiler/print/CompileCommandPrintMemStat.java line 27:
> 25: /*
> 26: * @test id=c2
> 27: * @summary Checks that -XX:CompileCommand=PrintMemStat,... works
Suggestion:
* @summary Checks that -XX:CompileCommand=MemStat,... works
test/hotspot/jtreg/compiler/print/CompileCommandPrintMemStat.java line 35:
> 33: /*
> 34: * @test id=c1
> 35: * @summary Checks that -XX:CompileCommand=PrintMemStat,... works
Suggestion:
* @summary Checks that -XX:CompileCommand=MemStat,... works
test/hotspot/jtreg/compiler/print/CompileCommandPrintMemStat.java line 86:
> 84: oa.reportDiagnosticSummary();
> 85:
> 86: // We expect two printouts for "PrintMemStat". A line at
> compilation time, and a line in a summary report
Suggestion:
// We expect two printouts for "MemStat". A line at compilation time,
and a line in a summary report
-------------
Changes requested by rcastanedalo (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/23530#pullrequestreview-2636581034
PR Review Comment: https://git.openjdk.org/jdk/pull/23530#discussion_r1967354177
PR Review Comment: https://git.openjdk.org/jdk/pull/23530#discussion_r1967350305
PR Review Comment: https://git.openjdk.org/jdk/pull/23530#discussion_r1967355138
PR Review Comment: https://git.openjdk.org/jdk/pull/23530#discussion_r1967358896
PR Review Comment: https://git.openjdk.org/jdk/pull/23530#discussion_r1967357706
PR Review Comment: https://git.openjdk.org/jdk/pull/23530#discussion_r1967359828
