RFR: 8274379: Allow process of unsafe access errors in check_special_condition_for_native_trans [v3]

[David Holmes]

On 7/10/2021 5:27 am, Patricio Chilano Mateo wrote:
> On Wed, 6 Oct 2021 00:51:18 GMT, David Holmes <dholmes at openjdk.org> wrote:
> 
>>> Patricio Chilano Mateo has updated the pull request incrementally with one additional commit since the last revision:
>>>
>>>    address David comments
>>
>> src/hotspot/share/runtime/thread.cpp line 1642:
>>
>>> 1640:       // suspend requests and object reallocation operations if any since we might be going to Java after this.
>>> 1641:       SafepointMechanism::process_if_requested_with_exit_check(this, true /* check asyncs */);
>>> 1642:       break;
>>
>> Other than now including the final assertion to be executed, I don't see why we need to change from "return" to "break". That final assertion seems unnecessary as it basically checks that Exceptions::throw... actually threw. I'd be tempted to just delete the final assertion and keep the return statements. But not a major concern either way.
> 
> Fixed.
> 
>> src/hotspot/share/runtime/thread.cpp line 1850:
>>
>>> 1848:   assert(!thread->has_last_Java_frame() || thread->frame_anchor()->walkable(), "Unwalkable stack in native->Java transition");
>>> 1849:
>>> 1850:   thread->set_thread_state(_thread_in_vm);
>>
>> It seems odd to change the thread-state explicitly here when this logic is being processed in the middle of a transition from native->java. So what we end up doing is native->native_trans->in_vm->in_java (and possibly some additional changes from in_vm to blocked and back). I understand why we need to be "in vm" before calling process_if_requested... but it makes the thread state transition story rather messy (just imagine trying to draw a state machine model for this :) ).
> 
> Once we call JavaThread::check_special_condition_for_native_trans() we are calling into the vm so to me it actually makes more sense to set the state to _thread_in_vm there. Specially given that this is not some leaf method, but we actually process safepoint/handshakes/stackwatermarks and all other operations we check for in handle_special_runtime_exit_condition().
> Staying in native_trans also doesn't simplify the state machine in my opinion and actually makes it more complex because inside process_if_requested_with_exit_check() we have to keep track of all these states different than _thread_in_vm: switch case with valid states for safepoint code, transition wrapper for state bookkeeping in handshake code (so we still have the native_trans->vm change), and having to manually switch to _thread_blocked and back because we cannot use ThreadBlockInVM in wait_for_object_deoptimization().
> 
> Note: As a future change I think we could just avoid the native_trans state altogether and just poll in a state of _thread_in_vm, since the only purpose of it is to poll in an unsafe state. Or we could even set the state to _thread_in_Java already, so that when there is nothing to process there are no extra changes of state needed, and when there are pending operations we can use a ThreadInVMfromJava wrapper here.

I like the sound of that future change. :)

Thanks,
David

> -------------
> 
> PR: https://git.openjdk.java.net/jdk/pull/5741
>