8263377: Store method handle linkers in the 'non-nmethods' heap #8760

yftsai · 2022-05-17T23:19:54Z

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8263377: Store method handle linkers in the 'non-nmethods' heap

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/8760/head:pull/8760
$ git checkout pull/8760

Update a local copy of the PR:
$ git checkout pull/8760
$ git pull https://git.openjdk.org/jdk pull/8760/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 8760

View PR using the GUI difftool:
$ git pr show -t 8760

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/8760.diff

bridgekeeper · 2022-05-17T23:20:12Z

👋 Welcome back yftsai! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2022-05-17T23:21:54Z

@yftsai The following label will be automatically applied to this pull request:

hotspot

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

src/hotspot/share/code/codeBlob.hpp

openjdk · 2022-05-26T16:16:25Z

@yftsai this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout intrinsics
git fetch https://git.openjdk.java.net/jdk master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

mlbridge · 2022-06-01T16:32:13Z

Webrevs

JornVernee

Some initial comments. The codeBlob code looks mostly good to me. But, I don't have enough knowledge about the GC and IC changes to be able to say the same about that.

src/hotspot/cpu/ppc/sharedRuntime_ppc.cpp

src/hotspot/cpu/arm/sharedRuntime_arm.cpp

src/hotspot/cpu/aarch64/sharedRuntime_aarch64.cpp

src/hotspot/cpu/riscv/sharedRuntime_riscv.cpp

src/hotspot/cpu/s390/sharedRuntime_s390.cpp

JornVernee · 2022-06-03T18:13:22Z

src/hotspot/share/code/codeBlob.cpp

+  if (mhi != NULL) {
+    debug_only(mhi->verify();) // might block
+  }


This is debug only. Looking at CodeCache::allocate, it can only return NULL if the allocation size is <= 0, in which case an earlier assert will already fire. So, this null check doesn't seem needed?

Suggested change

if (mhi != NULL) {

debug_only(mhi->verify();) // might block

}

debug_only(mhi->verify();) // might block

This seems needed. CodeCache::allocate may return NULL if the code cache is full.

src/hotspot/share/code/codeBlob.cpp

JornVernee · 2022-06-03T18:18:52Z

src/hotspot/share/classfile/systemDictionary.cpp

  assert(Arguments::is_interpreter_only() || (spe->method()->has_compiled_code() &&
-         spe->method()->code()->entry_point() == spe->method()->from_compiled_entry()),
+         spe->method()->code()->code_begin() == spe->method()->from_compiled_entry()),


Not sure if this assert is still needed/useful, since we should only encounter MH linker blobs here, for which the from_compiled_entry is always code_begin.

I'd suggest just removing this assert.

JornVernee · 2022-06-03T18:38:16Z

src/hotspot/share/compiler/compileBroker.hpp

