8255617: Zero: purge the remaining bytecode interpreter profiling support #944

shipilev · 2020-10-29T19:47:02Z

All the stubs in interpreter/zero/bytecodeInterpreterProfiling.hpp are empty. History shows the whole thing gradually moved to template interpreter. We can probably simplify Zero code by dropping these empty stubs altogether. Arguably, this makes porting to new architectures a bit harder, but it seems that the proper stepping stone after Zero is implementing template interpreter anyway.

On my TR 3970X, this improves:

Linux x86_64 Zero "make images" times from ~18 minutes to ~17.5 minutes

I would like to have the opinion of @GoeLin who added this for PPC64 porting back in 8u. And probably @DamonFool who is usually interested in Zero. And @jerboaa, @gnu-andrew who deal with Zero from time to time.

Progress

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

Testing

	Linux x64	Linux x86	Windows x64	macOS x64
Build	✔️ (5/5 passed)	✔️ (2/2 passed)	✔️ (2/2 passed)	✔️ (2/2 passed)
Test (tier1)	✔️ (9/9 passed)	✔️ (9/9 passed)	✔️ (9/9 passed)	✔️ (9/9 passed)

Issue

JDK-8255617: Zero: purge the remaining bytecode interpreter profiling support

Reviewers

Coleen Phillimore (@coleenp - Reviewer)
Severin Gehwolf (@jerboaa - Reviewer) ⚠️ Review applies to 2bcc4e3
Jie Fu (@DamonFool - Committer)

Download

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

bridgekeeper · 2020-10-29T19:47:48Z

👋 Welcome back shade! 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-29T19:49:43Z

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

hotspot-runtime

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.

…port

mlbridge · 2020-10-30T06:56:49Z

Webrevs

01: Full - Incremental (15df284)
00: Full (2bcc4e3)

coleenp

I left this code from the C++ interpreter because I had a dream that we could someday hook zero up to c1/c2 someday, but if so, there would be a more work than adding these back. I'd be interested to see what these other people with interest in Zero think. But this LGTM.

openjdk · 2020-10-30T11:59:22Z

@shipilev 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:

8255617: Zero: purge the remaining bytecode interpreter profiling support

Reviewed-by: coleenp, sgehwolf, jiefu

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 28 new commits pushed to the master branch:

2668d23: 8255861: Also update jaotc.1 for JDK 16
76fa974: 8255850: Hotspot recompiled on first incremental build
622f72b: 8255853: Update all nroff manpages for JDK 16 release
cdf9cd8: 8255784: appcds/javaldr/ExceptionDuringDumpAtObjectsInitPhase.java test failed resulting in VM crash
eab99f3: 8255852: Shenandoah: Fix incorrect copyright header in new files added by 8254315
450452b: 8254876: (fs) NullPointerException not thrown when first argument to Path.of or Paths.get is null
b46d73b: 8255239: The timezone of the hs_err_pid log file is corrupted in Japanese locale
6606e09: 8255380: (zipfs) ZipFileSystem::readExtra can fail if zipinfo-time is not set to false
88ee973: 8254827: JVMCI: Enable it for Windows+AArch64
f64a15d: 8254315: Shenandoah: Concurrent weak reference processing
... and 18 more: https://git.openjdk.java.net/jdk/compare/aa2862ad86ede366c15922fa3fbbb94b51225328...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.

shipilev · 2020-10-30T13:20:19Z

I left this code from the C++ interpreter because I had a dream that we could someday hook zero up to c1/c2 someday, but if so, there would be a more work than adding these back. I'd be interested to see what these other people with interest in Zero think. But this LGTM.

It is my understanding that Zero had basically stepped back to be the "pure" interpreter without any support for compilers, and thus becoming the most portable thing, for example for architectures that have no arch-specific code implemented whatsoever. When one needs an interpreter that hooks into C1/C2, they have to go to template interpreter and start implementing arch-specific stuff. So, porting is basically: a) get Zero working; b) get template interpreter working; c) get C1 working; d) get C2 working. On each of these steps you have something that works, with every step much harder to get it right, but better for performance.

