8288623: Move Continuation classes out of javaClasses.hpp #9191

iklam · 2022-06-16T19:12:25Z

javaClasses.hpp is getting too big - it contains the C++ representation of over 50 Java classes.

The RFE moves the following classes into a new file, continuationJavaClasses.hpp. The naming follows the same pattern as the existing header share/jvmci/jvmciJavaClasses.hpp.

jdk_internal_vm_ContinuationScope
jdk_internal_vm_Continuation
jdk_internal_vm_StackChunk

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-8288623: Move Continuation classes out of javaClasses.hpp

Reviewers

Stefan Karlsson (@stefank - Reviewer) ⚠️ Review applies to a2179280
@kristylee88 (no known github.com user name / role) ⚠️ Review applies to ebc3f9f8
Coleen Phillimore (@coleenp - Reviewer) ⚠️ Review applies to a2179280

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 9191

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

Using diff file

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

bridgekeeper · 2022-06-16T19:13:59Z

👋 Welcome back iklam! 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-06-16T19:16:26Z

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

mlbridge · 2022-06-16T20:02:23Z

Webrevs

stefank

If we are about to start to squirrel away parts of javaClasses into new files, I wonder if the discoverability of these functions will be better if we use this naming instead:

javaClasses.hpp
javaClassesContinuation.hpp
javaClassesInvoke.hpp
...

instead of:

javaClasses.hpp
continuationJavaClasses.hpp
invokeJavaClasses.hpp
...

stefank · 2022-06-17T08:35:36Z

src/hotspot/share/classfile/continuationJavaClasses.inline.hpp

+#ifndef SHARE_CLASSFILE_JDK_INTERNAL_VM_STACKCHUNK_INLINE_HPP
+#define SHARE_CLASSFILE_JDK_INTERNAL_VM_STACKCHUNK_INLINE_HPP


This include guard doesn't follow the current naming convention.

This has not been resolved.

coleenp · 2022-06-17T14:34:35Z

The naming of this follows what jvmci did - they have jvmciJavaClasses.hpp/cpp. Discovering continuation* is also something useful.

iklam · 2022-06-17T15:23:14Z

If we are about to start to squirrel away parts of javaClasses into new files, I wonder if the discoverability of these functions will be better if we use this naming instead:
javaClasses.hpp
javaClassesContinuation.hpp
javaClassesInvoke.hpp
...
instead of:
javaClasses.hpp
continuationJavaClasses.hpp
invokeJavaClasses.hpp
...

I am not in love with either naming convention (the names are too long), but the latter sounds more grammatically correct. Also, as Coleen mentioned, if we choose the former, we need to rename jvmciJavaClasses.hpp as well.

The former can be discovered as javaClasses* and the latter as *avaClasses*, so there's not much difference.

stefank · 2022-06-17T18:32:52Z

src/hotspot/share/classfile/javaClassesImpl.hpp

+#include "classfile/javaClasses.hpp"
+#include "classfile/continuationJavaClasses.hpp"


Include order

stefank · 2022-06-17T18:33:09Z

src/hotspot/share/gc/g1/g1CollectedHeap.inline.hpp

+#include "oops/stackChunkOop.hpp"
 #include "gc/g1/g1BarrierSet.hpp"


Include order

stefank · 2022-06-17T18:33:43Z

src/hotspot/share/classfile/javaClassesImpl.hpp

+#ifndef SHARE_CLASSFILE_JAVACLASSES_IMPL_HPP
+#define SHARE_CLASSFILE_JAVACLASSES_IMPL_HPP


Convention is to name this:
SHARE_CLASSFILE_JAVACLASSESIMPL_HPP

stefank · 2022-06-17T18:34:57Z

src/hotspot/share/classfile/continuationJavaClasses.cpp

+#include "classfile/javaClassesImpl.hpp"
+#include "classfile/javaClasses.hpp"
+#include "classfile/continuationJavaClasses.hpp"


Include order

stefank · 2022-06-17T20:18:56Z

I've talked to Ioi and Coleen about whether javaClasses or continuations should be considered as the most relevant subsystem. Their take is that continuations is. I'm warming up to that thought, but I think I'd then would prefer if continuationsJavaClasses.hpp etc. get moved to where the other continuation files are, in the runtime/ directory.

coleenp

I like this split very much. JavaClasses was getting too unwieldy. I'm ambivalent whether continuationJavaClasses.cpp/hpp should go in the runtime directory, so please take Stefan's recommendation and move it there. Tnanks.

openjdk · 2022-06-21T11:58:07Z

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

8288623: Move Continuation classes out of javaClasses.hpp

