RFR 8199868: Support JNI critical functions in object pinning API

[Zhengyu Gu]

Hi Ian,

On 05/03/2018 01:05 PM, Ian Rogers wrote:
> Re JDK-8199868 - should pinning be limited to just critical APIs? The 
> non-critical variants could also pin to avoid copies.
> I see this as related to JDK-8199919. It'd be nice to just move the 
> critical APIs to use the same logic as the non-critical APIs, then 
> pinning just becomes a performance optimization to avoid copying. Re 
> JDK-8199919's comment that there is no deprecation mechanism, compilers 
> like GCC allow function attributes that warn when a deprecated function 
> is used.

This is an interesting idea. I filed JDK-8202604 
(https://bugs.openjdk.java.net/browse/JDK-8202604) to track this.

Thanks,

-Zhengyu

> 
> Thanks,
> Ian
> 
> 
> 
> On Wed, May 2, 2018 at 2:12 PM Per Liden <per.liden at oracle.com 
> <mailto:per.liden at oracle.com>> wrote:
> 
>     Hi,
> 
>     On 05/02/2018 09:41 PM, Zhengyu Gu wrote:
>      > Hi,
>      >
>      > Can I have reviews for this RFR?
>      >
>      > This patch completes object pinning for JNI critical section,
>     provides
>      > critical native support.
>      >
>      > The approach is quite straightforward:
>      >
>      > During generating native wrapper for critical native method, it
>      > generates runtime call to pin every array argument, before
>     unpacks them.
>      >
>      > For pinned objects, it also needs to save them for unpinning
>     after JNI
>      > function call completes.
>      >
>      > If argument is passed on stack, it saves pinned object at the
>     original
>      > slot (as pin_object() may move the object). For register based
>      > arguments, it reuses oop handle area (where GCLocker based
>      > implementation saves register based arguments for safepoints).
>      >
>      > Currently, only Shenandoah uses object pinning for JNI critical
>     section,
>      > this patch has been baked quite some time there. However, I am
>     new to
>      > Runtime Stub code, I would appreciate your comments and suggestions.
>      >
>      > I rebased patch to jdk/jdk repo.
>      >
>      > Webrev: http://cr.openjdk.java.net/~zgu/8199868/webrev.02/
> 
>     Just want to say that I would really like to see this patch go in. As
>     mentioned, it completes the object pinning story and it's useful for
>     other GCs too (at least ZGC and possibly G1). However, I also agree
>     with
>     Aleksey that some one who really knows this code needs to review this.
>     Unfortunately that's not me. Anyone?
> 
>     cheers,
>     Per
> 
>      >
>      > Thanks,
>      >
>      > -Zhengyu
>      >
>      >
>      > On 04/06/2018 10:35 PM, Zhengyu Gu wrote:
>      >> Offline discussion with Aleksey, he suggested that
>      >> pin/unpin_critical_native_array methods can be made more generic as
>      >> pin/unpin_object.
>      >>
>      >> Updated webrev: http://cr.openjdk.java.net/~zgu/8199868/webrev.01/
>      >>
>      >> Test:
>      >>    Reran all tests, submit-hs tests still clean.
>      >>
>      >> Thanks,
>      >>
>      >> -Zhengyu
>      >>
>      >> On 04/06/2018 08:55 AM, Aleksey Shipilev wrote:
>      >>> On 04/04/2018 07:47 PM, Zhengyu Gu wrote:
>      >>>> Please review this patch that adds JNI critical native support to
>      >>>> object pinning.
>      >>>>
>      >>>> Shenandoah does not block GC while JNI critical session is in
>      >>>> progress. This patch allows it to pin
>      >>>> all incoming array objects before critical native call, and unpin
>      >>>> them after call completes.
>      >>>>
>      >>>>
>      >>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8199868
>      >>>> Webrev: http://cr.openjdk.java.net/~zgu/8199868/webrev.00/
>      >>>
>      >>> Looks good to me, but somebody more savvy with runtime stub
>      >>> generation should take a closer look.
>      >>>
>      >>> *) Should probably be "Why we are here?"
>      >>>
>      >>> 2867   assert(Universe::heap()->supports_object_pinning(), "Why we
>      >>> here?");
>      >>>
>      >>> 2876   assert(Universe::heap()->supports_object_pinning(), "Why we
>      >>> here?");
>      >>>
>      >>>
>      >>> Thanks,
>      >>> -Aleksey
>      >>>
>