So in this sense, having compiler hooks (like this interpreter profile support) penalizes Zero execution for nothing. If we continue it further, I'd say we can also purge deopt support from Zero too.

Need more thoughts about this, so:

/reviewers 3

openjdk · 2020-10-30T13:20:49Z

@shipilev
The number of required reviews for this PR is now set to 3 (with at least 1 of role reviewers).

jerboaa · 2020-10-30T17:12:39Z

IIRC the original idea of Zero's compiler support was for shark (via LLVM). That got removed with JDK-8171853. It seems fine to remove profiling support too. So what @shipilev said above makes sense to me. Zero is mostly a porter's tool. Keeping it as simple as possible seems the better approach. +1

shipilev · 2020-11-03T07:33:44Z

@DamonFool, @GoeLin, do you have an opinion here?

DamonFool · 2020-11-03T07:50:11Z

@DamonFool, @GoeLin, do you have an opinion here?

Hi @shipilev ，

Sorry, I missed this PR before.
I'll have a look and do some experiments and will let you know soon.

Thanks.

DamonFool · 2020-11-03T08:48:47Z

I left this code from the C++ interpreter because I had a dream that we could someday hook zero up to c1/c2 someday, but if so, there would be a more work than adding these back. I'd be interested to see what these other people with interest in Zero think. But this LGTM.

It is my understanding that Zero had basically stepped back to be the "pure" interpreter without any support for compilers, and thus becoming the most portable thing, for example for architectures that have no arch-specific code implemented whatsoever. When one needs an interpreter that hooks into C1/C2, they have to go to template interpreter and start implementing arch-specific stuff. So, porting is basically: a) get Zero working; b) get template interpreter working; c) get C1 working; d) get C2 working. On each of these steps you have something that works, with every step much harder to get it right, but better for performance.

So in this sense, having compiler hooks (like this interpreter profile support) penalizes Zero execution for nothing. If we continue it further, I'd say we can also purge deopt support from Zero too.

Need more thoughts about this, so:

/reviewers 3

+1

All tests for Zero VM passed in our CI/CD.
And the build time for zero-release vm reduced 20s ~ 30s on our platforms.
Thanks.

shipilev · 2020-11-03T10:36:55Z

I also figured we don't need mdx field in BytecodeInterpreter now. Removed. Plus a few comments/variables removed. Please take a look again, if you can.

DamonFool

Still looks good.

coleenp

Still looks good.

shipilev · 2020-11-04T06:37:37Z

Thanks folks!

/integrate

openjdk · 2020-11-04T06:42:40Z

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

4b88119: 8253037: G1: Improve check for string dedup
2668d23: 8255861: Also update jaotc.1 for JDK 16
76fa974: 8255850: Hotspot recompiled on first incremental build
622f72b: 8255853: Update all nroff manpages for JDK 16 release
cdf9cd8: 8255784: appcds/javaldr/ExceptionDuringDumpAtObjectsInitPhase.java test failed resulting in VM crash
eab99f3: 8255852: Shenandoah: Fix incorrect copyright header in new files added by 8254315
450452b: 8254876: (fs) NullPointerException not thrown when first argument to Path.of or Paths.get is null
b46d73b: 8255239: The timezone of the hs_err_pid log file is corrupted in Japanese locale
6606e09: 8255380: (zipfs) ZipFileSystem::readExtra can fail if zipinfo-time is not set to false
88ee973: 8254827: JVMCI: Enable it for Windows+AArch64
... and 19 more: https://git.openjdk.java.net/jdk/compare/aa2862ad86ede366c15922fa3fbbb94b51225328...master

Your commit was automatically rebased without conflicts.

Pushed as commit a5d8a9c.

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