Reviewed-by: stefank, coleenp

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 no new commits pushed to the master branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential 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.

stefank

I started to add comments to the PR, but then realized it would be easier to just provide a patch with my suggestions. Please take a look at:
https://github.com/stefank/jdk/tree/pr_9191

stefank · 2022-06-22T06:01:07Z

src/hotspot/share/classfile/continuationJavaClasses.inline.hpp

 #include "classfile/javaClasses.hpp"
-#include "classfile/continuationJavaClasses.hpp"
-#include "oops/access.inline.hpp"
-#include "oops/stackChunkOop.hpp"
 #include "logging/log.hpp"
+#include "oops/access.inline.hpp"
 #include "oops/stackChunkOop.inline.hpp"
+#include "runtime/continuationJavaClasses.hpp"


The .inline.hpp files should include the corresponding .hpp file as the first included file. This is a fairly recently addition to the style guide, that we did to resolve the compilation problems with circular dependencies between the .inline.hpp files. So, please change this to:

#include "runtime/continuationJavaClasses.hpp" #include "classfile/javaClasses.hpp" #include "logging/log.hpp" #include "oops/access.inline.hpp" #include "oops/stackChunkOop.inline.hpp"

stefank · 2022-06-22T06:06:55Z

src/hotspot/share/classfile/continuationJavaClasses.inline.hpp

+#ifndef SHARE_CLASSFILE_JDK_INTERNAL_VM_STACKCHUNK_INLINE_HPP
+#define SHARE_CLASSFILE_JDK_INTERNAL_VM_STACKCHUNK_INLINE_HPP


This has not been resolved.

stefank · 2022-06-22T06:56:17Z

src/hotspot/share/runtime/continuationJavaClasses.cpp

+ */
+
+#include "precompiled.hpp"
+#include "classfile/javaClasses.hpp"


Why is classfile/javaClasses.hpp included in this file?

stefank · 2022-06-22T06:59:05Z

src/hotspot/share/runtime/continuationJavaClasses.hpp

+#define SHARE_CLASSFILE_CONTINUATIONJAVACLASSES_HPP
+
+#include "memory/allStatic.hpp"
+#include "oops/oop.hpp"


oop isn't used in this function, include oopsHierarchy.hpp instead. This file should also include globalDefinitions.hpp and macros.hpp

stefank · 2022-06-22T06:59:20Z

src/hotspot/share/runtime/continuationJavaClasses.hpp

+#ifndef SHARE_CLASSFILE_CONTINUATIONJAVACLASSES_HPP
+#define SHARE_CLASSFILE_CONTINUATIONJAVACLASSES_HPP


Include guard needs to be updated.

stefank · 2022-06-22T08:08:31Z

src/hotspot/share/classfile/javaClassesImpl.hpp

+
+

spurious newline

stefank · 2022-06-22T08:09:16Z

src/hotspot/share/classfile/javaClasses.inline.hpp

@@ -220,159 +218,6 @@ inline void java_lang_Thread::set_jfr_epoch(oop ref, u2 epoch) {
 }
 #endif // INCLUDE_JFR



Spurious newline

stefank · 2022-06-22T08:29:07Z

src/hotspot/share/classfile/continuationJavaClasses.cpp

