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

8276824: refactor Thread::is_JavaThread_protected #6315

Closed
wants to merge 2 commits into from

Conversation

dcubed-ojdk
Copy link
Member

@dcubed-ojdk dcubed-ojdk commented Nov 9, 2021

Refactor Thread::is_JavaThread_protected(const JavaThread* p, bool checkTLHOnly)
into: bool Thread::is_JavaThread_protected(const JavaThread* p)
and: Thread::is_JavaThread_protected_by_TLH(const JavaThread* p).
Also change callers that passed checkTLHOnly == true into calls to
Thread::is_JavaThread_protected_by_TLH(const JavaThread* p).

This refactoring is being tested with Mach5 Tier[1-3] (in process).

In the "Files changed" view, enable "Settings -> Hide whitespace" for an easier review.


Progress

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

Issue

  • JDK-8276824: refactor Thread::is_JavaThread_protected

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 6315

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

Using diff file

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Nov 9, 2021

👋 Welcome back dcubed! 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.

@dcubed-ojdk
Copy link
Member Author

@dcubed-ojdk dcubed-ojdk commented Nov 9, 2021

/label add hotspot-runtime

@openjdk openjdk bot added the hotspot-runtime label Nov 9, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Nov 9, 2021

@dcubed-ojdk
The hotspot-runtime label was successfully added.

@dcubed-ojdk
Copy link
Member Author

@dcubed-ojdk dcubed-ojdk commented Nov 9, 2021

@coleenp, @dholmes-ora and @robehn - since the three of you were involved in the
review for: "JDK-8249004 Reduce ThreadsListHandle overhead in relation to direct handshakes"
I would like you to chime in on this refactoring. Thanks!

Advantages to this version (from my POV):

  • Thread::is_JavaThread_protected() is smaller and simpler without the flag.
  • Thread::is_JavaThread_protected_by_TLH() is very small and straight forward.

Disadvantages to this version (from my POV):

  • current_thread is materialized in both functions instead of just once.

@dholmes-ora says that materializing current_thread is no longer as expensive as
it used to be so I'm not worried about the disadvantage anymore.

@dcubed-ojdk dcubed-ojdk marked this pull request as ready for review Nov 9, 2021
@openjdk openjdk bot added the rfr label Nov 9, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Nov 9, 2021

Webrevs

coleenp
coleenp approved these changes Nov 9, 2021
Copy link
Contributor

@coleenp coleenp left a comment

Thank you for this refactoring. A small suggestion, we'll see if it's popular or not.

