On Wed, 12 May 2021 08:04:47 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 incrementally with one additional
> commit since the last revision:
>
> Fixes for Dan
Hi Robbin,
there seem to be issues in the jvmtiRawMonitor part of the change. Besides that
it looks good.
Cheers, Richard.
src/hotspot/share/prims/jvmtiRawMonitor.cpp line 47:
> 45: JavaThread* current_java_thread = JavaThread::current();
> 46: assert(current_java_thread->thread_state() == _thread_in_vm, "Must be
> in vm");
> 47: {
Looks like the assertion in L46 is redundant now. ThreadToNativeFromVM asserts
as well.
src/hotspot/share/prims/jvmtiRawMonitor.cpp line 386:
> 384: _waiters++;
> 385: ret = simple_wait(self, millis);
> 386: _waiters--;
We don't own the monitor yet so you cannot modify `_waiters` here.
Any reason you moved and duplicated this block?
src/hotspot/share/prims/jvmtiRawMonitor.cpp line 410:
> 408: ret = simple_wait(self, millis);
> 409: _waiters--;
> 410: _recursions = save;
The values of `_waiters` and `_recursions` should be adjusted after the monitor
is owned again.
src/hotspot/share/prims/jvmtiRawMonitor.hpp line 52:
> 50: // native for all operations. However we need to honor a suspend request,
> not
> 51: // entering a monitor if suspended, and check for interrupts. Honoring a
> suspend
> 52: // request// and reading the interrupt flag must be done from VM state
`s/request///request/` 😊
-------------
Changes requested by rrich (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/3875
