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