@@ -0,0 +1,110 @@
+/*


This file should also move to runtime/

openjdk · 2022-06-22T17:30:30Z

@iklam 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 8288623-move-continuation-out-of-javaClasses-hpp
git fetch https://git.openjdk.org/jdk master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

…es-hpp

…split

iklam · 2022-06-22T18:09:50Z

Hi @stefank, thank you so much for providing the additional fixes. I have incorporated them into my PR and also synced with Coleen's javaThread.hpp split PR (#9218). Doing more thorough build testing now.

…es-hpp

iklam · 2022-06-24T04:28:41Z

Thanks to @stefank and @coleenp for the review.

Passes our CI tier1 and build-tier5.
GHA linux-x86 errors are unrelated to my changes.

/integrate

openjdk · 2022-06-24T04:29:59Z

Going to push as commit 64782a7.

openjdk · 2022-06-24T04:30:18Z

@iklam Pushed as commit 64782a7.

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

8288623: Move Continuation classes out of javaClasses.hpp

001c215

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

iklam marked this pull request as ready for review June 16, 2022 19:56

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

added missing includes that were hidden by previously unnedded includes

caf085a

stefank reviewed Jun 17, 2022

View reviewed changes

fixed include guard macro and copyright year

ebc3f9f

stefank reviewed Jun 17, 2022

View reviewed changes

@stefank review comments

145e44a

coleenp approved these changes Jun 21, 2022

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jun 21, 2022

moved to runtime/continuationJavaClasses.hpp

9d998d7

stefank suggested changes Jun 22, 2022

View reviewed changes

Review comments

2012fc6

openjdk bot added merge-conflict Pull request has merge conflict with target branch and removed ready Pull request is ready to be integrated labels Jun 22, 2022

iklam added 2 commits June 22, 2022 10:35

Merge branch 'master' into 8288623-move-continuation-out-of-javaClass…

2b78367

…es-hpp

added missing javaClasses.inline.hpp after merge with javaThread.hpp …

6798bb4

…split

openjdk bot added ready Pull request is ready to be integrated and removed merge-conflict Pull request has merge conflict with target branch labels Jun 22, 2022

fixed GHA build failures

9adf490

one more missing header

a217928

stefank approved these changes Jun 23, 2022

View reviewed changes

coleenp approved these changes Jun 23, 2022

View reviewed changes

Merge branch 'master' into 8288623-move-continuation-out-of-javaClass…

cd06342

…es-hpp

openjdk bot added the integrated Pull request has been integrated label Jun 24, 2022

openjdk bot closed this Jun 24, 2022

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jun 24, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8288623: Move Continuation classes out of javaClasses.hpp #9191

8288623: Move Continuation classes out of javaClasses.hpp #9191

iklam commented Jun 16, 2022 •

edited by openjdk bot

bridgekeeper bot commented Jun 16, 2022

openjdk bot commented Jun 16, 2022

mlbridge bot commented Jun 16, 2022 •

edited

stefank left a comment

stefank Jun 17, 2022

stefank Jun 22, 2022

coleenp commented Jun 17, 2022

iklam commented Jun 17, 2022

stefank Jun 17, 2022

stefank Jun 17, 2022

stefank Jun 17, 2022

stefank Jun 17, 2022

stefank commented Jun 17, 2022 •

edited

coleenp left a comment

openjdk bot commented Jun 21, 2022 •

edited

stefank left a comment

stefank Jun 22, 2022

stefank Jun 22, 2022

stefank Jun 22, 2022

stefank Jun 22, 2022

stefank Jun 22, 2022

stefank Jun 22, 2022

stefank Jun 22, 2022

stefank Jun 22, 2022

openjdk bot commented Jun 22, 2022

iklam commented Jun 22, 2022

iklam commented Jun 24, 2022

openjdk bot commented Jun 24, 2022

openjdk bot commented Jun 24, 2022

		#ifndef SHARE_CLASSFILE_JDK_INTERNAL_VM_STACKCHUNK_INLINE_HPP
		#define SHARE_CLASSFILE_JDK_INTERNAL_VM_STACKCHUNK_INLINE_HPP

		#include "classfile/javaClasses.hpp"
		#include "classfile/continuationJavaClasses.hpp"

		#include "oops/stackChunkOop.hpp"
		#include "gc/g1/g1BarrierSet.hpp"

		#ifndef SHARE_CLASSFILE_JAVACLASSES_IMPL_HPP
		#define SHARE_CLASSFILE_JAVACLASSES_IMPL_HPP

		#ifndef SHARE_CLASSFILE_CONTINUATIONJAVACLASSES_HPP
		#define SHARE_CLASSFILE_CONTINUATIONJAVACLASSES_HPP

		@@ -220,159 +218,6 @@ inline void java_lang_Thread::set_jfr_epoch(oop ref, u2 epoch) {
		}
		#endif // INCLUDE_JFR

8288623: Move Continuation classes out of javaClasses.hpp #9191

8288623: Move Continuation classes out of javaClasses.hpp #9191

Conversation

iklam commented Jun 16, 2022 • edited by openjdk bot

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Jun 16, 2022

openjdk bot commented Jun 16, 2022

mlbridge bot commented Jun 16, 2022 • edited

Webrevs

stefank left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coleenp commented Jun 17, 2022

iklam commented Jun 17, 2022

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

stefank commented Jun 17, 2022 • edited

coleenp left a comment

Choose a reason for hiding this comment

openjdk bot commented Jun 21, 2022 • edited

stefank 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

Choose a reason for hiding this comment

openjdk bot commented Jun 22, 2022

iklam commented Jun 22, 2022

iklam commented Jun 24, 2022

openjdk bot commented Jun 24, 2022

openjdk bot commented Jun 24, 2022

iklam commented Jun 16, 2022 •

edited by openjdk bot

mlbridge bot commented Jun 16, 2022 •

edited

stefank commented Jun 17, 2022 •

edited

openjdk bot commented Jun 21, 2022 •

edited