JDK-8267916: Adopt cast notation for CompilerThread conversions

[albertnetymk]

Simple refactoring around as_CompilerThread. A new file, compilerThread.inline.hpp, is created to get around the circular dependency.

---

Progress

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

Issue

• JDK-8267916: Adopt cast notation for CompilerThread conversions

Reviewers

• Kim Barrett (@kimbarrett - Reviewer)
• Ioi Lam (@iklam - Reviewer)
• David Holmes (@dholmes-ora - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/4240/head:pull/4240
$ git checkout pull/4240

Update a local copy of the PR:
$ git checkout pull/4240
$ git pull https://git.openjdk.java.net/jdk pull/4240/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 4240

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/4240.diff

[bridgekeeper]

👋 Welcome back ayang! 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]

@albertnetymk 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]

Webrevs

• 01: Full - Incremental (c919ae36)
• 00: Full (cc3141b2)

[dholmes-ora]

Sorry I'm not seeing the need for a new compilerThread.inline.hpp. That isn't making things consistent AFAICS - if we haven't factored out CompilerThread into its own header we shouldn't have compilerThread.inline.hpp.

David

[albertnetymk]

That isn't making things consistent

It's the renaming and move to thread.inline.hpp (two points covered in the ticket description) that is supposed to make it more consistent. The creation of compilerThread.inline.hpp is just to work around the circular dependency, CompilerThread::current() in compilerThread.hpp calls JavaThread::as_CompilerThread (in thread.inline.hpp after the move).

[dholmes-ora]

Sorry I looked at this too late at night:

if we haven't factored out CompilerThread into its own header

but we do of course have that.

[albertnetymk]

I encountered some circular dependencies after moving the definition to thread.inline.hpp, so I replaced it with a static cast method, as Kim instructed from an offline discussion. Note now there's a discrepancy: CompilerThread::cast(t) vs t->as_Worker_thread() (or t->as_Java_thread()). I can do the same for other as_*_thread methods as well (in this PR or another one) if people feel like this approach.

[kimbarrett]

Looks good.

[kimbarrett]

I encountered some circular dependencies after moving the definition to thread.inline.hpp, so I replaced it with a static cast method, as Kim instructed from an offline discussion. Note now there's a discrepancy: CompilerThread::cast(t) vs t->as_Worker_thread() (or t->as_Java_thread()). I can do the same for other as_*_thread methods as well (in this PR or another one) if people feel like this approach.

as_Worker_thread => WorkerThread::cast could pretty easily be added to this change. There's only one call.

There are lots of as_Java_thread calls. Maybe check with runtime team how they feel about the inconsistency vs code churn.

[iklam]

I encountered some circular dependencies after moving the definition to thread.inline.hpp, so I replaced it with a static cast method, as Kim instructed from an offline discussion. Note now there's a discrepancy: CompilerThread::cast(t) vs t->as_Worker_thread() (or t->as_Java_thread()). I can do the same for other as_*_thread methods as well (in this PR or another one) if people feel like this approach.

as_Worker_thread => WorkerThread::cast could pretty easily be added to this change. There's only one call.

There are lots of as_Java_thread calls. Maybe check with runtime team how they feel about the inconsistency vs code churn.

I prefer JavaThread::cast() over Thread::as_Java_thread(). The former style is consistent with other functions such as InstanceKlass:cast().

As far as code churn, JDK-8252685 already removed a bunch of as_Java_thread() calls (we have about 110 of them now, vs about 180 in JDK 16), so we may as well fix the rest for a more consistent style, which is also better with header dependencies. @dholmes-ora what do you think? In any case, we should do that in a different PR.

[iklam]

This looks good to me. If we decide to use Compiler::cast, the RFE title should be updated before integration.

[dholmes-ora]

The change to CompilerThread::cast doesn't seem necessary to address any circular header dependencies - what am I missing?

I personally prefer the as_X() style for thread casts (though X is spelt incorrectly in most places), but cast is a more traditional name and used for other type hierarchies in the VM. So we can change this for other thread types in a separate RFE.

As Ioi notes, this RFE needs a new synopsis: "Adopt cast notation for CompilerThread conversions" or something like that.

Thanks,
David

[dholmes-ora]

The change to CompilerThread::cast doesn't seem necessary to address any circular header dependencies - what am I missing?

Doh! I was missing the fact the cast method goes in compilerThread.hpp not thread.hpp.

[openjdk]

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

8267916: Adopt cast notation for CompilerThread conversions

Reviewed-by: kbarrett, iklam, dholmes

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

• 9bf347b: 8226362: langtools ProblemList file contains more than 1 entry for a test
• bdeaeb4: 8240256: Better resource cleaning for SunPKCS11 Provider
• 06f87cf: 8266337: ThreadTimesClosure doesn't handle exceptions properly
• ef01e47: 8268150: tier2: test/jdk/tools/jpackage/junit/junit.java needs updating for jtreg 6
• e9f3e32: 8259530: Generated docs contain MIT/GPL-licenced works without reproducing the licence
• d46a2c8: 8268147: need to update reference to testng module for jtreg6
• 76fdf2c: 8268146: fix for JDK-8266254 fails validate-source
• 136badb: 8248268: Support KWP in addition to KW
• 3482cb8: 8266254: Update to use jtreg 6
• 375f8f3: 8268138: docs build error after JDK-8263332 integration
• ... and 88 more: https://git.openjdk.java.net/jdk/compare/bfa46f0af30e9ec073ab5e6411ca94e140113790...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.

[albertnetymk]

as_Worker_thread => WorkerThread::cast could pretty easily be added to this change. There's only one call.

I will do that in another PR to keep this one more focused.

As Ioi notes, this RFE needs a new synopsis: "Adopt cast notation for CompilerThread conversions" or something like that.

Updated.

Thank you all for the suggestions and reviews.

/integrate

[openjdk]

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

• 9bf347b: 8226362: langtools ProblemList file contains more than 1 entry for a test
• bdeaeb4: 8240256: Better resource cleaning for SunPKCS11 Provider
• 06f87cf: 8266337: ThreadTimesClosure doesn't handle exceptions properly
• ef01e47: 8268150: tier2: test/jdk/tools/jpackage/junit/junit.java needs updating for jtreg 6
• e9f3e32: 8259530: Generated docs contain MIT/GPL-licenced works without reproducing the licence
• d46a2c8: 8268147: need to update reference to testng module for jtreg6
• 76fdf2c: 8268146: fix for JDK-8266254 fails validate-source
• 136badb: 8248268: Support KWP in addition to KW
• 3482cb8: 8266254: Update to use jtreg 6
• 375f8f3: 8268138: docs build error after JDK-8263332 integration
• ... and 88 more: https://git.openjdk.java.net/jdk/compare/bfa46f0af30e9ec073ab5e6411ca94e140113790...master

Your commit was automatically rebased without conflicts.

Pushed as commit a52a08d.

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