RFR: 8366990: C2: Compilation hits the memory limit when verifying loop opts in Split-If code [v2]

[Damon Fenacci]

On Fri, 10 Oct 2025 13:25:51 GMT, Benoît Maillard <bmaillard at openjdk.org> wrote:

>> This PR prevents the C2 compiler from hitting memory limits during compilation when using `-XX:+StressLoopPeeling` and `-XX:+VerifyLoopOptimizations` in certain edge cases. The fix addresses an issue where the `ciEnv` arena grows uncontrollably due to the high number of verification passes, a complex IR graph, and repeated field accesses leading to unnecessary memory allocations.
>> 
>> ### Analysis
>> 
>> This issue was initially detected with the fuzzer. The original test from the fuzzer was reduced
>> and added to this PR as a regression test.
>> 
>> The test contains a switch inside a loop, and stressing the loop peeling results in
>> a fairly complex graph.  The split-if optimization is applied agressively, and we
>> run a verification pass at every progress made.
>> 
>> We end up with a relatively high number of verification passes, with each pass being
>> fairly expensive because of the size of the graph.
>> Each verification pass requires building a new `IdealLoopTree`. This is quite slow
>> (which is unfortunately hard to mitigate), and also causes inefficient memory usage
>> on the `ciEnv` arena.
>> 
>> The inefficient usages are caused by the `ciInstanceKlass::get_field_by_offset` method.
>> At every call, we have
>> - One allocation on the `ciEnv` arena to store the returned `ciField`
>> - The constructor of `ciField` results in a call to `ciObjectFactory::get_symbol`, which:
>>   - Allocates a new `ciSymbol` on the `ciEnv` arena at every call (when not found in `vmSymbols`)
>>   - Pushes the new symbol to the `_symbols` array
>> 
>> The `ciEnv` objects returned by `ciInstanceKlass::get_field_by_offset` are only used once, to
>> check if the `BasicType` of a static field is a reference type.
>> 
>> In `ciObjectFactory`, the `_symbols` array ends up containg a large number of duplicates for certain symbols
>> (up to several millions), which hints at the fact that `ciObjectFactory::get_symbol` should not be called
>> repeatedly as it is done here.
>> 
>> The stack trace of how we get to the `ciInstanceKlass::get_field_by_offset` is shown below:
>> 
>> 
>> ciInstanceKlass::get_field_by_offset ciInstanceKlass.cpp:412
>> TypeOopPtr::TypeOopPtr type.cpp:3484
>> TypeInstPtr::TypeInstPtr type.cpp:3953
>> TypeInstPtr::make type.cpp:3990
>> TypeInstPtr::add_offset type.cpp:4509
>> AddPNode::bottom_type addnode.cpp:696
>> MemNode::adr_type memnode.cpp:73
>> PhaseIdealLoop::get_late_ctrl_with_anti_dep loopnode.cpp:6477
>> PhaseIdealLoop::get_late_ctrl loopnode.cpp:6439
>> PhaseIdealLoop::build_lo...
>
> Benoît Maillard has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Add -XX:+UnlockDiagnosticVMOptions

src/hotspot/share/ci/ciInstanceKlass.hpp line 207:

> 205:   ciField* get_field_by_offset(int field_offset, bool is_static);
> 206:   ciField* get_field_by_name(ciSymbol* name, ciSymbol* signature, bool is_static);
> 207:   BasicType get_field_type_by_offset(int field_offset, bool is_static);

Following @chhagedorn's suggestion:
Suggestion:

  BasicType get_field_type_by_offset(const int field_offset, const bool is_static);

src/hotspot/share/opto/type.cpp line 3494:

> 3492:         } else {
> 3493:           // Instance fields which contains a compressed oop references.
> 3494:           BasicType basic_elem_type = ik->get_field_type_by_offset(_offset, false);;

Suggestion:

          BasicType basic_elem_type = ik->get_field_type_by_offset(_offset, false);

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/27731#discussion_r2425357342
PR Review Comment: https://git.openjdk.org/jdk/pull/27731#discussion_r2425372940