// Is the target JavaThread protected by a ThreadsListHandle (TLH) associated
// with the calling Thread?
//
bool Thread::is_JavaThread_protected_by_TLH(const JavaThread* p) {
Copy link
Contributor

@coleenp coleenp Nov 9, 2021

Choose a reason for hiding this comment

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

I think you could pass JavaThread* current as the first parameter here. The naming will help to keep it straight and make this an assert(current == JavaThread::current() if this is deemed to be expensive.
At the callsites below, it would make it obvious that "this" isn't meant to be the current thread (and calling Thread::current() inside a (tbd) assert would make it clear which thread is which.

With this handshake code I continuously am trying to figure out which thread is current vs. target.

Copy link
Member Author

@dcubed-ojdk dcubed-ojdk Nov 9, 2021

Choose a reason for hiding this comment

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

I'm not fond of passing the current thread in as a parameter because that could
make folks think that these functions can be called on any Thread*. They must
be called in the context of the current thread which is also why they are static.
I would like to rename the p parameter to target and I could add /*target*/
to the call sites to make things more clear.

@openjdk
Copy link

@openjdk openjdk bot commented Nov 9, 2021

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

8276824: refactor Thread::is_JavaThread_protected

Reviewed-by: coleenp, rehn, 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 23 new commits pushed to the master branch:

  • e91e9d8: 8276721: G1: Refine G1EvacFailureObjectsSet::iterate
  • 8822d41: 8274736: Concurrent read/close of SSLSockets causes SSLSessions to be invalidated unnecessarily
  • c1e41fe: 8276842: G1: Only calculate size in bytes from words when needed
  • c8b0ee6: 8276833: G1: Make G1EvacFailureRegions::par_iterate const
  • 0699220: 8268882: C2: assert(n->outcnt() != 0 || C->top() == n || n->is_Proj()) failed: No dead instructions after post-alloc
  • d7012fb: 8276880: Remove java/lang/RuntimeTests/exec/ExecWithDir as unnecessary
  • f9024d0: 8230130: javadoc search result dialog shows cut off headers for long results
  • 055de6f: 8223358: Incorrect HTML structure in annotation pages
  • a60e912: 8198626: java/awt/KeyboardFocusmanager/TypeAhead/TestDialogTypeAhead.html fails on mac
  • dde959d: 8276808: java/nio/channels/Channels/TransferTo.java timed out
  • ... and 13 more: https://git.openjdk.java.net/jdk/compare/905e3e88137d46f90de7034e9fc324e97af873a6...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 label Nov 9, 2021
robehn
robehn approved these changes Nov 9, 2021
@dcubed-ojdk
Copy link
Member Author

@dcubed-ojdk dcubed-ojdk commented Nov 9, 2021

I've pushed changes to clarify the target thread parameter.

@dcubed-ojdk
Copy link
Member Author

@dcubed-ojdk dcubed-ojdk commented Nov 9, 2021

@coleenp and @robehn - Thanks for reviewing.

coleenp
coleenp approved these changes Nov 9, 2021
Copy link
Contributor

@coleenp coleenp left a comment

Ok, thanks!

@dcubed-ojdk
Copy link
Member Author

@dcubed-ojdk dcubed-ojdk commented Nov 9, 2021

@robehn - are you okay with the comment updates?

Copy link
Member

@dholmes-ora dholmes-ora left a comment

Hi Dan,

I'm okay with the changes as-is but also would not object to passing the current thread to avoid manifesting it - we do that a lot in other parts of the VM.

One (multi-place) nit below.

Thanks,
David

@@ -1750,13 +1761,13 @@ void JavaThread::send_thread_stop(oop java_throwable) {
// - Target thread will not enter any new monitors.
//
bool JavaThread::java_suspend() {
guarantee(Thread::is_JavaThread_protected(this, /* checkTLHOnly */ true),
guarantee(Thread::is_JavaThread_protected_by_TLH(/* target */ this),
Copy link
Member

@dholmes-ora dholmes-ora Nov 10, 2021

Choose a reason for hiding this comment

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

Nit: there's no need to comment a single argument.

Copy link
Member Author

@dcubed-ojdk dcubed-ojdk Nov 12, 2021

Choose a reason for hiding this comment

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

I added the comment due to this comment from @coleenp:

At the callsites below, it would make it obvious that "this" isn't meant to be the current thread (and calling Thread::current() inside a (tbd) assert would make it clear which thread is which.

I read @coleenp's comment as meaning that the uncommented this could be made more
obvious so I'm going to leave those two /* target */ comments for clarity.

@dcubed-ojdk
Copy link
Member Author

@dcubed-ojdk dcubed-ojdk commented Nov 12, 2021

@dholmes-ora - Thanks for re-review! I think we're good to go on this cleanup.

/integrate

@openjdk
Copy link

@openjdk openjdk bot commented Nov 12, 2021

Going to push as commit 176d21d.
Since your change was applied there have been 69 commits pushed to the master branch:

  • 74f3e69: 8277071: [BACKOUT] JDK-8276743 Make openjdk build Zip Archive generation "reproducible"
  • b85500e: 8276123: ZipFile::getEntry will not return a file entry when there is a directory entry of the same name within a Zip File
  • 0d2980c: 8258192: Obsolete the CriticalJNINatives flag
  • 5a2452c: 8274835: Remove unnecessary castings in java.base
  • 3b2585c: 8276658: Clean up JNI local handles code
  • aeba653: 8276743: Make openjdk build Zip Archive generation "reproducible"
  • 51a5731: 8277016: Use blessed modifier order in jdk.httpserver
  • c4b4432: 8277012: Use blessed modifier order in src/utils
  • 13deb38: 8276848: sun.net.httpserver.simpleserver.CommandLinePositiveTest: test does not specify port
  • 710f496: 8273277: C2: Move conditional negation into rc_predicate
  • ... and 59 more: https://git.openjdk.java.net/jdk/compare/905e3e88137d46f90de7034e9fc324e97af873a6...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot closed this Nov 12, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Nov 12, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Nov 12, 2021

@dcubed-ojdk Pushed as commit 176d21d.

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

@dcubed-ojdk dcubed-ojdk deleted the JDK-8276824 branch Nov 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot-runtime integrated
4 participants