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

[Doerr, Martin]

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/

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
> >>
> >>
> >>
> >