8253920: Share method trampolines in CDS dynamic archive #868

calvinccheung · 2020-10-26T18:12:16Z

This patch is to allow sharing of the same method trampoline for archived Methods with the same AdapterHandleEntry when using CDS dynamic archive.

Running javac on HelloWorld with CDS dynamic archive, the number of calls to SharedRuntime::generate_trampoline() is reduced to 406 times vs 12601 times without the patch.

In terms of saving on instructions and time (on linux-x64):
instr delta = -2807662 -0.1369%
time delta = -6.860 ms -1.8798%

It passed tiers 1,2,3,4 testing.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed

Issue

JDK-8253920: Share method trampolines in CDS dynamic archive

Reviewers

Claes Redestad (@cl4es - Reviewer) ⚠️ Review applies to af513fb
Yumin Qi (@yminqi - Reviewer) ⚠️ Review applies to af513fb
Ioi Lam (@iklam - Reviewer)

Download

$ git fetch https://git.openjdk.java.net/jdk pull/868/head:pull/868
$ git checkout pull/868

bridgekeeper · 2020-10-26T18:12:50Z

👋 Welcome back ccheung! 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 · 2020-10-26T18:14:25Z

@calvinccheung 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.

calvinccheung · 2020-10-26T18:15:12Z

/label remove hotspot

calvinccheung · 2020-10-26T18:15:28Z

/label add hotspot-runtime

openjdk · 2020-10-26T18:15:47Z

@calvinccheung
The hotspot label was successfully removed.

openjdk · 2020-10-26T18:16:14Z

@calvinccheung
The hotspot-runtime label was successfully added.

mlbridge · 2020-10-26T18:21:22Z

Webrevs

cl4es

LGTM

Thanks for ensuring static data-structures only used when dumping are lazily allocated.

cl4es · 2020-10-26T18:37:51Z

src/hotspot/share/classfile/systemDictionaryShared.cpp

-  ResourceObj::C_HEAP> _loaded_unregistered_classes;
+  ResourceObj::C_HEAP> {};
+
+static LoadedUnregisteredClassesTable* _loaded_unregistered_classes = NULL;


@cl4es Thanks for your quick review.

openjdk · 2020-10-26T18:43:28Z

@calvinccheung This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8253920: Share method trampolines in CDS dynamic archive

Reviewed-by: redestad, minqi, iklam

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 20 new commits pushed to the master branch:

7d41a54: 8255450: runtime/ThreadCountLimit.java causes high system load
504cb00: 8252113: Move jfr man page into jfr module
552192f: 8255305: Add Linux x86_32 tier1 to submit workflow
66a3917: 8255331: Problemlist java/foreign/TestMismatch.java on 32-bit platforms until JDK-8254162
cf56c7e: 8254980: ZGC: ZHeapIterator visits armed nmethods with -XX:-ClassUnloading
18d9905: 8255342: Remove non-specified JVM checks on Classes with Record attributes
7679650: 8231231: The printing result is different from the case instruction
f7c59c6: 8255231: Avoid upcalls when initializing the statSampler
dccfd2b: 8255389: ConcurrentHashTable::NoOp omits return in non-void return method
d735f91: 8255040: Replace __sync_add_and_fetch with __atomic_add_fetch to avoid build errors with clang
... and 10 more: https://git.openjdk.java.net/jdk/compare/83a91bfaed4194064e3a5550b1b25980c9adcb8c...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

yminqi

Looks good. Minor suggestions.

yminqi · 2020-10-26T18:34:48Z

src/hotspot/share/memory/archiveBuilder.cpp

