On Tue, 25 Feb 2025 16:43:21 GMT, Thomas Stuefe <stu...@openjdk.org> 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 updated the pull request incrementally with five additional
> commits since the last revision:
>
> - feedback ashu
> - feedback roberto
> - final-statistics-switch
> - performance fix
> - remove test code
Changes requested by rcastanedalo (Reviewer).
src/hotspot/share/compiler/compilationMemStatInternals.hpp line 243:
> 241: int retrieve_live_node_count() const;
> 242:
> 243: DEBUG_ONLY(void verify() const;)
Unused. I suggest to either call this function from some appropriate point (in
debug mode only) or just remove it.
src/hotspot/share/runtime/globals.hpp line 1402:
> 1400: "Print metaspace statistics upon VM exit.")
> \
> 1401:
> \
> 1402: product(bool, PrintCompilerMemoryStatisticsAtExit, false, DIAGNOSTIC,
> \
Would it be possible to add a test for this new flag, perhaps by extending the
existing test logic in `CompileCommandPrintMemStat`?
src/hotspot/share/utilities/globalDefinitions.hpp line 371:
> 369:
> 370: #define PROPERFMT "%zu%s"
> 371: #define PROPERFMT_W(width) "%" #width "zu%s"
Unused, please remove.
src/hotspot/share/utilities/ostream.cpp line 225:
> 223: while (count > 0) {
> 224: int nw = (count > 8) ? 8 : count;
> 225: this->write(tmp, nw);
Are these changes essential for the rest of the changeset? If not, I would
suggest to leave them to a separate RFE, for simplicity.
-------------
PR Review: https://git.openjdk.org/jdk/pull/23530#pullrequestreview-2647251550
PR Review Comment: https://git.openjdk.org/jdk/pull/23530#discussion_r1973265196
PR Review Comment: https://git.openjdk.org/jdk/pull/23530#discussion_r1973277216
PR Review Comment: https://git.openjdk.org/jdk/pull/23530#discussion_r1973266713
PR Review Comment: https://git.openjdk.org/jdk/pull/23530#discussion_r1973272906
