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

Fri Jan 26 16:02:42 UTC 2024

On Thu, 25 Jan 2024 14:53:07 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:
> 
>   Whitespace fix
>   
>   Co-authored-by: Thomas Schatzl <59967451+tschatzl at users.noreply.github.com>

Just did an initial read through of the PR. Just added some cleanup suggestion. Also noticed something I though looked wrong in the ARM32 port. 

I also went through and tried to find the handful of places in the codebase where the term `ICHolder` (or its derivatives) were still used.  Put them in a separate branch to not clutter this PR. Would be nice to take this all the way and not have stale comments or naming lurking about.  (Also nuked the `DECC` copy-paste-typo)
Comment cleanups:
f1bb02ea472eb314c93d80b830c59bd03e280116

All platforms use `data` as a register alias for the `CompileICData*` register in the `ic_check`. But c2i and itable stubs still use `holder`.  Maybe go all the way here?
5422ed32def491bd1e145959b7f3c49c88cfc50e

Also for PPC and s390 I think the code is easier to understand if the global inline cache register aliases these platforms have are used. But maybe that is just me.
39c0a7ede5187cba52d6fcf48c0852213c48c899

As for the implementation I could not see anything wrong (except the ARM32 port). But I'll leave it people with more expertise in this area.

src/hotspot/cpu/arm/compiledIC_arm.cpp line 107:

> 105:   address stub = find_stub();
> 106:   guarantee(stub != nullptr, "stub not found");
> 107: 

The other platforms removed the trace logging here. If the ARM porters still want this in at least update to log the correct class name. `s/CompiledDirectStaticCall/CompiledDirectCall/`

src/hotspot/cpu/arm/sharedRuntime_arm.cpp line 631:

> 629: 
> 630:   __ ic_check(1 /* end_alignment */);
> 631:   __ ldr(Rmethod, Address(receiver_klass, CompiledICData::speculated_method_offset()));

Maybe I am missing something here but this looks very wrong. The speculated `Klass*` gets loaded into `R4` (which `receiver_klass` alias) in `ic_check` this load would result in loading a `InstanceKlass*` c++ vtable pointer. 
`Ricklass` (`R8` alias) contains the  `CompiledICData*` .

I would think the correct diff would be

-  const Register receiver_klass = R4;
-
-  __ load_klass(receiver_klass, receiver);
-  __ ldr(holder_klass, Address(Ricklass, CompiledICHolder::holder_klass_offset()));
-  __ ldr(Rmethod, Address(Ricklass, CompiledICHolder::holder_metadata_offset()));
-  __ cmp(receiver_klass, holder_klass);
+  __ ic_check(1 /* end_alignment */);
+  __ ldr(Rmethod, Address(Ricklass, CompiledICData::speculated_method_offset()));

The fact that you say ARM32 tests are passing makes me doubt my understanding of the inline cache.

src/hotspot/share/code/compiledIC.cpp line 195:

> 193:   c_ic->verify();
> 194:   return c_ic;
> 195: }

Purely a style thing but could rewrite the `CompiledIC* CompiledIC_X(...)` functions in terms of each other. Made them all fit very well even on my small laptop screen.
```c++

CompiledIC* CompiledIC_before(CompiledMethod* nm, address return_addr) {
  address call_site = nativeCall_before(return_addr)->instruction_address();
  return CompiledIC_at(nm, call_site);
}

CompiledIC* CompiledIC_at(CompiledMethod* nm, address call_site) {
  RelocIterator iter(nm, call_site, call_site + 1);
  iter.next();
  return CompiledIC_at(&iter);
}

CompiledIC* CompiledIC_at(Relocation* call_reloc) {
  address call_site = call_reloc->addr();
  CompiledMethod* cm = CodeCache::find_blob(call_reloc->addr())->as_compiled_method();
  return CompiledIC_at(cm, call_site);
}

CompiledIC* CompiledIC_at(RelocIterator* reloc_iter) {
  CompiledIC* c_ic = new CompiledIC(reloc_iter);
  c_ic->verify();
  return c_ic;
}

src/hotspot/share/code/compiledIC.cpp line 571:

> 569:     return true;
> 570:   } else if (cb->is_vtable_blob()) {
> 571:     return VtableStubs::is_icholder_entry(entry);

`VtableStubs::is_icholder_entry` is no longer used. Should be removed as well.

src/hotspot/share/code/compiledMethod.hpp line 381:

> 379:   void run_nmethod_entry_barrier();
> 380: 
> 381:   // Verify and count cached icholder relocations.

The comment belonged to the removed method.

src/hotspot/share/oops/compiledICHolder.hpp line 44:

> 42: 
> 43: 
> 44: class CompiledICHolder : public CHeapObj<mtCompiler> {

Still has a forward declaration in `src/hotspot/share/oops/oopsHierarchy.hpp`

src/hotspot/share/runtime/vmStructs.cpp line 215:

> 213:   volatile_nonstatic_field(ArrayKlass,         _lower_dimension,                              ArrayKlass*)                           \
> 214:   nonstatic_field(CompiledICHolder,            _holder_metadata,                              Metadata*)                             \
> 215:   nonstatic_field(CompiledICHolder,            _holder_klass,                                 Klass*)                                \

Making the serviceability agent aware of CompiledICData seems like a RFE. However CompiledICHolder has a mirror in the SA. Should probably nuke `src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/oops/CompiledICHolder.java`

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

PR Review: https://git.openjdk.org/jdk/pull/17495#pullrequestreview-1845953963
PR Review Comment: https://git.openjdk.org/jdk/pull/17495#discussion_r1467778057
PR Review Comment: https://git.openjdk.org/jdk/pull/17495#discussion_r1467786778
PR Review Comment: https://git.openjdk.org/jdk/pull/17495#discussion_r1467796274
PR Review Comment: https://git.openjdk.org/jdk/pull/17495#discussion_r1467803589
PR Review Comment: https://git.openjdk.org/jdk/pull/17495#discussion_r1467797143
PR Review Comment: https://git.openjdk.org/jdk/pull/17495#discussion_r1467804557
PR Review Comment: https://git.openjdk.org/jdk/pull/17495#discussion_r1467806821