-  static nmethod* compile_method(const methodHandle& method,
+  static CodeBlob* compile_method(const methodHandle& method,
                                 int osr_bci,
                                 int comp_level,
                                 const methodHandle& hot_method,
                                 int hot_count,
                                 CompileTask::CompileReason compile_reason,
                                 TRAPS);

-  static nmethod* compile_method(const methodHandle& method,
+  static CodeBlob* compile_method(const methodHandle& method,


Not so sure about these changes. It seems to me that if a method is requested to be compiled, it should always result in an nmethod.

Alternatively, would it be possible to keep these functions returning an nmethod but add an assert at the start to check that the passed method is not a method handle intrinsic?

The original implementation assumed that MH intrinsics are possible input. If that's not the cases, the newly added assertion simplifies some handling branches.

src/hotspot/share/oops/method.cpp

dean-long · 2022-06-04T01:23:58Z

I don't think we need a new RuntimeBlob subclasses. Can't we use the existing AdapterBlob or MethodHandlesAdapterBlob?

When changing from nmethod to CodeBlob, don't we need to replace COMPILED_METHOD_LOAD JVMTI events with DYNAMIC_CODE_GENERATED events?

dean-long · 2022-06-04T01:43:08Z

Looking at all the CompiledMethod --> CodeBlob changes, I wondering if it wouldn't be better to use a subclass of CompiledMethod, liked we did for AOT methods. However, there may be assumptions in the code that the only subclass of CompiledMethod is nmethod.

dean-long · 2022-06-04T02:01:44Z

/label hotspot-compiler

openjdk · 2022-06-04T02:02:29Z

@dean-long
The hotspot-compiler label was successfully added.

dean-long · 2022-06-04T02:02:33Z

src/hotspot/share/ci/ciMethod.cpp

-      code->log_identity(log);
+    CodeBlob* code = get_Method()->code();
+    if (code != NULL && code->is_compiled()) {
+      code->as_compiled_method()->log_identity(log);


Doesn't this change the log output?

Yes, the output of MH intrinsic cases will become like the cases where codes are not set. The removed parts are compiled ID, compiler, and compile level. They are all constants for MH intrinsics, not providing much information.

remove unused argument of NativeJump::check_verified_entry_alignment remove unused argument of NativeJumip::patch_verified_entry remove dead codes in SharedRuntime::generate_method_handle_intrinsic_wrapper

yftsai · 2022-06-07T21:18:36Z

if it wouldn't be better to use a subclass of CompiledMethod

The idea has been explored in this earlier commit. Overall, it is more complex in reimplementing many CompiledMethod interfaces to support nmethod life cycle and GC scans. These interfaces are logically irrelevant to MH intrinsics which never unload and contain no oops.

yftsai · 2022-06-07T23:21:37Z

Can't we use the existing AdapterBlob or MethodHandlesAdapterBlob?

An MH intrinsic is handled differently from them in SharedRuntime::continuation_for_implicit_exception and compiledIC.
The extra field _method is used in places like trace_exception unless this information is not important.

bridgekeeper · 2022-08-07T21:58:01Z

@yftsai This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

…s_merge

bridgekeeper · 2022-09-10T00:31:51Z

@yftsai This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.

Yi-Fan Tsai added 11 commits April 14, 2022 18:31

Move method handle intrinsics from nmethod

273d015

cleanup implementation

5814532

Merge branch 'master' of https://github.com/yftsai/jdk into intrinsics

69a134a

Merge branch 'master' of https://github.com/yftsai/jdk into intrinsics

51db3bd

Make mhmethod available on more platforms

a5a092a

Fix x86 build

127609e

Move MH intrinsics from CompiledMethod

c06a9cf

minimize changes

f2b065b

Change _code to CodeBlob

fb52510

Move MHI to BufferBlob

4248c03

Merge branch 'master' of https://github.com/yftsai/jdk into intrinsics

0b99d9e

openjdk bot added the hotspot hotspot-dev@openjdk.org label May 17, 2022

JornVernee reviewed May 18, 2022

View reviewed changes

src/hotspot/share/code/codeBlob.hpp Show resolved Hide resolved

Move to RuntimeBlob

cde34ad

openjdk bot added the merge-conflict Pull request has merge conflict with target branch label May 26, 2022

Merge branch 'master' of https://github.com/yftsai/jdk into intrinsics

e69ac16

openjdk bot removed the merge-conflict Pull request has merge conflict with target branch label May 26, 2022

Ubuntu and others added 2 commits May 31, 2022 22:18

Merge branch 'master' of https://github.com/yftsai/jdk into intrinsics

321cee4

Add PrintCodeCache support

63771d6

yftsai marked this pull request as ready for review June 1, 2022 16:25

openjdk bot added the rfr Pull request is ready for review label Jun 1, 2022

JornVernee reviewed Jun 3, 2022

View reviewed changes

openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Jun 4, 2022

dean-long reviewed Jun 4, 2022

View reviewed changes

Remove dead codes

00c9943

remove unused argument of NativeJump::check_verified_entry_alignment remove unused argument of NativeJumip::patch_verified_entry remove dead codes in SharedRuntime::generate_method_handle_intrinsic_wrapper

Yi-Fan Tsai added 4 commits June 21, 2022 05:38

Post dynamic_code_generate event when MH intrinsic generated

e3e9f97

Merge branch 'master' of https://github.com/yftsai/jdk into intrinsics

d92b864

Merge branch 'master' of https://github.com/yftsai/jdk into intrinsics

8c8f7c9

Fix merge difference

f65f7c0

Merge branch 'master' of https://github.com/yftsai/jdk into intrinsic…

f20fe0f

…s_merge

bridgekeeper bot closed this Sep 10, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8263377: Store method handle linkers in the 'non-nmethods' heap #8760

8263377: Store method handle linkers in the 'non-nmethods' heap #8760

yftsai commented May 17, 2022 •

edited by openjdk bot

bridgekeeper bot commented May 17, 2022

openjdk bot commented May 17, 2022

openjdk bot commented May 26, 2022

mlbridge bot commented Jun 1, 2022 •

edited

JornVernee left a comment •

edited

JornVernee Jun 3, 2022

yftsai Jun 7, 2022

JornVernee Jun 3, 2022

JornVernee Jun 3, 2022

yftsai Jun 7, 2022 •

edited

dean-long commented Jun 4, 2022

dean-long commented Jun 4, 2022

dean-long commented Jun 4, 2022

openjdk bot commented Jun 4, 2022

dean-long Jun 4, 2022

yftsai Jun 20, 2022

yftsai commented Jun 7, 2022 •

edited

yftsai commented Jun 7, 2022

bridgekeeper bot commented Aug 7, 2022

bridgekeeper bot commented Sep 10, 2022

8263377: Store method handle linkers in the 'non-nmethods' heap #8760

8263377: Store method handle linkers in the 'non-nmethods' heap #8760

Conversation

yftsai commented May 17, 2022 • edited by openjdk bot

Progress

Issue

Reviewing

bridgekeeper bot commented May 17, 2022

openjdk bot commented May 17, 2022

openjdk bot commented May 26, 2022

mlbridge bot commented Jun 1, 2022 • edited

Webrevs

JornVernee left a comment • edited

Choose a reason for hiding this comment

JornVernee Jun 3, 2022

Choose a reason for hiding this comment

yftsai Jun 7, 2022

Choose a reason for hiding this comment

JornVernee Jun 3, 2022

Choose a reason for hiding this comment

JornVernee Jun 3, 2022

Choose a reason for hiding this comment

yftsai Jun 7, 2022 • edited

Choose a reason for hiding this comment

dean-long commented Jun 4, 2022

dean-long commented Jun 4, 2022

dean-long commented Jun 4, 2022

openjdk bot commented Jun 4, 2022

dean-long Jun 4, 2022

Choose a reason for hiding this comment

yftsai Jun 20, 2022

Choose a reason for hiding this comment

yftsai commented Jun 7, 2022 • edited

yftsai commented Jun 7, 2022

bridgekeeper bot commented Aug 7, 2022

bridgekeeper bot commented Sep 10, 2022

yftsai commented May 17, 2022 •

edited by openjdk bot

mlbridge bot commented Jun 1, 2022 •

edited

JornVernee left a comment •

edited

yftsai Jun 7, 2022 •

edited

yftsai commented Jun 7, 2022 •

edited