Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8268164: Adopt cast notation for WorkerThread conversions #4334

Closed
wants to merge 3 commits into from

Conversation

albertnetymk
Copy link
Member

@albertnetymk albertnetymk commented Jun 3, 2021

Followup of JDK-8267916 (#4240), the same refactoring for WorkerThread.


Progress

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

Issue

  • JDK-8268164: Adopt cast notation for WorkerThread conversions

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 4334

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

Using diff file

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

@bridgekeeper
Copy link

bridgekeeper bot commented Jun 3, 2021

👋 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 openjdk bot added the rfr Pull request is ready for review label Jun 3, 2021
@openjdk
Copy link

openjdk bot commented Jun 3, 2021

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

@openjdk openjdk bot added the hotspot hotspot-dev@openjdk.org label Jun 3, 2021
@mlbridge
Copy link

mlbridge bot commented Jun 3, 2021

Webrevs

Copy link
Member

@stefank stefank left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if it wouldn't be nicer to be able to write:

WorkerThread::current()->id()

@albertnetymk
Copy link
Member Author

I wonder if it wouldn't be nicer to be able to write:

WorkerThread::current()->id()

That will cause a compiling error:

src/hotspot/share/gc/shared/referenceProcessor.cpp:933:35: error: no member named 'id' in 'Thread'
    id = WorkerThread::current()->id();
         ~~~~~~~~~~~~~~~~~~~~~~~  ^
1 error generated.

WorkerThread doesn't have a static current() method, so it will get the one from Thread, which returns a Thread*. However, id() is only defined for WorkerThread, not Thread.

@stefank
Copy link
Member

stefank commented Jun 3, 2021

I wonder if it wouldn't be nicer to be able to write:

WorkerThread::current()->id()

That will cause a compiling error:

src/hotspot/share/gc/shared/referenceProcessor.cpp:933:35: error: no member named 'id' in 'Thread'
    id = WorkerThread::current()->id();
         ~~~~~~~~~~~~~~~~~~~~~~~  ^
1 error generated.

WorkerThread doesn't have a static current() method, so it will get the one from Thread, which returns a Thread*. However, id() is only defined for WorkerThread, not Thread.

Looks like you just called Thread::current() via WorkerThread. I mean that it would be nice if you could create a function that looked something like this:

static WorkerThread* WorkerThread::current() {
  return WorkerThread::cast(Thread::current());
}

@stefank
Copy link
Member

stefank commented Jun 3, 2021

I now see that they actually have this for the CompilerThread:
https://github.com/openjdk/jdk/pull/4240/files#diff-2dcac7a3550ebd199e8cffdeaabdba67643e794c6a6f9b0042a8cfe7dbaa9066

@albertnetymk
Copy link
Member Author

I mean that it would be nice if you could create a function that looked something like this...

I see. Updated.

}

static WorkerThread* cast(Thread* t) {
assert(t->is_Worker_thread(), "incorrect cast to const WorkerThread");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should probably not say const here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as_Worker_thread() used const so I wonder whether the new cast function should too?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean static const WorkerThread* cast(const Thread* t) { ... }? As the implementation of this method is nothing but a static_cast (and it will probably stay that way in the future), having const Thread* won't really catch anything interesting, IMO.

@openjdk
Copy link

openjdk bot commented Jun 3, 2021

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

8268164: Adopt cast notation for WorkerThread conversions

Reviewed-by: stefank, 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 48 new commits pushed to the master branch:

  • b05fa02: 8267904: C2 crash when compile negative Arrays.copyOf length after loop
  • 95ddf7d: 8267839: trivial mem leak in numa
  • 52d88ee: 8268292: compiler/intrinsics/VectorizedMismatchTest.java fails with release VMs
  • 042f0bd: 8256465: [macos] Java frame and dialog presented full screen freeze application
  • 8abf36c: 8268289: build failure due to missing signed flag in x86 evcmpb instruction
  • b05c40c: 8266951: Partial in-lining for vectorized mismatch operation using AVX512 masked instructions
  • f768fbf: 8268286: ProblemList serviceability/sa/TestJmapCore.java on linux-aarch64 with ZGC
  • b2e9eb9: 8268087: Update documentation of the JPasswordField
  • 91f9adc: 8268139: CDS ArchiveBuilder may reference unloaded classes
  • 36bff6f: 8066694: Strange code in JavacParser.java
  • ... and 38 more: https://git.openjdk.java.net/jdk/compare/a52a08d20be13721fcde65cad3567bbfa04f45cd...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.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Jun 3, 2021
@mlbridge
Copy link

mlbridge bot commented Jun 3, 2021

Mailing list message from David Holmes on hotspot-dev:

On 4/06/2021 6:56 am, Albert Mingkun Yang wrote:

On Thu, 3 Jun 2021 13:02:56 GMT, David Holmes <dholmes at openjdk.org> wrote:

src/hotspot/share/runtime/nonJavaThread.hpp line 111:

109:
110: static WorkerThread* cast(Thread* t) {
111: assert(t->is_Worker_thread(), "incorrect cast to const WorkerThread");

It should probably not say *const* here.

`as_Worker_thread()` used const so I wonder whether the new cast function should too?

You mean `static const WorkerThread* cast(const Thread* t) { ... }`? As the implementation of this method is nothing but a `static_cast` (and it will probably stay that way in the future), having `const Thread*` won't really catch anything interesting, IMO.

Okay - just asking the question. :) For JavaThread we have a const and
non-const version (not sure why though).

David

@albertnetymk
Copy link
Member Author

For JavaThread we have a const and non-const version (not sure why though).

as_Java_thread is called with const Thread* in two places, listed below. The criteria of having a const Thread*-version cast method hinges on the caller, whether it has a const Thread* and/or wants a const XThread* in return. as_Java_thread is called both with and with const, while XThread* cast(Thread* t) is enough for others currently.

traceid JfrThreadId::id(const Thread* t);

const char* JfrThreadName::name(const Thread* t);

@albertnetymk
Copy link
Member Author

Thanks for the review.

/integrate

@openjdk openjdk bot closed this Jun 7, 2021
@openjdk 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 Jun 7, 2021
@openjdk
Copy link

openjdk bot commented Jun 7, 2021

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

  • 9fc914b: 8204686: Dynamic parallel reference processing support for Parallel GC
  • 908aca2: 8262891: Compiler implementation for Pattern Matching for switch (Preview)
  • 3e48244: 8268301: Closed test: compiler/c2/6371167/Test.java fails after JDK-8267904
  • 204b492: 8267703: runtime/cds/appcds/cacheObject/HeapFragmentationTest.java crashed with OutOfMemory
  • 2aeeeb4: 8268279: gc/shenandoah/compiler/TestLinkToNativeRBP.java fails after LibraryLookup is gone
  • b05fa02: 8267904: C2 crash when compile negative Arrays.copyOf length after loop
  • 95ddf7d: 8267839: trivial mem leak in numa
  • 52d88ee: 8268292: compiler/intrinsics/VectorizedMismatchTest.java fails with release VMs
  • 042f0bd: 8256465: [macos] Java frame and dialog presented full screen freeze application
  • 8abf36c: 8268289: build failure due to missing signed flag in x86 evcmpb instruction
  • ... and 43 more: https://git.openjdk.java.net/jdk/compare/a52a08d20be13721fcde65cad3567bbfa04f45cd...master

Your commit was automatically rebased without conflicts.

Pushed as commit 58bdabc.

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

@albertnetymk albertnetymk deleted the as_worker_thread branch June 7, 2021 08:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot hotspot-dev@openjdk.org integrated Pull request has been integrated
3 participants