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

[Doerr, Martin]

Hi Dean,

thanks for the review. Pushed.

Best regards,
Martin


> -----Original Message-----
> From: dean.long at oracle.com <dean.long at oracle.com>
> Sent: Samstag, 10. August 2019 00:02
> To: Doerr, Martin <martin.doerr at sap.com>; David Holmes
> <david.holmes at oracle.com>; hotspot-runtime-dev at openjdk.java.net
> Subject: Re: RFR(XS): 8229236: CriticalJNINatives: dll handling should be done
> in native thread state
> 
> Still good.
> 
> dl
> 
> On 8/9/19 2:29 AM, 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
> /
> >
> > 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
> >>>>
> >>>>
> >>>>