On Wed, 19 May 2021 07:26:14 GMT, Robbin Ehn <[email protected]> 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
