RFR (S) 8210842: Handle JNIGlobalRefLocker.cpp

JC Beyler jcbeyler at google.com
Thu Sep 20 15:56:41 UTC 2018


Hi Alex,

Done here, thanks for the review:

Webrev: http://cr.openjdk.java.net/~jcbeyler/8210842/webrev.03/
Bug: https://bugs.openjdk.java.net/browse/JDK-8210842

Thanks again!
Jc


On Wed, Sep 19, 2018 at 5:13 PM Alex Menkov <alexey.menkov at oracle.com>
wrote:

> Hi Jc,
>
> Looks good to me.
> A minor note:
> - I'd move ErrorHandler typedef to SafeJNIEnv class to avoid global
> namespece pollution (ErrorHandler is too generic name).
>
> --alex
>
> On 09/19/2018 15:56, JC Beyler wrote:
> > Hi Alex,
> >
> > I've updated the webrev to:
> > Webrev: http://cr.openjdk.java.net/~jcbeyler/8210842/webrev.02/
> > <http://cr.openjdk.java.net/%7Ejcbeyler/8210842/webrev.02/>
> > Bug: https://bugs.openjdk.java.net/browse/JDK-8210842
> >
> > That webrev has the code that is shown here in snippets.
> >
> >
> > Thanks for the reviews, I've relatively followed your reviews except for
> > one detail due to me wanting to handle the NSK_JNI_VERIFY macros via
> > this system as well later down the road. For an example:
> >
> > We currently have in the code:
> > if ( ! NSK_JNI_VERIFY(pEnv, (mhClass = NSK_CPP_STUB2(GetObjectClass,
> > pEnv, mhToCall)) != NULL) )
> >
> > 1) The NSK_CPP_STUB2 is trivially removed with a refactor (JDK-8210728)
> > <https://bugs.openjdk.java.net/browse/JDK-8210728> to:
> > if ( ! NSK_JNI_VERIFY(pEnv, (mhClass = pEnv->GetObjectClass(pEnv,
> > mhToCall)) != NULL) )
> >
> > 2) Then the NSK_JNI_VERIFY, I'd like to remove it to and it becomes via
> > this wrapping of JNIEnv:
> > if ( ! (mhClass = pEnv->GetObjectClass(pEnv, mhToCall)) )
> >
> > 3) Then, via removing the assignment, we'd arrive to a:
> > mhClass = pEnv->GetObjectClass(pEnv, mhToCall));
> > if (!mhClass)
> >
> > Without any loss of checking for failures, etc.
> >
> > So that is my motivation for most of this work with a concrete example
> > (hopefully it helps drive this conversation).
> >
> > I inlined my answers here, let me know what you think.
> >
> > On Wed, Sep 19, 2018 at 2:30 PM Alex Menkov <alexey.menkov at oracle.com
> > <mailto:alexey.menkov at oracle.com>> wrote:
> >
> >     Hi Jc,
> >
> >     Updated tests looks good.
> >     Some notes about implementation.
> >
> >     - FatalOnException implements both verification and error handling
> >     It would be better to separate them (and this makes easy to implement
> >     error handling different from JNIEnv::FatalError).
> >     The simplest way is to define error handler as
> >     class SafeJNIEnv {
> >     public:
> >           typedef void (*ErrorHandler)(JNIEnv *env, const char*
> errorMsg);
> >           // error handler which terminates jvm by using FatalError()
> >           static void FatalError(JNIEnv *env, const char *errrorMsg);
> >
> >           SafeJNIEnv(JNIEnv* jni_env, ErrorHandler errorHandler =
> >     FatalError);
> >     (SafeJNIEnv methods should create FatalOnException objects passing
> >     errorHandler)
> >
> >
> >
> > Agreed, I tried to keep the code simple. The concepts you talk about
> > here are generally what I reserve for when I need it (ie extensions and
> > handling new cases). But a lot are going to be needed soon so I think it
> > is a good time to iron the code out now on this "simple" webrev.
> >
> > So done for this.
> >
> >
> >
> >     - FatalOnException is used in SafeJNIEnv methods as
> >         FatalOnException marker(this, "msg");
> >         ret = <JNI call>
> >         (optional)marker.check_for_null(ret);
> >         return ret;
> >     But actually I'd call it something like JNICallResultVerifier and
> >     create
> >     the object after JNI call. like
> >         ret = <JNI call>
> >         JNICallResultVerifier(this, "msg")
> >           (optional).notNull(ret);
> >         return ret;
> >     or even (if you like such syntax sugar) you can define
> >         template<typename T>
> >         T resultNotNull(T result) {
> >             notNull(result);
> >             return result;
> >         }
> >     and do
> >         ret = <JNI call>
> >         return JNICallResultVerifier(this, "msg").resultNotNull(ret);
> >
> >
> > So I renamed FatalOnException to now being JNIVerifier.
> >
> > Though I like it, I don't think we can do it, except if we do it
> > slightly differently:
> > I'm trying to solve two problems with one stone:
> >     - How to check for returns of JNI calls in the way that is done here
> >     - How to remove NSK_JNI_VERIFY* (from nsk/share/jni/jni_tools)
> >
> > However, the NSK_JNI_VERIFY need to start a tracing system before the
> > call to JNI, so it won't work this way. (Side question would be do we
> > still care about the tracing in NSK_JNI_VERIFY, if we don't then your
> > solution works well in most situations).
> >
> > My vision or intuition is that we would throw a template at some point
> > on SafeJNIEnv to handle both cases and have JNIVerifier become a
> > specialization in certain cases and something different for the
> > NSK_JNI_VERIFY case (or have a different constructor to enable tracing).
> > But for now, I offer the implementation that does:
> >
> > jclass SafeJNIEnv::GetObjectClass(jobject obj) {
> >    JNIVerifier<jclass> marker(this, "GetObjectClass");
> >    return marker.ResultNotNull(_jni_env->GetObjectClass(obj));
> > }
> >
> > and:
> >
> > void SafeJNIEnv::SetObjectField(jobject obj, jfieldID field, jobject
> > value) {
> >    JNIVerifier<> marker(this, "SetObjectField");
> >    _jni_env->SetObjectField(obj, field, value);
> > }
> >
> >
> >
> >
> >     - you added #include <memory> in the test (and you have to add it to
> >     every test).
> >     It would be simpler to add the include to SafeJNIEnv.hpp and define
> >     typedef std::unique_ptr<SafeJNIEnv> SafeJNIEnvPtr;
> >     Then each in the test methods:
> >         SafeJNIEnvPtr env(new SafeJNIEnv(jni_env));
> >     or you can add
> >     static SafeJNIEnv::SafeJNIEnvPtr wrap(JNIEnv* jni_env, ErrorHandler
> >     errorHandler = fatalError)
> >     and get
> >         SafeJNIEnvPtr env = SafeJNIEnv::wrap(jni_env);
> >
> >
> > Done, I like that, even less code change to tests then.
> >
> >
> >
> >     - it would be better to wrap internal classes (FatalOnException) into
> >     unnamed namespace - this helps to avoid conflicts with other classes)
> >
> >     - FatalOnException::check_for_null(void* ptr)
> >     should be
> >     FatalOnException::check_for_null(const void* ptr)
> >     And calling the method you don't need reinterpret_cast
> >
> >
> > Also done, it got renamed to ResultNotNull, is using a template now, but
> > agreed, that worked.
> > Thanks again for the review,
> > Jc
> >
> >
> >     --alex
> >
> >
> >     On 09/18/2018 11:07, JC Beyler wrote:
> >      > Hi David,
> >      >
> >      > Thanks for the quick review and thoughts. I have now a new
> >     version here
> >      > that addresses your comments:
> >      >
> >      > Webrev: http://cr.openjdk.java.net/~jcbeyler/8210842/webrev.01/
> >     <http://cr.openjdk.java.net/%7Ejcbeyler/8210842/webrev.01/>
> >      > <http://cr.openjdk.java.net/%7Ejcbeyler/8210842/webrev.01/>
> >      > Bug:https://bugs.openjdk.java.net/browse/JDK-8210842
> >      >
> >      > I've also inlined my answers/comments.
> >      >
> >      >
> >      >
> >      >      > I've slowly started working on JDK-8191519
> >      >      > <https://bugs.openjdk.java.net/browse/JDK-8191519>.
> >     However before
> >      >      > starting to really refactor all the tests, I thought I'd
> >     get a few
> >      >      > opinions. I am working on internalizing the error handling
> >     of JNI
> >      >     calls
> >      >      > via a SafeJNIEnv class that redefines all the JNI calls to
> >     add an
> >      >     error
> >      >      > checker.
> >      >      >
> >      >      > The advantage is that the test code will look and feel like
> >      >     normal JNI
> >      >      > code and calls but will have the checks we want to have
> >     for our
> >      >     tests.
> >      >
> >      >     Not sure I get that. Normal JNI code has to check for
> >     errors/exceptions
> >      >     after every call. The tests need those checks too. Today they
> are
> >      >     explicit, with this change they become implicit. Not sure
> >     what we are
> >      >     gaining here ??
> >      >
> >      >
> >      > In my humble opinion, having the error checking out of the way
> >     allows
> >      > the code reader to concentrate on the JNI code while maintaining
> >     error
> >      > checking. We use something similar internally, so perhaps I'm
> >     biased to
> >      > it :-).
> >      > If this is not a desired/helpful "feature" to simplify tests in
> >     general,
> >      > I will backtrack it and just add the explicit tests to the native
> >     code
> >      > of the locks for the fix
> >      > https://bugs.openjdk.java.net/browse/JDK-8191519 instead.
> >      >
> >      > Let me however try to make my case and let me know what you all
> >     think!
> >      >
> >      >
> >      >      > If agreed with this, we can augment the SafeJNIEnv class
> >     as needed.
> >      >      > Also, if the tests require something else than fatal
> >     errors, we
> >      >     can add
> >      >      > a different marker and make it a parameter to the base
> class.
> >      >      >
> >      >      > Webrev:
> >     http://cr.openjdk.java.net/~jcbeyler/8210842/webrev.00/
> >     <http://cr.openjdk.java.net/%7Ejcbeyler/8210842/webrev.00/>
> >      >     <http://cr.openjdk.java.net/%7Ejcbeyler/8210842/webrev.00/>
> >      >      > <http://cr.openjdk.java.net/%7Ejcbeyler/8210842/webrev.00/
> >
> >      >      > Bug: https://bugs.openjdk.java.net/browse/JDK-8210842
> >      >      >
> >      >      > Let me know what you think,
> >      >
> >      >     Two initial suggestions:
> >      >
> >      >     1. FatalOnException should be constructed with a description
> >     string so
> >      >     that it can report the failing operation when calling
> >     FatalError rather
> >      >     than the general "Problem with a JNI call".
> >      >
> >      >
> >      > Agreed with you, the new webrev produces:
> >      >
> >      > FATAL ERROR in native method: GetObjectClass
> >      >          at
> >
>  nsk.share.gc.lock.CriticalSectionTimedLocker.criticalSection(CriticalSectionTimedLocker.java:47)
> >      >          at
> >     nsk.share.gc.lock.jniref.JNIGlobalRefLocker.criticalNative(Native
> >     Method)
> >      >          at
> >
>  nsk.share.gc.lock.jniref.JNIGlobalRefLocker.criticalSection(JNIGlobalRefLocker.java:44)
> >      >          at
> >
>  nsk.share.gc.lock.CriticalSectionLocker$1.run(CriticalSectionLocker.java:56)
> >      >          at
> >     java.lang.Thread.run(java.base at 12-internal/Thread.java:834)
> >      >
> >      >
> >      > and for a return NULL in NewGlobalRef, we would get automatically:
> >      >
> >      > FATAL ERROR in native method: NewGlobalRef:Return is NULL
> >      >          at
> >     nsk.share.gc.lock.jniref.JNIGlobalRefLocker.criticalNative(Native
> >     Method)
> >      >
> >      >          at
> >      >
> >
>  nsk.share.gc.lock.jniref.JNIGlobalRefLocker.criticalSection(JNIGlobalRefLocker.java:44)
> >
> >      >
> >      >
> >      >
> >      > Again as we port and simplify more tests (I'll only do the locks
> >     for now
> >      > and we can figure out the next steps as start working on moving
> >     tests
> >      > out of vmTestbase),
> >      > we can add, if needed, other exception handlers that don't throw
> >     or do
> >      > other things depending on the JNI method outputs.
> >      >
> >      >
> >      >     2. Make the local SafeJNIEnv a pointer called env so that the
> >     change is
> >      >     less disruptive. All the env->op() calls will remain and only
> >     the local
> >      >     error checking will be removed.
> >      >
> >      >
> >      > Done, I used a unique_ptr to make the object
> >     created/destroyed/available
> >      > as a pointer do-able in one line:
> >      > std::unique_ptr<SafeJNIEnv> env(new SafeJNIEnv(jni_env));
> >      >
> >      > and then you can see that, as you mentioned, the disruption to
> >     the code
> >      > is much less:
> >      >
> >
> http://cr.openjdk.java.net/~jcbeyler/8210842/webrev.01/test/hotspot/jtreg/vmTestbase/nsk/share/gc/lock/jniref/JNIGlobalRefLocker.cpp.udiff.html
> >     <
> http://cr.openjdk.java.net/%7Ejcbeyler/8210842/webrev.01/test/hotspot/jtreg/vmTestbase/nsk/share/gc/lock/jniref/JNIGlobalRefLocker.cpp.udiff.html
> >
> >
> >      >
> >     <
> http://cr.openjdk.java.net/%7Ejcbeyler/8210842/webrev.01/test/hotspot/jtreg/vmTestbase/nsk/share/gc/lock/jniref/JNIGlobalRefLocker.cpp.udiff.html
> >
> >      >
> >      > Basically the tests now become internal to the SafeJNIEnv and the
> >     code
> >      > now only contains the JNI calls happening but we are protected
> >     and will
> >      > fail any test that has an exception or a NULL return for the
> >     call. Of
> >      > course, this is not 100% proof in terms of not having any error
> >     handling
> >      > in the test but in some cases like this, the new code seems to
> >     just work
> >      > better:
> >      >
> >      >
> >
> http://cr.openjdk.java.net/~jcbeyler/8210842/webrev.01/test/hotspot/jtreg/vmTestbase/nsk/share/gc/lock/jniref/JNIGlobalRefLocker.cpp.html
> >     <
> http://cr.openjdk.java.net/%7Ejcbeyler/8210842/webrev.01/test/hotspot/jtreg/vmTestbase/nsk/share/gc/lock/jniref/JNIGlobalRefLocker.cpp.html
> >
> >
> >      >
> >     <
> http://cr.openjdk.java.net/%7Ejcbeyler/8210842/webrev.01/test/hotspot/jtreg/vmTestbase/nsk/share/gc/lock/jniref/JNIGlobalRefLocker.cpp.html
> >
> >      >
> >      >
> >      >
> >      >     The switch from, e.g., checking NULL returns to checking for
> >     pending
> >      >     exceptions, need to be sure that the JNI operations clearly
> >     specify
> >      >     that
> >      >     NULL implies there will be an exception pending. This change
> >     may be an
> >      >     issue for static code analysis if not smart enough to
> >     understand the
> >      >     RAII wrappers.
> >      >
> >      >
> >      > Agreed, I fixed it to be more strict and say: in normal test
> >     handling,
> >      > the JNI calls should never return NULL or throw an exception. This
> >      > should hold for tests I imagine but if not we can add a different
> >     call
> >      > verifier as we go.
> >      >
> >      >
> >      >     Thanks,
> >      >     David
> >      >
> >      >      > Jc
> >      >
> >      >
> >      >
> >      > Let me know what you all think. When talking about it a bit, I had
> >      > gotten some interest to see what it would look like. Here it is
> >     :-). Now
> >      > if it is not wanted like I said, I can backtrack and just put the
> >     error
> >      > checks after each JNI call for all the tests as we are porting
> them.
> >      > Jc
> >
> >
> >
> > --
> >
> > Thanks,
> > Jc
>


-- 

Thanks,
Jc
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20180920/c34fe550/attachment-0001.html>


More information about the serviceability-dev mailing list