RFR(XS): 8229236: CriticalJNINatives: dll handling should be done in native thread state

[David Holmes]

On 9/08/2019 7:29 pm, Doerr, Martin wrote:
> Hi Dean and David,
> 
>>> Do you want to skip the dll lookup if
>>> method->is_method_handle_intrinsic() is true?  Otherwise looks good.
> Yes, that avoids the unnecessary lookup.
> 
>> I'm not familiar with this code so did not know what the refactoring
>> would look like, but this seems to do the lookup much earlier and
>> potentially unnecessarily ?
> Earlier: Yes.
> Potentially unnecessarily: Fixed with Dean's proposal.
> 
> In order to reduce the overhead further, I have moved the transition to native forth and back into nativeLookup again (as in webrev.00).
> This avoids unnecessary transitions in many cases.
> 
> New webrev:
> http://cr.openjdk.java.net/~mdoerr/8229236_critNat_resolution/webrev.02/

Okay. I'm not certain if this is really runtime code or compiler code, 
but it seems okay to me, so if Dean is okay with it then that's fine.

Aside: I'm wondering why ARM does not use this critical lookup 
functionality?

Thanks,
David

> Thanks and best regards,
> Martin
> 
> 
>> -----Original Message-----
>> From: David Holmes <david.holmes at oracle.com>
>> Sent: Freitag, 9. August 2019 01:41
>> To: dean.long at oracle.com; Doerr, Martin <martin.doerr at sap.com>;
>> hotspot-runtime-dev at openjdk.java.net
>> Subject: Re: RFR(XS): 8229236: CriticalJNINatives: dll handling should be done
>> in native thread state
>>
>> On 9/08/2019 9:23 am, dean.long at oracle.com wrote:
>>> Do you want to skip the dll lookup if
>>> method->is_method_handle_intrinsic() is true?  Otherwise looks good.
>>
>> I'm not familiar with this code so did not know what the refactoring
>> would look like, but this seems to do the lookup much earlier and
>> potentially unnecessarily ?
>>
>> David
>>
>>> dl
>>>
>>> On 8/8/19 9:05 AM, Doerr, Martin wrote:
>>>> Hi Dean and David,
>>>>
>>>> thanks for your feedback.
>>>>
>>>>> Hi Martin.  Making an exception for this code makes me a little
>>>>> nervous.  It looks like with a little refactoring the problem could go
>>>>> way: move the call to method->critical_native_function() up into
>>>>> AdapterHandlerLibrary::create_native_wrapper() before the lock is
>>>>> entered.
>>>> I had already thought about this. Disadvantage is that several threads
>>>> can perform the same dll lookup concurrently.
>>>> But I guess we can live with that.
>>>>
>>>>> Further, doing I/O while holding a lock seems like a bad idea anyway, so
>>>>> if this call can be done before the lock is acquired as Dean suggested,
>>>>> I think that would be a much better solution.
>>>> I'm not convinced that it's always a bad idea to hold a lock while
>>>> performing I/O.
>>>> At least if the duration is not completely unpredictable and the lock
>>>> is not used for anything critical, it may be ok.
>>>> But I don't like the exception to allow a lock only for this case,
>>>> either.
>>>>
>>>> Here's the new version:
>>>>
>> http://cr.openjdk.java.net/~mdoerr/8229236_critNat_resolution/webrev.01
>> /
>>>>
>>>> Please review.
>>>>
>>>> Best regards,
>>>> Martin
>>>>
>>>>
>>>>
>>>