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

8268902: Testing for threadObj != NULL is unnecessary in handshake #4512

Closed
wants to merge 1 commit into from

Conversation

coleenp
Copy link
Contributor

@coleenp coleenp commented Jun 16, 2021

The handshake code tests if the JavaThread->is_exiting or that the threadObj() is null. Ever since JDK-8244997, once the JavaThread is running, the _threadObj won't be null until JavaThread is destroyed. So testing is_exiting is all you need to do.
In gtest, the test JavaThread doesn't create a _threadObj JDK-8215948 so removing this unnecessary test allows writing gtests for handshakes.

Tested with tier1-6.


Progress

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

Issue

  • JDK-8268902: Testing for threadObj != NULL is unnecessary in handshake

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 4512

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

Using diff file

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

@bridgekeeper
Copy link

bridgekeeper bot commented Jun 16, 2021

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

@coleenp
Copy link
Contributor Author

coleenp commented Jun 16, 2021

We can't remove all the checks for threadObj for NULL since there are places, particularly in JVMTI that use this test for threads that are newly created.

@openjdk openjdk bot added the rfr Pull request is ready for review label Jun 16, 2021
@openjdk
Copy link

openjdk bot commented Jun 16, 2021

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

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

mlbridge bot commented Jun 16, 2021

Webrevs

