Re: RFR: 8265753: Remove manual JavaThread transitions to blocked [v4]
On Wed, 19 May 2021 07:26:14 GMT, Robbin Ehn wrote:
>> Please consider this change which removes the manual transitions to blocked.
>> This adds a preprocess template/functor which is executed in the destructor
>> of 'ThreadBlockInVM' if we are going to do any processing.
>> This gives us a way to backout of the object/raw monitor before suspend or
>> other processing, such as a safepoint.
>>
>> The object monitor code could be straight forward changed to use this
>> instead of manual transitions.
>>
>> Raw monitors on the other hand are a bit more complicated due to 'implicit'
>> rules (consequences of the specs).
>> Added a comment in hpp trying to explain it; we cannot simply transition
>> with a raw monitor held.
>> This caused the change in the destructor ~ThreadInVMfromNative() (this
>> specific change have also been tested in unrelated exploration of
>> transition), now this RAII does the same as we do when going to native from
>> Java, just setting the state.
>> Since we are going from an unsafe state, in VM, to a safe state, in native,
>> we do not need to check the poll.
>> That made it possible to careful use ThreadInVMfromNative in raw monitors.
>>
>> I also remove the early CAS in raw_enter.
>> We lock a lock to do a CAS, in the uncontended case means CAS on lock then
>> CAS raw monitor.
>> Now we instead do a transitions, in the uncontended case means fence, CAS
>> raw monitor, fence.
>> (multiple fence (CAS is also a fence) very close to each other have little
>> additional performance impact on contemporary hardware)
>>
>> Passes t1-t7 and manual stressing relevant test groups.
>
> Robbin Ehn has updated the pull request with a new target base due to a merge
> or a rebase. The incremental webrev excludes the unrelated changes brought in
> by the merge/rebase. The pull request contains five additional commits since
> the last revision:
>
> - Review fixes
> - Merge branch 'master' into 8265753
> - Fixes for Dan
> - Merge branch 'master' into 8265753
> - Removed manual transitions
> _Mailing list message from [David Holmes](mailto:david.hol...@oracle.com) on
> [hotspot-runtime-dev](mailto:hotspot-runtime-...@mail.openjdk.java.net):_
>
> On 20/05/2021 5:10 pm, Robbin Ehn wrote:
>
> > On Wed, 19 May 2021 14:25:41 GMT, Richard Reingruber
> > wrote:
> > > Just one more, rather unimportant comment...
> > > Either way: LGTM!
> > > Thanks, Richard.
> >
> >
> > Thanks!
> > > src/hotspot/share/prims/jvmtiRawMonitor.cpp line 382:
> > > > 380:
> > > > 381: _recursions = 0;
> > > > 382: ret = simple_wait(self, millis);
> > >
> > >
> > > IMHO the guarantee at L379 is redundant with the newly added identical
> > > guarantee in `JvmtiRawMonitor::simple_wait()` at L240.
> > > In case you agree to remove the guarantee, I don't see why the following
> > > statements cannot be pulled out of the if-statement.
> > > ```
> > > _recursions = 0;
> > > ret = simple_wait(self, millis);
> > > _recursions = save;
> > > ```
> >
> >
> > Yes, it can be re-written as something below, but is this more readable?
>
> I'd say yes with two minor modifications:
>
> > _recursions = 0;
> > ret = simple_wait(self, millis);
>
> + // Now we need to re-enter the monitor. For JavaThread's
> + // we need to manage suspend requests.
>
> > if (self->is_Java_thread()) { // JavaThread re-enter
> > JavaThread* jt = self->as_Java_thread();
> > {
>
> I think this extra block scope can also go.
>
> Cheers,
> David
> -
Fixed, thanks, Robbin
-
PR: https://git.openjdk.java.net/jdk/pull/3875
Re: RFR: 8265753: Remove manual JavaThread transitions to blocked [v4]
On Thu, 20 May 2021 07:07:20 GMT, Robbin Ehn wrote:
>> src/hotspot/share/prims/jvmtiRawMonitor.cpp line 382:
>>
>>> 380:
>>> 381: _recursions = 0;
>>> 382: ret = simple_wait(self, millis);
>>
>> IMHO the guarantee at L379 is redundant with the newly added identical
>> guarantee in `JvmtiRawMonitor::simple_wait()` at L240.
>>
>> In case you agree to remove the guarantee, I don't see why the following
>> statements cannot be pulled out of the if-statement.
>>
>> _recursions = 0;
>> ret = simple_wait(self, millis);
>> _recursions = save;
>
> Yes, it can be re-written as something below, but is this more readable?
>
>
> _recursions = 0;
>
> ret = simple_wait(self, millis);
>
>
> if (self->is_Java_thread()) { // JavaThread re-enter
>
> JavaThread* jt = self->as_Java_thread();
>
> {
>
> ThreadInVMfromNative tivmfn(jt);
>
> for (;;) {
>
> ExitOnSuspend eos(this);
>
> {
>
> ThreadBlockInVMPreprocess tbivmp(jt, eos);
>
> simple_enter(jt);
>
> }
>
> if (!eos.monitor_exited()) {
>
> break;
>
> }
>
> }
>
> if (jt->is_interrupted(true)) {
>
> ret = M_INTERRUPTED;
>
> }
>
> }
>
> } else { // Non-JavaThread re-enter
>
> assert(ret != M_INTERRUPTED, "Only JavaThreads can be interrupted");
>
> simple_enter(self);
>
> }
> _recursions = save;
Fixed
-
PR: https://git.openjdk.java.net/jdk/pull/3875
Re: RFR: 8265753: Remove manual JavaThread transitions to blocked [v4]
On Thu, 20 May 2021 06:03:28 GMT, David Holmes wrote: >> Robbin Ehn has updated the pull request with a new target base due to a >> merge or a rebase. The incremental webrev excludes the unrelated changes >> brought in by the merge/rebase. The pull request contains five additional >> commits since the last revision: >> >> - Review fixes >> - Merge branch 'master' into 8265753 >> - Fixes for Dan >> - Merge branch 'master' into 8265753 >> - Removed manual transitions > > src/hotspot/share/runtime/objectMonitor.cpp line 448: > >> 446: // Completed the tranisition. >> 447: SafepointMechanism::process_if_requested(current); >> 448: current->set_thread_state(_thread_in_vm); > > The comment block above this code is no longer accurate as there is no longer > an opportunity to go to a safepoint at the end of the block. I'm not sure > what a thread dump would show with the new code. I moved comment and current->set_current_pending_monitor(NULL); into loop. And I set current->set_current_pending_monitor(om); to OM again in ExitOnSuspend if we exit the OM. The thread dump is then identical to what we have today if ~ThreadBlockInVMPreprocess safepoints. - PR: https://git.openjdk.java.net/jdk/pull/3875
Re: RFR: 8265753: Remove manual JavaThread transitions to blocked [v4]
On Thu, 20 May 2021 05:14:43 GMT, David Holmes wrote:
>> Robbin Ehn has updated the pull request with a new target base due to a
>> merge or a rebase. The incremental webrev excludes the unrelated changes
>> brought in by the merge/rebase. The pull request contains five additional
>> commits since the last revision:
>>
>> - Review fixes
>> - Merge branch 'master' into 8265753
>> - Fixes for Dan
>> - Merge branch 'master' into 8265753
>> - Removed manual transitions
>
> src/hotspot/share/runtime/interfaceSupport.inline.hpp line 236:
>
>> 234:
>> 235: template
>> 236: class ThreadBlockInVMPreprocess : public ThreadStateTransition {
>
> Can we add a comment before this template definition please:
>
> // Perform a transition to _thread_blocked and take a call-back to be
> executed before
> // SafepointMechanism::process_if_requested when returning to the VM. This
> allows us
> // to perform an "undo" action if we might block processing a
> safepoint/handshake operation
> // (such as thread suspension).
Fixed
> src/hotspot/share/runtime/interfaceSupport.inline.hpp line 245:
>
>> 243: // Once we are blocked vm expects stack to be walkable
>> 244: thread->frame_anchor()->make_walkable(thread);
>> 245: thread->set_thread_state(_thread_blocked);
>
> This is a pre-existing issue. Everywhere we call make_walkable and then call
> plain set_thread_state (other than on PPC/Aarch64 which do a release_store)
> we are at risk of the thread_state update being reordered with stores related
> to making the stack walkable. Potentially allowing the VMThread (or other
> thread) to walk a stack that is not yet walkable! The original
> ObjectMonitor::enter code was aware of this:
> `current->frame_anchor()->make_walkable(current);`
> `// Thread must be walkable before it is blocked.`
> `// Read in reverse order.`
> `OrderAccess::storestore();`
> `for (;;) {`
> ` current->set_thread_state(_thread_blocked);`
Good catch, fixed.
> src/hotspot/share/runtime/objectMonitor.hpp line 309:
>
>> 307:protected:
>> 308: ObjectMonitor* _om;
>> 309: bool _exited;
>
> For consistency with raw monitor code this would be _om_exited
Fixed
-
PR: https://git.openjdk.java.net/jdk/pull/3875
Re: RFR: 8265753: Remove manual JavaThread transitions to blocked [v4]
On 20/05/2021 5:10 pm, Robbin Ehn wrote:
On Wed, 19 May 2021 14:25:41 GMT, Richard Reingruber wrote:
Just one more, rather unimportant comment...
Either way: LGTM!
Thanks, Richard.
Thanks!
src/hotspot/share/prims/jvmtiRawMonitor.cpp line 382:
380:
381: _recursions = 0;
382: ret = simple_wait(self, millis);
IMHO the guarantee at L379 is redundant with the newly added identical
guarantee in `JvmtiRawMonitor::simple_wait()` at L240.
In case you agree to remove the guarantee, I don't see why the following
statements cannot be pulled out of the if-statement.
_recursions = 0;
ret = simple_wait(self, millis);
_recursions = save;
Yes, it can be re-written as something below, but is this more readable?
I'd say yes with two minor modifications:
_recursions = 0;
ret = simple_wait(self, millis);
+ // Now we need to re-enter the monitor. For JavaThread's
+ // we need to manage suspend requests.
if (self->is_Java_thread()) { // JavaThread re-enter
JavaThread* jt = self->as_Java_thread();
{
I think this extra block scope can also go.
Cheers,
David
-
ThreadInVMfromNative tivmfn(jt);
for (;;) {
ExitOnSuspend eos(this);
{
ThreadBlockInVMPreprocess tbivmp(jt, eos);
simple_enter(jt);
}
if (!eos.monitor_exited()) {
break;
}
}
if (jt->is_interrupted(true)) {
ret = M_INTERRUPTED;
}
}
} else { // Non-JavaThread re-enter
assert(ret != M_INTERRUPTED, "Only JavaThreads can be interrupted");
simple_enter(self);
}
_recursions = save;
-
PR: https://git.openjdk.java.net/jdk/pull/3875
Re: RFR: 8265753: Remove manual JavaThread transitions to blocked [v4]
On Wed, 19 May 2021 14:25:41 GMT, Richard Reingruber wrote:
> Just one more, rather unimportant comment...
>
> Either way: LGTM!
>
> Thanks, Richard.
Thanks!
> src/hotspot/share/prims/jvmtiRawMonitor.cpp line 382:
>
>> 380:
>> 381: _recursions = 0;
>> 382: ret = simple_wait(self, millis);
>
> IMHO the guarantee at L379 is redundant with the newly added identical
> guarantee in `JvmtiRawMonitor::simple_wait()` at L240.
>
> In case you agree to remove the guarantee, I don't see why the following
> statements cannot be pulled out of the if-statement.
>
> _recursions = 0;
> ret = simple_wait(self, millis);
> _recursions = save;
Yes, it can be re-written as something below, but is this more readable?
_recursions = 0;
ret = simple_wait(self, millis);
if (self->is_Java_thread()) { // JavaThread re-enter
JavaThread* jt = self->as_Java_thread();
{
ThreadInVMfromNative tivmfn(jt);
for (;;) {
ExitOnSuspend eos(this);
{
ThreadBlockInVMPreprocess tbivmp(jt, eos);
simple_enter(jt);
}
if (!eos.monitor_exited()) {
break;
}
}
if (jt->is_interrupted(true)) {
ret = M_INTERRUPTED;
}
}
} else { // Non-JavaThread re-enter
assert(ret != M_INTERRUPTED, "Only JavaThreads can be interrupted");
simple_enter(self);
}
_recursions = save;
-
PR: https://git.openjdk.java.net/jdk/pull/3875
Re: RFR: 8265753: Remove manual JavaThread transitions to blocked [v4]
On Wed, 19 May 2021 07:26:14 GMT, Robbin Ehn wrote:
>> Please consider this change which removes the manual transitions to blocked.
>> This adds a preprocess template/functor which is executed in the destructor
>> of 'ThreadBlockInVM' if we are going to do any processing.
>> This gives us a way to backout of the object/raw monitor before suspend or
>> other processing, such as a safepoint.
>>
>> The object monitor code could be straight forward changed to use this
>> instead of manual transitions.
>>
>> Raw monitors on the other hand are a bit more complicated due to 'implicit'
>> rules (consequences of the specs).
>> Added a comment in hpp trying to explain it; we cannot simply transition
>> with a raw monitor held.
>> This caused the change in the destructor ~ThreadInVMfromNative() (this
>> specific change have also been tested in unrelated exploration of
>> transition), now this RAII does the same as we do when going to native from
>> Java, just setting the state.
>> Since we are going from an unsafe state, in VM, to a safe state, in native,
>> we do not need to check the poll.
>> That made it possible to careful use ThreadInVMfromNative in raw monitors.
>>
>> I also remove the early CAS in raw_enter.
>> We lock a lock to do a CAS, in the uncontended case means CAS on lock then
>> CAS raw monitor.
>> Now we instead do a transitions, in the uncontended case means fence, CAS
>> raw monitor, fence.
>> (multiple fence (CAS is also a fence) very close to each other have little
>> additional performance impact on contemporary hardware)
>>
>> Passes t1-t7 and manual stressing relevant test groups.
>
> Robbin Ehn has updated the pull request with a new target base due to a merge
> or a rebase. The incremental webrev excludes the unrelated changes brought in
> by the merge/rebase. The pull request contains five additional commits since
> the last revision:
>
> - Review fixes
> - Merge branch 'master' into 8265753
> - Fixes for Dan
> - Merge branch 'master' into 8265753
> - Removed manual transitions
Hi Robbin,
Overall this looks good to me, but there is one issue that needs fixing
(partially pre-existing but now also affecting ObjectMonitor::enter).
Other minor comments below.
Thanks,
David
src/hotspot/share/runtime/interfaceSupport.inline.hpp line 236:
> 234:
> 235: template
> 236: class ThreadBlockInVMPreprocess : public ThreadStateTransition {
Can we add a comment before this template definition please:
// Perform a transition to _thread_blocked and take a call-back to be executed
before
// SafepointMechanism::process_if_requested when returning to the VM. This
allows us
// to perform an "undo" action if we might block processing a
safepoint/handshake operation
// (such as thread suspension).
src/hotspot/share/runtime/interfaceSupport.inline.hpp line 245:
> 243: // Once we are blocked vm expects stack to be walkable
> 244: thread->frame_anchor()->make_walkable(thread);
> 245: thread->set_thread_state(_thread_blocked);
This is a pre-existing issue. Everywhere we call make_walkable and then call
plain set_thread_state (other than on PPC/Aarch64 which do a release_store) we
are at risk of the thread_state update being reordered with stores related to
making the stack walkable. Potentially allowing the VMThread (or other thread)
to walk a stack that is not yet walkable! The original ObjectMonitor::enter
code was aware of this:
`current->frame_anchor()->make_walkable(current);`
`// Thread must be walkable before it is blocked.`
`// Read in reverse order.`
`OrderAccess::storestore();`
`for (;;) {`
` current->set_thread_state(_thread_blocked);`
src/hotspot/share/runtime/objectMonitor.cpp line 448:
> 446: // Completed the tranisition.
> 447: SafepointMechanism::process_if_requested(current);
> 448: current->set_thread_state(_thread_in_vm);
The comment block above this code is no longer accurate as there is no longer
an opportunity to go to a safepoint at the end of the block. I'm not sure what
a thread dump would show with the new code.
-
Changes requested by dholmes (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/3875
Re: RFR: 8265753: Remove manual JavaThread transitions to blocked [v4]
On Wed, 19 May 2021 07:26:14 GMT, Robbin Ehn wrote: >> Please consider this change which removes the manual transitions to blocked. >> This adds a preprocess template/functor which is executed in the destructor >> of 'ThreadBlockInVM' if we are going to do any processing. >> This gives us a way to backout of the object/raw monitor before suspend or >> other processing, such as a safepoint. >> >> The object monitor code could be straight forward changed to use this >> instead of manual transitions. >> >> Raw monitors on the other hand are a bit more complicated due to 'implicit' >> rules (consequences of the specs). >> Added a comment in hpp trying to explain it; we cannot simply transition >> with a raw monitor held. >> This caused the change in the destructor ~ThreadInVMfromNative() (this >> specific change have also been tested in unrelated exploration of >> transition), now this RAII does the same as we do when going to native from >> Java, just setting the state. >> Since we are going from an unsafe state, in VM, to a safe state, in native, >> we do not need to check the poll. >> That made it possible to careful use ThreadInVMfromNative in raw monitors. >> >> I also remove the early CAS in raw_enter. >> We lock a lock to do a CAS, in the uncontended case means CAS on lock then >> CAS raw monitor. >> Now we instead do a transitions, in the uncontended case means fence, CAS >> raw monitor, fence. >> (multiple fence (CAS is also a fence) very close to each other have little >> additional performance impact on contemporary hardware) >> >> Passes t1-t7 and manual stressing relevant test groups. > > Robbin Ehn has updated the pull request with a new target base due to a merge > or a rebase. The incremental webrev excludes the unrelated changes brought in > by the merge/rebase. The pull request contains five additional commits since > the last revision: > > - Review fixes > - Merge branch 'master' into 8265753 > - Fixes for Dan > - Merge branch 'master' into 8265753 > - Removed manual transitions Updates seem fine. Lucky _waiters was unused :) Will take another full pass over the changes. Thanks, David src/hotspot/share/runtime/objectMonitor.hpp line 309: > 307:protected: > 308: ObjectMonitor* _om; > 309: bool _exited; For consistency with raw monitor code this would _om_exited - PR: https://git.openjdk.java.net/jdk/pull/3875
Re: RFR: 8265753: Remove manual JavaThread transitions to blocked [v4]
On Wed, 19 May 2021 07:26:14 GMT, Robbin Ehn wrote: >> Please consider this change which removes the manual transitions to blocked. >> This adds a preprocess template/functor which is executed in the destructor >> of 'ThreadBlockInVM' if we are going to do any processing. >> This gives us a way to backout of the object/raw monitor before suspend or >> other processing, such as a safepoint. >> >> The object monitor code could be straight forward changed to use this >> instead of manual transitions. >> >> Raw monitors on the other hand are a bit more complicated due to 'implicit' >> rules (consequences of the specs). >> Added a comment in hpp trying to explain it; we cannot simply transition >> with a raw monitor held. >> This caused the change in the destructor ~ThreadInVMfromNative() (this >> specific change have also been tested in unrelated exploration of >> transition), now this RAII does the same as we do when going to native from >> Java, just setting the state. >> Since we are going from an unsafe state, in VM, to a safe state, in native, >> we do not need to check the poll. >> That made it possible to careful use ThreadInVMfromNative in raw monitors. >> >> I also remove the early CAS in raw_enter. >> We lock a lock to do a CAS, in the uncontended case means CAS on lock then >> CAS raw monitor. >> Now we instead do a transitions, in the uncontended case means fence, CAS >> raw monitor, fence. >> (multiple fence (CAS is also a fence) very close to each other have little >> additional performance impact on contemporary hardware) >> >> Passes t1-t7 and manual stressing relevant test groups. > > Robbin Ehn has updated the pull request with a new target base due to a merge > or a rebase. The incremental webrev excludes the unrelated changes brought in > by the merge/rebase. The pull request contains five additional commits since > the last revision: > > - Review fixes > - Merge branch 'master' into 8265753 > - Fixes for Dan > - Merge branch 'master' into 8265753 > - Removed manual transitions Just one more, rather unimportant comment... Either way: LGTM! Thanks, Richard. src/hotspot/share/prims/jvmtiRawMonitor.cpp line 382: > 380: > 381: _recursions = 0; > 382: ret = simple_wait(self, millis); IMHO the guarantee at L379 is redundant with the newly added identical guarantee in `JvmtiRawMonitor::simple_wait()` at L240. In case you agree to remove the guarantee, I don't see why the following statements cannot be pulled out of the if-statement. _recursions = 0; ret = simple_wait(self, millis); _recursions = save; - Marked as reviewed by rrich (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/3875
Re: RFR: 8265753: Remove manual JavaThread transitions to blocked [v4]
> Please consider this change which removes the manual transitions to blocked. > This adds a preprocess template/functor which is executed in the destructor > of 'ThreadBlockInVM' if we are going to do any processing. > This gives us a way to backout of the object/raw monitor before suspend or > other processing, such as a safepoint. > > The object monitor code could be straight forward changed to use this instead > of manual transitions. > > Raw monitors on the other hand are a bit more complicated due to 'implicit' > rules (consequences of the specs). > Added a comment in hpp trying to explain it; we cannot simply transition with > a raw monitor held. > This caused the change in the destructor ~ThreadInVMfromNative() (this > specific change have also been tested in unrelated exploration of > transition), now this RAII does the same as we do when going to native from > Java, just setting the state. > Since we are going from an unsafe state, in VM, to a safe state, in native, > we do not need to check the poll. > That made it possible to careful use ThreadInVMfromNative in raw monitors. > > I also remove the early CAS in raw_enter. > We lock a lock to do a CAS, in the uncontended case means CAS on lock then > CAS raw monitor. > Now we instead do a transitions, in the uncontended case means fence, CAS raw > monitor, fence. > (multiple fence (CAS is also a fence) very close to each other have little > additional performance impact on contemporary hardware) > > Passes t1-t7 and manual stressing relevant test groups. Robbin Ehn has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains five additional commits since the last revision: - Review fixes - Merge branch 'master' into 8265753 - Fixes for Dan - Merge branch 'master' into 8265753 - Removed manual transitions - Changes: - all: https://git.openjdk.java.net/jdk/pull/3875/files - new: https://git.openjdk.java.net/jdk/pull/3875/files/1c6448e3..a8b8469c Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=3875=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3875=02-03 Stats: 23290 lines in 735 files changed: 8980 ins; 9379 del; 4931 mod Patch: https://git.openjdk.java.net/jdk/pull/3875.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3875/head:pull/3875 PR: https://git.openjdk.java.net/jdk/pull/3875
