RFR: 8296440: Remove Method* handling from cleanup_inline_caches_impl [v2]

[Coleen Phillimore]

On Mon, 20 Mar 2023 17:47:49 GMT, Richard Reingruber <rrich at openjdk.org> wrote:

>> This PR replaces cleaning of static stubs in CompiledMethod::cleanup_inline_caches_impl() with a guarantee that it is actually not needed because the holder of the embedded target Method* is alive if the caller nmethod is not unloading.
>> 
>> The holder of the target Method* has to be alive because it is reachable from the caller nmethod's oop pool. This is checked by `check_path_to_callee()` when a statically bound call gets resolved.
>> 
>> C2i entry barriers can be removed for the same reason.
>> 
>> Testing:
>> 
>> Many rounds in our CI testing which includes most JCK and JTREG tests, Renaissance benchmark and SAP specific tests with fastdebug and release builds on the standard platforms plus PPC64.
>> 
>> I've also done tier1 and tier2 tests with -XX:-Inline and tier1 tests with ZGC.
>> 
>> I've started hotspot and jdk tier1 tests with -Xcomp. They were not finished when I stopped them after 24h.
>
> Richard Reingruber has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains ten additional commits since the last revision:
> 
>  - Adding TestStaticallyBoundTargetIsReachable.java
>  - Path to target exists also if the receiver is a constant of the caller
>  - Use nmethod::oops_do() to search for to_holder in from_nm
>  - Merge branch 'master'
>  - Remove MacroAssembler::resolve_weak_handle()
>  - Remove keep_alive_offset() and holder_offset() from CLD
>  - Remove MacroAssembler::load_method_holder_cld()
>  - Remove c2i entry barrier
>  - Check dependency for statically bound call

I don't really know if this is right but some comments anyway.

src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp line 4308:

> 4306: void MacroAssembler::load_method_holder_cld(Register rresult, Register rmethod) {
> 4307:   load_method_holder(rresult, rmethod);
> 4308:   ldr(rresult, Address(rresult, InstanceKlass::class_loader_data_offset()));

Can you remove InstanceKlass::klass_loader_data_offset() also?

src/hotspot/share/classfile/classLoaderData.cpp line 791:

> 789: 
> 790: bool ClassLoaderData::handles_contain(oop obj) {
> 791:   return _handles.contains(obj);

This might need to be protected by a metaspace_lock.

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

> 1352: }
> 1353: 
> 1354: class Search2OopsClosure : public OopClosure {

Should this all be under #ifdef ASSERT?

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

> 1416:         return; // `to` is reachable by iterating parents of `from`
> 1417:       }
> 1418:     }

I'd be happier if this part was a function in ClassLoaderData or refactored from record_dependency (along with the constains function).  Since it's similar code.

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

PR Review: https://git.openjdk.org/jdk/pull/12802#pullrequestreview-1349193798
PR Review Comment: https://git.openjdk.org/jdk/pull/12802#discussion_r1142542383
PR Review Comment: https://git.openjdk.org/jdk/pull/12802#discussion_r1142553804
PR Review Comment: https://git.openjdk.org/jdk/pull/12802#discussion_r1142560411
PR Review Comment: https://git.openjdk.org/jdk/pull/12802#discussion_r1142563560