…port Reviewed-by: coleenp, sgehwolf, jiefu

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

8255617: Zero: purge the remaining bytecode interpreter profiling sup…

2bcc4e3

…port

shipilev force-pushed the JDK-8255617-zero-purge-bi-profiliing branch from 1c5276b to 2bcc4e3 Compare October 30, 2020 06:49

shipilev marked this pull request as ready for review October 30, 2020 06:52

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

coleenp approved these changes Oct 30, 2020

View reviewed changes

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

openjdk bot removed the ready Pull request is ready to be integrated label Oct 30, 2020

jerboaa approved these changes Oct 30, 2020

View reviewed changes

DamonFool approved these changes Nov 3, 2020

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Nov 3, 2020

shipilev added 4 commits November 3, 2020 11:00

Remove leftover comments

8ad79e9

Remove leftover variable

76c08f2

Also remove now unused BytecodeInterpreter::mdx field

e81a874

Merge branch 'master' into JDK-8255617-zero-purge-bi-profiliing

15df284

openjdk bot added hotspot hotspot-dev@openjdk.org and removed hotspot-runtime hotspot-runtime-dev@openjdk.org labels Nov 3, 2020

DamonFool approved these changes Nov 3, 2020

View reviewed changes

coleenp approved these changes Nov 3, 2020

View reviewed changes

openjdk bot closed this Nov 4, 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 Nov 4, 2020

openjdk-notifier bot referenced this pull request Nov 4, 2020

8255617: Zero: purge the remaining bytecode interpreter profiling sup…

a5d8a9c

…port Reviewed-by: coleenp, sgehwolf, jiefu

shipilev deleted the JDK-8255617-zero-purge-bi-profiliing branch November 4, 2020 13:19

8255617: Zero: purge the remaining bytecode interpreter profiling support #944

8255617: Zero: purge the remaining bytecode interpreter profiling support #944

shipilev commented Oct 29, 2020 •

edited by openjdk bot

bridgekeeper bot commented Oct 29, 2020

openjdk bot commented Oct 29, 2020

mlbridge bot commented Oct 30, 2020 •

edited

coleenp left a comment

openjdk bot commented Oct 30, 2020 •

edited

shipilev commented Oct 30, 2020 •

edited

openjdk bot commented Oct 30, 2020

jerboaa commented Oct 30, 2020

shipilev commented Nov 3, 2020

DamonFool commented Nov 3, 2020

DamonFool commented Nov 3, 2020

shipilev commented Nov 3, 2020

DamonFool left a comment

coleenp left a comment

shipilev commented Nov 4, 2020

openjdk bot commented Nov 4, 2020

8255617: Zero: purge the remaining bytecode interpreter profiling support #944

8255617: Zero: purge the remaining bytecode interpreter profiling support #944

Conversation

shipilev commented Oct 29, 2020 • edited by openjdk bot

Progress

Testing

Issue

Reviewers

Download

bridgekeeper bot commented Oct 29, 2020

openjdk bot commented Oct 29, 2020

mlbridge bot commented Oct 30, 2020 • edited

Webrevs

coleenp left a comment

Choose a reason for hiding this comment

openjdk bot commented Oct 30, 2020 • edited

shipilev commented Oct 30, 2020 • edited

openjdk bot commented Oct 30, 2020

jerboaa commented Oct 30, 2020

shipilev commented Nov 3, 2020

DamonFool commented Nov 3, 2020

DamonFool commented Nov 3, 2020

shipilev commented Nov 3, 2020

DamonFool left a comment

Choose a reason for hiding this comment

coleenp left a comment

Choose a reason for hiding this comment

shipilev commented Nov 4, 2020

openjdk bot commented Nov 4, 2020

shipilev commented Oct 29, 2020 •

edited by openjdk bot

mlbridge bot commented Oct 30, 2020 •

edited

openjdk bot commented Oct 30, 2020 •

edited

shipilev commented Oct 30, 2020 •

edited