Comment on lines +627 to +629
assert(_handshakee->threadObj() != NULL,
"null threadobj is impossible because we're handshaking while the thread is being created");
if (_handshakee->is_exiting()) {
Copy link
Member

Choose a reason for hiding this comment

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

The assert() is racy when it comes before the _handshakee->is_exiting() check.
It is possible for the target thread (_handshakee) to set the exiting condition:

src/hotspot/share/runtime/thread.cpp: JavaThread::exit():

    set_terminated(_thread_exiting);

and call:

  ensure_join(this);

which clears the _threadObj field while the caller has just
called HandshakeState::suspend_with_handshake().
The caller would then fail the assert() when racing with the
exiting thread when what you want it to do is detect the
exiting condition and return false.

Copy link
Member

Choose a reason for hiding this comment

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

Update: After re-reading more of the code, I think I have to change
my comment here. The code above (HandshakeState::suspend_with_handshake())
is called from SuspendThreadHandshake::do_thread() which is a synchronous
handshake. The caller requesting the synchronous handshake cannot proceed until
the target thread is handshakeable. When the target thread is in JavaThread::exit()
it is in thread state _thread_in_vm and that's not a safe/handshakeable state so
the calling thread must wait for the target thread to become handshakeable or for
the exiting condition to be true. No race here is possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the _handshakee thread - or target thread can't do suspend_with_handshake unless it's in a safe state, so the ensure_join() function must have completed or not been called. Also ensure_join() clears the ThreadObj's Thread field but the JavaThread->_threadObj field isn't cleared:

java_lang_Thread::set_thread(threadObj(), NULL);

The _threadObj field isn't cleared until the JavaThread* destructor so this code should not have a reference to JavaThread at this point.

Copy link
Member

@dcubed-ojdk dcubed-ojdk left a comment

Choose a reason for hiding this comment

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

Thumbs up (after revising my comments).

@openjdk
Copy link

openjdk bot commented Jun 16, 2021

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

8268902: Testing for threadObj != NULL is unnecessary in handshake

Reviewed-by: dcubed

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

  • c20f80a: 8259066: Obsolete -XX:+AlwaysLockClassLoader
  • e4908a4: 8268778: CDS check_excluded_classes needs DumpTimeTable_lock

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
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 16, 2021
@coleenp
Copy link
Contributor Author

coleenp commented Jun 16, 2021

Thanks for the review, Dan!

Copy link
Member

@dholmes-ora dholmes-ora left a comment

Choose a reason for hiding this comment

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

Hi Coleen,

What about an attaching thread that hits a handshake when trying to allocate the java.lang.Thread object?

David

@mlbridge
Copy link

mlbridge bot commented Jun 21, 2021

Mailing list message from Coleen Phillimore on hotspot-runtime-dev:

On 6/21/21 12:28 AM, David Holmes wrote:

On Wed, 16 Jun 2021 16:03:04 GMT, Coleen Phillimore <coleenp at openjdk.org> wrote:

The handshake code tests if the JavaThread->is_exiting or that the threadObj() is null. Ever since JDK-8244997, once the JavaThread is running, the _threadObj won't be null until JavaThread is destroyed. So testing is_exiting is all you need to do.
In gtest, the test JavaThread doesn't create a _threadObj JDK-8215948 so removing this unnecessary test allows writing gtests for handshakes.

Tested with tier1-6.
Hi Coleen,

What about an attaching thread that hits a handshake when trying to allocate the java.lang.Thread object?

The attaching thread can't be handshake because it hasn't been added to
the Threads list until threadObj is initialized.? As far as I can tell,
you can't handshake a thread that's not on the list.

Coleen

@mlbridge
Copy link

mlbridge bot commented Jun 22, 2021

Mailing list message from David Holmes on hotspot-runtime-dev:

On 22/06/2021 1:41 am, Coleen Phillimore wrote:

On 6/21/21 12:28 AM, David Holmes wrote:

On Wed, 16 Jun 2021 16:03:04 GMT, Coleen Phillimore
<coleenp at openjdk.org> wrote:

The handshake code tests if the JavaThread->is_exiting or that the
threadObj() is null. Ever since JDK-8244997, once the JavaThread is
running, the _threadObj won't be null until JavaThread is destroyed.
So testing is_exiting is all you need to do.
In gtest, the test JavaThread doesn't create a _threadObj JDK-8215948
so removing this unnecessary test allows writing gtests for handshakes.

Tested with tier1-6.
Hi Coleen,

What about an attaching thread that hits a handshake when trying to
allocate the java.lang.Thread object?

The attaching thread can't be handshake because it hasn't been added to
the Threads list until threadObj is initialized.? As far as I can tell,
you can't handshake a thread that's not on the list.

I'm not sure what code you are looking at but attach_current_thread does:

// This thread will not do a safepoint check, since it has
// not been added to the Thread list yet.
{ MutexLocker ml(Threads_lock);
thread->set_active_handles(JNIHandleBlock::allocate_block());
Threads::add(thread, daemon);
}

and the thread is now on the threads-list; then it does:

thread->allocate_threadObj(thread_group, thread_name, daemon, THREAD);

after that. And that code does:

instanceHandle thread_oop = ik->allocate_instance_handle(CHECK);

java_lang_Thread::set_thread(thread_oop(), this);
java_lang_Thread::set_priority(thread_oop(), NormPriority);
set_threadObj(thread_oop());

The thread must be on the threads-list and able to participate in
safepoints because it is going to allocate a Java object. After this
point it also executes Java code - so again absolutely must be on the
threads-list already.

David
-----

Coleen

1 similar comment
@mlbridge
Copy link

mlbridge bot commented Jun 22, 2021

Mailing list message from David Holmes on hotspot-runtime-dev:

On 22/06/2021 1:41 am, Coleen Phillimore wrote:

On 6/21/21 12:28 AM, David Holmes wrote:

On Wed, 16 Jun 2021 16:03:04 GMT, Coleen Phillimore
<coleenp at openjdk.org> wrote:

The handshake code tests if the JavaThread->is_exiting or that the
threadObj() is null. Ever since JDK-8244997, once the JavaThread is
running, the _threadObj won't be null until JavaThread is destroyed.
So testing is_exiting is all you need to do.
In gtest, the test JavaThread doesn't create a _threadObj JDK-8215948
so removing this unnecessary test allows writing gtests for handshakes.

Tested with tier1-6.
Hi Coleen,

What about an attaching thread that hits a handshake when trying to
allocate the java.lang.Thread object?

The attaching thread can't be handshake because it hasn't been added to
the Threads list until threadObj is initialized.? As far as I can tell,
you can't handshake a thread that's not on the list.

I'm not sure what code you are looking at but attach_current_thread does:

// This thread will not do a safepoint check, since it has
// not been added to the Thread list yet.
{ MutexLocker ml(Threads_lock);
thread->set_active_handles(JNIHandleBlock::allocate_block());
Threads::add(thread, daemon);
}

and the thread is now on the threads-list; then it does:

thread->allocate_threadObj(thread_group, thread_name, daemon, THREAD);

after that. And that code does:

instanceHandle thread_oop = ik->allocate_instance_handle(CHECK);

java_lang_Thread::set_thread(thread_oop(), this);
java_lang_Thread::set_priority(thread_oop(), NormPriority);
set_threadObj(thread_oop());

The thread must be on the threads-list and able to participate in
safepoints because it is going to allocate a Java object. After this
point it also executes Java code - so again absolutely must be on the
threads-list already.

David
-----

Coleen

@coleenp
Copy link
Contributor Author

coleenp commented Jun 22, 2021

I am looking at all the threads we create in the vm like the ServiceThread (look for Threads::add()), but not attach_current_thread in jni. You're right, we have to make this thread safepoint here.
I wonder why some threads we create call ThreadGroup.add and some don't. There's lots of duplicate code that's slightly different for each.
Unfortunately now I need a fix for JDK-8215948, hopefully without another duplicate copy of this thread creation code so I can write my handshake test.

@coleenp coleenp closed this Jun 22, 2021
@coleenp coleenp deleted the threadobj branch June 22, 2021 14:37
@mlbridge
Copy link

mlbridge bot commented Jun 22, 2021

Mailing list message from David Holmes on hotspot-runtime-dev:

On 23/06/2021 12:39 am, Coleen Phillimore wrote:

On Wed, 16 Jun 2021 16:03:04 GMT, Coleen Phillimore <coleenp at openjdk.org> wrote:

The handshake code tests if the JavaThread->is_exiting or that the threadObj() is null. Ever since JDK-8244997, once the JavaThread is running, the _threadObj won't be null until JavaThread is destroyed. So testing is_exiting is all you need to do.
In gtest, the test JavaThread doesn't create a _threadObj JDK-8215948 so removing this unnecessary test allows writing gtests for handshakes.

Tested with tier1-6.

I am looking at all the threads we create in the vm like the ServiceThread (look for Threads::add()), but not attach_current_thread in jni. You're right, we have to make this thread safepoint here.
I wonder why some threads we create call ThreadGroup.add and some don't. There's lots of duplicate code that's slightly different for each.

I'm not sure on the "why". Some internal threads (e.g. AttachListener)
don't call the j.l.Thread constructor and so have to directly call the
ThreadGroup.add method. I suspect because these are not true Java
threads that there is some Java-side logic that Thread construction
performs which is not suitable for these internal threads.

Unfortunately now I need a fix for JDK-8215948, hopefully without another duplicate copy of this thread creation code so I can write my handshake test.

Okay guess I'd better look at it again then. :) I can determine which
variant of the j.l.Thread creation logic is suitable and then factor
that out into a support method like Thread::create_internal_java_thread().

David
-----

@mlbridge
Copy link

mlbridge bot commented Jun 23, 2021

Mailing list message from Coleen Phillimore on hotspot-runtime-dev:

On 6/22/21 6:59 PM, David Holmes wrote:

On 23/06/2021 12:39 am, Coleen Phillimore wrote:

On Wed, 16 Jun 2021 16:03:04 GMT, Coleen Phillimore
<coleenp at openjdk.org> wrote:

The handshake code tests if the JavaThread->is_exiting or that the
threadObj() is null. Ever since JDK-8244997, once the JavaThread is
running, the _threadObj won't be null until JavaThread is destroyed.
So testing is_exiting is all you need to do.
In gtest, the test JavaThread doesn't create a _threadObj
JDK-8215948 so removing this unnecessary test allows writing gtests
for handshakes.

Tested with tier1-6.

I am looking at all the threads we create in the vm like the
ServiceThread (look for Threads::add()), but not
attach_current_thread in jni.? You're right, we have to make this
thread safepoint here.
I wonder why some threads we create call ThreadGroup.add and some
don't.? There's lots of duplicate code that's slightly different for
each.

I'm not sure on the "why". Some internal threads (e.g. AttachListener)
don't call the j.l.Thread constructor and so have to directly call the
ThreadGroup.add method. I suspect because these are not true Java
threads that there is some Java-side logic that Thread construction
performs which is not suitable for these internal threads.

Seems that ServiceTherad, MonitorDeflationThread and JFRRecorderThread
do not call ThreadGroup.add, but NotificationThread, SignalThread (for
-xrs), AttachListener, JNI attached threads, don't know JvmtiAgent
threads, and JVM_StartThread (calls JavaThread::prepare).

There's lots of variations with similar code to create threads.

Unfortunately now I need a fix for JDK-8215948, hopefully without
another duplicate copy of this thread creation code so I can write my
handshake test.

Okay guess I'd better look at it again then. :) I can determine which
variant of the j.l.Thread creation logic is suitable and then factor
that out into a support method like
Thread::create_internal_java_thread().

Please!? Refactoring would be very nice!

Thanks,
Coleen

David
-----

1 similar comment
@mlbridge
Copy link

mlbridge bot commented Jun 23, 2021

Mailing list message from Coleen Phillimore on hotspot-runtime-dev:

On 6/22/21 6:59 PM, David Holmes wrote:

On 23/06/2021 12:39 am, Coleen Phillimore wrote:

On Wed, 16 Jun 2021 16:03:04 GMT, Coleen Phillimore
<coleenp at openjdk.org> wrote:

The handshake code tests if the JavaThread->is_exiting or that the
threadObj() is null. Ever since JDK-8244997, once the JavaThread is
running, the _threadObj won't be null until JavaThread is destroyed.
So testing is_exiting is all you need to do.
In gtest, the test JavaThread doesn't create a _threadObj
JDK-8215948 so removing this unnecessary test allows writing gtests
for handshakes.

Tested with tier1-6.

I am looking at all the threads we create in the vm like the
ServiceThread (look for Threads::add()), but not
attach_current_thread in jni.? You're right, we have to make this
thread safepoint here.
I wonder why some threads we create call ThreadGroup.add and some
don't.? There's lots of duplicate code that's slightly different for
each.

I'm not sure on the "why". Some internal threads (e.g. AttachListener)
don't call the j.l.Thread constructor and so have to directly call the
ThreadGroup.add method. I suspect because these are not true Java
threads that there is some Java-side logic that Thread construction
performs which is not suitable for these internal threads.

Seems that ServiceTherad, MonitorDeflationThread and JFRRecorderThread
do not call ThreadGroup.add, but NotificationThread, SignalThread (for
-xrs), AttachListener, JNI attached threads, don't know JvmtiAgent
threads, and JVM_StartThread (calls JavaThread::prepare).

There's lots of variations with similar code to create threads.

Unfortunately now I need a fix for JDK-8215948, hopefully without
another duplicate copy of this thread creation code so I can write my
handshake test.

Okay guess I'd better look at it again then. :) I can determine which
variant of the j.l.Thread creation logic is suitable and then factor
that out into a support method like
Thread::create_internal_java_thread().

Please!? Refactoring would be very nice!

Thanks,
Coleen

David
-----

@coleenp coleenp restored the threadobj branch June 25, 2021 16:34
@coleenp coleenp reopened this Jun 25, 2021
@coleenp
Copy link
Contributor Author

coleenp commented Jun 25, 2021

Things got conflated in this PR. The test is here:

bool HandshakeState::suspend_with_handshake() {
if (_handshakee->is_exiting() ||
_handshakee->threadObj() == NULL) {
log_trace(thread, suspend)("JavaThread:" INTPTR_FORMAT " exiting", p2i(_handshakee));
return false;
}

We get the JavaThread from threadObj then call java_suspend, so it can't be NULL!

@coleenp coleenp closed this Jun 25, 2021
@coleenp coleenp deleted the threadobj branch June 25, 2021 16:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot-runtime hotspot-runtime-dev@openjdk.org ready Pull request is ready to be integrated rfr Pull request is ready for review
3 participants