+void ArchiveBuilder::update_method_trampolines() {
+  for (int i = 0; i < klasses()->length(); i++) {
+    Klass* k = klasses()->at(i);
+    if (!k->is_instance_klass()) {


Could you use
if (k->is_instance_klass()) {
...
}
as same style of line 869

@yminqi Thanks for your review. I've made the change.

yminqi · 2020-10-26T18:35:30Z

src/hotspot/share/oops/method.cpp

-    constMethod()->set_adapter_trampoline(cds_adapter->get_adapter_trampoline());
-    _from_compiled_entry = cds_adapter->get_c2i_entry_trampoline();
+  assert(_from_compiled_entry != NULL, "sanity");
+  if (DumpSharedSpaces) {


I think you can remove this if check.

No, I can't remove the check. It doesn't work with the DynamicDumpSharedSpaces case. Note that the conditions of the assert statements are the same as the original code.

Yes, I mess up with the two. Then it is OK, thanks for the explain.

I sent Calvin a patch which will allow the if (DumpSharedSpaces) to be removed. The rest of the changes look good to me.

iklam · 2020-10-26T22:10:11Z

src/hotspot/share/oops/method.cpp

-    constMethod()->set_adapter_trampoline(cds_adapter->get_adapter_trampoline());
-    _from_compiled_entry = cds_adapter->get_c2i_entry_trampoline();
+  assert(_from_compiled_entry != NULL, "sanity");
+  if (DumpSharedSpaces) {


I sent Calvin a patch which will allow the if (DumpSharedSpaces) to be removed. The rest of the changes look good to me.

calvinccheung · 2020-10-27T16:12:26Z

/integrate

openjdk · 2020-10-27T16:19:17Z

@calvinccheung Since your change was applied there have been 20 commits pushed to the master branch:

7d41a54: 8255450: runtime/ThreadCountLimit.java causes high system load
504cb00: 8252113: Move jfr man page into jfr module
552192f: 8255305: Add Linux x86_32 tier1 to submit workflow
66a3917: 8255331: Problemlist java/foreign/TestMismatch.java on 32-bit platforms until JDK-8254162
cf56c7e: 8254980: ZGC: ZHeapIterator visits armed nmethods with -XX:-ClassUnloading
18d9905: 8255342: Remove non-specified JVM checks on Classes with Record attributes
7679650: 8231231: The printing result is different from the case instruction
f7c59c6: 8255231: Avoid upcalls when initializing the statSampler
dccfd2b: 8255389: ConcurrentHashTable::NoOp omits return in non-void return method
d735f91: 8255040: Replace __sync_add_and_fetch with __atomic_add_fetch to avoid build errors with clang
... and 10 more: https://git.openjdk.java.net/jdk/compare/83a91bfaed4194064e3a5550b1b25980c9adcb8c...master

Your commit was automatically rebased without conflicts.

Pushed as commit 84e985d.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

8253920: Share method trampolines in CDS dynamic archive

af513fb

openjdk bot added the hotspot hotspot-dev@openjdk.org label Oct 26, 2020

openjdk bot removed the hotspot hotspot-dev@openjdk.org label Oct 26, 2020

openjdk bot added the hotspot-runtime hotspot-runtime-dev@openjdk.org label Oct 26, 2020

calvinccheung marked this pull request as ready for review October 26, 2020 18:16

openjdk bot added the rfr Pull request is ready for review label Oct 26, 2020

cl4es approved these changes Oct 26, 2020

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Oct 26, 2020

yminqi approved these changes Oct 26, 2020

View reviewed changes

update per review comment

213262e

iklam approved these changes Oct 26, 2020

View reviewed changes

update with patch from Ioi

7c745e6

iklam approved these changes Oct 27, 2020

View reviewed changes

openjdk bot added ready Pull request is ready to be integrated and removed ready Pull request is ready to be integrated labels Oct 27, 2020

openjdk bot closed this Oct 27, 2020

openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Oct 27, 2020

calvinccheung deleted the 8253920 branch October 27, 2020 16:50

8253920: Share method trampolines in CDS dynamic archive #868

8253920: Share method trampolines in CDS dynamic archive #868

calvinccheung commented Oct 26, 2020 •

edited by openjdk bot

bridgekeeper bot commented Oct 26, 2020

openjdk bot commented Oct 26, 2020

calvinccheung commented Oct 26, 2020

calvinccheung commented Oct 26, 2020

openjdk bot commented Oct 26, 2020

openjdk bot commented Oct 26, 2020

mlbridge bot commented Oct 26, 2020 •

edited

cl4es left a comment

cl4es Oct 26, 2020

calvinccheung Oct 26, 2020

openjdk bot commented Oct 26, 2020 •

edited

yminqi left a comment

yminqi Oct 26, 2020

calvinccheung Oct 26, 2020

yminqi Oct 26, 2020

calvinccheung Oct 26, 2020

yminqi Oct 26, 2020

iklam Oct 26, 2020

iklam Oct 26, 2020

calvinccheung commented Oct 27, 2020

openjdk bot commented Oct 27, 2020

8253920: Share method trampolines in CDS dynamic archive #868

8253920: Share method trampolines in CDS dynamic archive #868

Conversation

calvinccheung commented Oct 26, 2020 • edited by openjdk bot

Progress

Issue

Reviewers

Download

bridgekeeper bot commented Oct 26, 2020

openjdk bot commented Oct 26, 2020

calvinccheung commented Oct 26, 2020

calvinccheung commented Oct 26, 2020

openjdk bot commented Oct 26, 2020

openjdk bot commented Oct 26, 2020

mlbridge bot commented Oct 26, 2020 • edited

Webrevs

cl4es left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openjdk bot commented Oct 26, 2020 • edited

yminqi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

calvinccheung commented Oct 27, 2020

openjdk bot commented Oct 27, 2020

calvinccheung commented Oct 26, 2020 •

edited by openjdk bot

mlbridge bot commented Oct 26, 2020 •

edited

openjdk bot commented Oct 26, 2020 •

edited