RFR: 8322630: Remove ICStubs and related safepoints [v6]

Thu Feb 8 12:52:07 UTC 2024

On Tue, 30 Jan 2024 09:08:01 GMT, Erik Österlund <eosterlund at openjdk.org> wrote:

>> ICStubs solve an atomicity problem when setting both the destination and data of an inline cache. Unfortunately, it also leads to occasional safepoint carpets when multiple threads need to ICRefill the stubs at the same time, and spurious GuaranteedSafepointInterval "Cleanup" safepoints every second. This patch changes inline caches to not change the data part at all during the nmethod life cycle, hence removing the need for ICStubs.
>> 
>> The new scheme is less stateful. Instead of adding and removing callsite metadata back and forth when transitioning inline cache states, it installs all state any shape of call will ever need at resolution time in a struct that I call CompiledICData. This reduces inline cache state changes to simply changing the destination of the call, and it doesn't really matter what state transitions to what other state.
>> 
>> With this patch, we get rid of ICStub and ICBuffer classes and the related ICRefill and almost all Cleanup safepoints in practice. It also makes the inline cache code much simpler.
>> 
>> I have tested the changes from tier1-7, and run through full aurora performance tests.
>
> Erik Österlund has updated the pull request incrementally with one additional commit since the last revision:
> 
>   ARM32 fixes

First, I'm a bit hesitant to approve this because while it looks good to me, I'm no expert in that code and in particular how it interacts with other things all around.
Also I only looked at x86 assembler code (and a bit aarch64), so this is not a complete review anyway.
The only comments I can contribute is some additional refactoring, so I'm keeping this review as Comment.

src/hotspot/share/code/compiledIC.hpp line 57:

> 55: };
> 56: 
> 57: // A CompiledICData* is a helper object for the inline cache implementation.

Suggestion:

// A CompiledICData is a helper object for the inline cache implementation.

src/hotspot/share/code/compiledMethod.cpp line 457:

> 455:   if (clean_all || !cm->is_in_use() || cm->is_unloading() || cm->method()->code() != cm) {
> 456:     cdc->set_to_clean();
> 457:   }

Maybe a single `clean_if_nmethod_is_unloaded()` with the destination address as a parameter would avoid the code duplication with the other variant; otherwise another static helper function would be great.

src/hotspot/share/code/nmethod.cpp line 2310:

> 2308:       } else {
> 2309:         CompiledDirectCall::at(call_site);
> 2310:       }

Since `CompiledICLocker` does not lock anyway if it is safe (using the `is_safe` method), isn't the `if` superfluous here (and just keeping the `else` part does the same)?

src/hotspot/share/oops/oopsHierarchy.hpp line 182:

> 180: class   Method;
> 181: class   ConstantPool;
> 182: //      class CHeapObj

Not sure why this is here, but maybe also remove this line.

src/hotspot/share/runtime/sharedRuntime.cpp line 1369:

> 1367:   } else {
> 1368:     // Callsite is a direct call - set it to the destination method
> 1369:     CompiledICLocker ml(caller_nm);

Not sure if it makes sense to factor out the `CompiledICLocker ml(caller_nm); call in both if branches.

src/hotspot/share/runtime/sharedRuntime.cpp line 1665:

> 1663: 
> 1664:     // Check relocations for the matching call to 1) avoid false positives,
> 1665:     // and 2) determine the type.

To me the comment is confusing (probably pre-existing): There does not seem to be a result of the code checking the relocations containing the "type" of the following code. I.e. the only reason this seems to be done here is reason 1), reason 2) seems obsolete.

src/hotspot/share/runtime/sharedRuntime.cpp line 1803:

> 1801:   RelocIterator iter(caller, callsite_addr, callsite_addr + 1);
> 1802:   if (!iter.next()) {
> 1803:     // No reloc entry found; not a static or opt virutal call

Suggestion:

    // No reloc entry found; not a static or optimized virtual call

(actually s/virutal/virtual is as fine. Or maybe use `opt_virtual` as in line 1798 above)

src/hotspot/share/runtime/sharedRuntime.cpp line 1807:

> 1805:   }
> 1806: 
> 1807:   assert(iter.has_current(), "must have a reloc at java call site");

This assert seems to be superfluous after the `if (!iter.next())` bailout just above.

src/hotspot/share/runtime/sharedRuntime.cpp line 1837:

> 1835:   CompiledMethod* caller_nm = cb->as_compiled_method();
> 1836: 
> 1837:   for (;;) {

I think the comment in line 1826 about

  // Transitioning IC caches may require transition stubs. If we run out
  // of transition stubs, we have to drop locks and perform a safepoint
  // that refills them.

is out of date and should be removed. At least the code that generates the IC stubs below has been removed...

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

PR Review: https://git.openjdk.org/jdk/pull/17495#pullrequestreview-1869869238
PR Review Comment: https://git.openjdk.org/jdk/pull/17495#discussion_r1482897763
PR Review Comment: https://git.openjdk.org/jdk/pull/17495#discussion_r1482895770
PR Review Comment: https://git.openjdk.org/jdk/pull/17495#discussion_r1482870537
PR Review Comment: https://git.openjdk.org/jdk/pull/17495#discussion_r1482845871
PR Review Comment: https://git.openjdk.org/jdk/pull/17495#discussion_r1482825184
PR Review Comment: https://git.openjdk.org/jdk/pull/17495#discussion_r1482810188
PR Review Comment: https://git.openjdk.org/jdk/pull/17495#discussion_r1482820114
PR Review Comment: https://git.openjdk.org/jdk/pull/17495#discussion_r1482822437
PR Review Comment: https://git.openjdk.org/jdk/pull/17495#discussion_r1482796779