RFR (L) 8213501 : Deploy ExceptionJniWrapper for a few tests
Chris Plummer
chris.plummer at oracle.com
Tue Dec 4 04:15:05 UTC 2018
Hi JC,
Overall it looks good. A few naming nits thought:
In bi01t001.cpp, why have you declared the ExceptionCheckingJniEnvPtr
using jni_env(jni). Elsewhere you use jni(jni_env) and rename the method
argument passed in from jni to jni_env.
Related to this, I also noticed in some files that already are using
ExceptionCheckingJniEnvPtr, such as CharArrayCriticalLocker.cpp, you
delcared it as env(jni_env). So that means there are 3 different names
you have used for the ExceptionCheckingJniEnvPtr local variable. They
should be consistent.
Also, can you rename get_basename() to get_dirname()? I know Serguei
suggested get_basename() a while back, but unless "basename" is commonly
used for this purpose, I think "dirname" is more self explanatory.
thanks,
Chris
On 12/2/18 10:29 PM, David Holmes wrote:
> Hi Jc,
>
> I've been lurking on this one and have had a look through. I'm okay
> with the FatalError approach for the tests - we don't expect anything
> to go wrong in a well written test in a correctly functioning VM.
>
> Thanks,
> David
>
>
>
> On 3/12/2018 3:24 pm, JC Beyler wrote:
>> Hi all,
>>
>> Would someone on the GC or runtime team be motivated to give this a
>> review? :)
>>
>> It would be much appreciated!
>>
>> Webrev: http://cr.openjdk.java.net/~jcbeyler/8213501/webrev.02/
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8213501
>>
>> Thanks for your help,
>> Jc
>>
>> On Tue, Nov 27, 2018 at 4:36 PM JC Beyler <jcbeyler at google.com
>> <mailto:jcbeyler at google.com>> wrote:
>>
>> Hi Chris,
>>
>> Yes I was waiting for another review since you had explicitly
>> asked :)
>>
>> And sounds good that when someone from GC or runtime gives a review,
>> I'll wait for your full review on the webrev.02!
>>
>> Thanks again for your help,
>> Jc
>>
>>
>> On Tue, Nov 27, 2018 at 12:48 PM Chris Plummer
>> <chris.plummer at oracle.com <mailto:chris.plummer at oracle.com>> wrote:
>>
>> Hi JC,
>>
>> I think it would be good to get a review from the gc or runtime
>> teams, since this also affects their tests.
>>
>> Also, once we are settled on this FatalError approach, I still
>> need to give your webrev-02 a full review. I only skimmed over
>> parts of it (I did look at all the changes in webrevo-01).
>>
>> thanks,
>>
>> Chris
>>
>> On 11/27/18 8:58 AM, serguei.spitsyn at oracle.com
>> <mailto:serguei.spitsyn at oracle.com> wrote:
>>> Hi Jc,
>>>
>>> I've already reviewed this too.
>>>
>>> Thanks,
>>> Serguei
>>>
>>>
>>> On 11/27/18 06:56, JC Beyler wrote:
>>>> Thanks Chris,
>>>>
>>>> Anybody else motivated to look at this and review it? :)
>>>> Jc
>>>>
>>>> On Mon, Nov 26, 2018 at 1:26 PM Chris Plummer
>>>> <chris.plummer at oracle.com <mailto:chris.plummer at oracle.com>>
>>>> wrote:
>>>>
>>>> Hi JC,
>>>>
>>>> I'm ok with the FatalError approach, but would like to
>>>> hear opinions from others also.
>>>>
>>>> thanks,
>>>>
>>>> Chris
>>>>
>>>> On 11/21/18 8:19 AM, JC Beyler wrote:
>>>>> Hi Chris,
>>>>>
>>>>> Thanks for taking the time to look at it and yes you
>>>>> have raised exactly why the webrev is between two
>>>>> worlds: in cases where a fatal error on failure is
>>>>> wanted, should we simplify the code to remove the return
>>>>> tests since we do them internally? Now that I've looked
>>>>> around for non-fatal cases, I think the answer is yes,
>>>>> it simplifies the code while maintaining the checks.
>>>>>
>>>>> I looked a bit and it seems that I can't find easily a
>>>>> case where the test accepts a JNI failure to then move
>>>>> on. Therefore, perhaps, for now, the fail with a Fatal
>>>>> is enough and we can work on the tests to clean them up?
>>>>>
>>>>> That means that this is the new webrev with only Fatal
>>>>> and cleans up the tests so that it is no longer in
>>>>> between two worlds:
>>>>>
>>>>> Webrev:
>>>>> http://cr.openjdk.java.net/~jcbeyler/8213501/webrev.02/
>>>>> <http://cr.openjdk.java.net/%7Ejcbeyler/8213501/webrev.02/>
>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8213501
>>>>>
>>>>> (This passes testing on my dev machine for all the
>>>>> modified tests)
>>>>>
>>>>> with the example you provided, it now looks like:
>>>>> http://cr.openjdk.java.net/~jcbeyler/8213501/webrev.02/test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/allocation/AP04/ap04t003/ap04t003.cpp.frames.html
>>>>> <http://cr.openjdk.java.net/%7Ejcbeyler/8213501/webrev.02/test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/allocation/AP04/ap04t003/ap04t003.cpp.frames.html>
>>>>>
>>>>> Where it does, to me at least, seem cleaner and less
>>>>> "noisy".
>>>>>
>>>>> Let me know what you think,
>>>>> Jc
>>>>>
>>>>>
>>>>> On Tue, Nov 20, 2018 at 9:33 PM Chris Plummer
>>>>> <chris.plummer at oracle.com
>>>>> <mailto:chris.plummer at oracle.com>> wrote:
>>>>>
>>>>> Hi JC,
>>>>>
>>>>> Sorry about the delay. I had to go back an look at
>>>>> the initial 8210842 webrev and RFR thread to see
>>>>> what this was initially all about.
>>>>>
>>>>> In general the changes look good.
>>>>>
>>>>> I don't have a good answer to your
>>>>> FatalError/NonFatalError question. It makes the code
>>>>> a lot cleaner to use FatalError, but then it is a
>>>>> behavior change, and you also need to deal with
>>>>> tests that intentionally induce errors (do you have
>>>>> an example of that).
>>>>>
>>>>> In any case, right now your webrev seems to be
>>>>> between two worlds. You are producing FatalError,
>>>>> but still checking results. Here's a good example:
>>>>>
>>>>> http://cr.openjdk.java.net/~jcbeyler/8213501/webrev.01/test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/allocation/AP04/ap04t003/ap04t003.cpp.frames.html
>>>>> <http://cr.openjdk.java.net/%7Ejcbeyler/8213501/webrev.01/test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/allocation/AP04/ap04t003/ap04t003.cpp.frames.html>
>>>>>
>>>>> I'm not sure if this is just a temporary state until
>>>>> it was decided which approach to take.
>>>>>
>>>>> thanks,
>>>>>
>>>>> Chris
>>>>>
>>>>>
>>>>> On 11/20/18 2:14 PM, JC Beyler wrote:
>>>>>> Hi all,
>>>>>>
>>>>>> Chris thought it made sense to have more eyes on
>>>>>> this change than just serviceability as it will
>>>>>> modify to tests that are not only serviceability
>>>>>> tests so I've moved this to conversation here :)
>>>>>>
>>>>>> For convenience, I've copy-pasted the initial RFR:
>>>>>>
>>>>>> Could I have a review for the extension and usage
>>>>>> of the ExceptionJniWrapper. This adds lines and
>>>>>> filenames to the end of the wrapper JNI methods,
>>>>>> adds tracing, and throws an error if need be. I've
>>>>>> ported the gc/lock files to use the new
>>>>>> TRACE_JNI_CALL add-on and I've ported a few of the
>>>>>> tests that were already changed for the assignment
>>>>>> webrev for JDK-8212884.
>>>>>>
>>>>>> Webrev:
>>>>>> http://cr.openjdk.java.net/~jcbeyler/8213501/webrev.01
>>>>>> <http://cr.openjdk.java.net/%7Ejcbeyler/8213501/webrev.01>
>>>>>> Bug:
>>>>>> https://bugs.openjdk.java.net/browse/JDK-8213501
>>>>>>
>>>>>> For illustration, if I force an error to the
>>>>>> AP04/ap04t03 test and set the verbosity on, I get
>>>>>> something like:
>>>>>>
>>>>>> >> Calling JNI method FindClass from
>>>>>> ap04t003.cpp:343
>>>>>> >> Calling with these parameter(s):
>>>>>> java/lang/Threadd
>>>>>> Wait for thread to finish
>>>>>> << Called JNI method FindClass from ap04t003.cpp:343
>>>>>> Exception in thread "Thread-0"
>>>>>> java.lang.NoClassDefFoundError: java/lang/Threadd
>>>>>> at
>>>>>> nsk.jvmti.scenarios.allocation.AP04.ap04t003.runIterateOverHeap(Native
>>>>>>
>>>>>> Method)
>>>>>> at
>>>>>> nsk.jvmti.scenarios.allocation.AP04.ap04t003HeapIterator.runIteration(ap04t003.java:140)
>>>>>> at
>>>>>> nsk.jvmti.scenarios.allocation.AP04.ap04t003Thread.run(ap04t003.java:201)
>>>>>> Caused by: java.lang.ClassNotFoundException:
>>>>>> java.lang.Threadd
>>>>>> at
>>>>>> java.base/jdk.internal.loader.BuiltinClassLoader.loadClass(BuiltinClassLoader.java:583)
>>>>>> at
>>>>>> java.base/jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(ClassLoaders.java:178)
>>>>>> at
>>>>>> java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:521)
>>>>>> ... 3 more
>>>>>> FATAL ERROR in native method: JNI method FindClass
>>>>>> : internal error from ap04t003.cpp:343
>>>>>> at
>>>>>> nsk.jvmti.scenarios.allocation.AP04.ap04t003.runIterateOverHeap(Native
>>>>>>
>>>>>> Method)
>>>>>> at
>>>>>> nsk.jvmti.scenarios.allocation.AP04.ap04t003HeapIterator.runIteration(ap04t003.java:140)
>>>>>> at
>>>>>> nsk.jvmti.scenarios.allocation.AP04.ap04t003Thread.run(ap04t003.java:201)
>>>>>>
>>>>>> Questions/comments I have about this are:
>>>>>> - Do we want to force fatal errors when a JNI
>>>>>> call fails in general? Most of these tests do the
>>>>>> right thing and test the return of the JNI calls,
>>>>>> for example:
>>>>>> thrClass = jni->FindClass("java/lang/Threadd",
>>>>>> TRACE_JNI_CALL);
>>>>>> if (thrClass == NULL) {
>>>>>>
>>>>>> but now the wrapper actually would do a fatal if
>>>>>> the FindClass call would return a nullptr, so we
>>>>>> could remove that test altogether. What do you
>>>>>> think?
>>>>>> - I prefer to leave them as the tests then
>>>>>> become closer to what real users would have in
>>>>>> their code and is the "recommended" way of doing it
>>>>>>
>>>>>> - The alternative is to use the NonFatalError I
>>>>>> added which then just prints out that something
>>>>>> went wrong, letting the test continue. Question
>>>>>> will be what should be the default? The fatal or
>>>>>> the non-fatal error handling?
>>>>>>
>>>>>> On a different subject:
>>>>>> - On the new tests, I've removed the
>>>>>> NSK_JNI_VERIFY since the JNI wrapper handles the
>>>>>> tracing and the verify in almost the same way; only
>>>>>> difference I can really tell is that the complain
>>>>>> method from NSK has a max complain before stopping
>>>>>> to "complain"; I have not added that part of the
>>>>>> code in this webrev
>>>>>>
>>>>>> Once we decide on these, I can continue on the
>>>>>> files from JDK-8212884 and then do both the
>>>>>> assignment in an if extraction followed-by this
>>>>>> type of webrev in an easier fashion. Depending on
>>>>>> decisions here, NSK*VERIFY can be deprecated as
>>>>>> well as we go forward.
>>>>>>
>>>>>> Thanks!
>>>>>> Jc
>>>>>>
>>>>>> On Mon, Nov 19, 2018 at 11:34 AM Chris Plummer
>>>>>> <chris.plummer at oracle.com
>>>>>> <mailto:chris.plummer at oracle.com>> wrote:
>>>>>>
>>>>>> On 11/19/18 10:07 AM, JC Beyler wrote:
>>>>>>> Hi all,
>>>>>>>
>>>>>>> @David/Chris: should I then push this RFR to
>>>>>>> the hotspot mailing or the runtime one? For
>>>>>>> what it's worth, a lot of the tests under the
>>>>>>> vmTestbase are jvmti so the review also
>>>>>>> affects serviceability; it just turns out I
>>>>>>> started with the GC originally and then hit
>>>>>>> some other tests I had touched via the
>>>>>>> assignment extraction.
>>>>>> I think hotspot would be best.
>>>>>>
>>>>>> Chris
>>>>>>>
>>>>>>> @Serguei: Done for the method renaming, for
>>>>>>> the indent, are you talking about going from
>>>>>>> the 8-indent to 4-indent? If so, would it not
>>>>>>> just be better to do a new JBS bug and do the
>>>>>>> whole files in one go? I ask because
>>>>>>> otherwise, it will look a bit weird to have
>>>>>>> parts of the file as 8-indent and others
>>>>>>> 4-indent?
>>>>>>>
>>>>>>> Thanks for looking at it!
>>>>>>> Jc
>>>>>>>
>>>>>>> On Mon, Nov 19, 2018 at 1:25 AM
>>>>>>> serguei.spitsyn at oracle.com
>>>>>>> <mailto:serguei.spitsyn at oracle.com>
>>>>>>> <serguei.spitsyn at oracle.com
>>>>>>> <mailto:serguei.spitsyn at oracle.com>> wrote:
>>>>>>>
>>>>>>> Hi Jc,
>>>>>>>
>>>>>>> We have to start this review anyway. :)
>>>>>>> It looks good to me in general.
>>>>>>> Thank you for your consistency in this
>>>>>>> refactoring!
>>>>>>>
>>>>>>> Some minor comments.
>>>>>>>
>>>>>>> http://cr.openjdk.java.net/%7Ejcbeyler/8213501/webrev.00/test/hotspot/jtreg/vmTestbase/nsk/share/jni/ExceptionCheckingJniEnv.cpp.udiff.html
>>>>>>> +static const char* remove_folders(const
>>>>>>> char* fullname) { I'd suggest to rename
>>>>>>> the function name to something traditional
>>>>>>> like get_basename. Otherwise, it sounds
>>>>>>> like this function has to really remove
>>>>>>> folders. :) Also, all *Locker.cpp have
>>>>>>> wrong indent in the bodies of if and while
>>>>>>> statements. Could this be fixed with the
>>>>>>> refactoring? I did not look on how this
>>>>>>> impacts the tests other than
>>>>>>> serviceability. Thanks, Serguei
>>>>>>>
>>>>>>>
>>>>>>> On 11/16/18 19:43, JC Beyler wrote:
>>>>>>>> Hi all,
>>>>>>>>
>>>>>>>> Anybody motivated to review this? :)
>>>>>>>> Jc
>>>>>>>>
>>>>>>>> On Wed, Nov 7, 2018 at 9:53 PM JC Beyler
>>>>>>>> <jcbeyler at google.com
>>>>>>>> <mailto:jcbeyler at google.com>> wrote:
>>>>>>>>
>>>>>>>> Hi all,
>>>>>>>>
>>>>>>>> Could I have a review for the
>>>>>>>> extension and usage of the
>>>>>>>> ExceptionJniWrapper. This adds lines
>>>>>>>> and filenames to the end of the
>>>>>>>> wrapper JNI methods, adds tracing,
>>>>>>>> and throws an error if need be. I've
>>>>>>>> ported the gc/lock files to use the
>>>>>>>> new TRACE_JNI_CALL add-on and I've
>>>>>>>> ported a few of the tests that were
>>>>>>>> already changed for the assignment
>>>>>>>> webrev for JDK-8212884.
>>>>>>>>
>>>>>>>> Webrev:
>>>>>>>> http://cr.openjdk.java.net/~jcbeyler/8213501/webrev.00/
>>>>>>>> <http://cr.openjdk.java.net/%7Ejcbeyler/8213501/webrev.00/>
>>>>>>>> Bug:
>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8213501
>>>>>>>>
>>>>>>>> For illustration, if I force an error
>>>>>>>> to the AP04/ap04t03 test and set the
>>>>>>>> verbosity on, I get something like:
>>>>>>>>
>>>>>>>> >> Calling JNI method FindClass from
>>>>>>>> ap04t003.cpp:343
>>>>>>>> >> Calling with these parameter(s):
>>>>>>>> java/lang/Threadd
>>>>>>>> Wait for thread to finish
>>>>>>>> << Called JNI method FindClass from
>>>>>>>> ap04t003.cpp:343
>>>>>>>> Exception in thread "Thread-0"
>>>>>>>> java.lang.NoClassDefFoundError:
>>>>>>>> java/lang/Threadd
>>>>>>>> at
>>>>>>>> nsk.jvmti.scenarios.allocation.AP04.ap04t003.runIterateOverHeap(Native
>>>>>>>>
>>>>>>>> Method)
>>>>>>>> at
>>>>>>>> nsk.jvmti.scenarios.allocation.AP04.ap04t003HeapIterator.runIteration(ap04t003.java:140)
>>>>>>>> at
>>>>>>>> nsk.jvmti.scenarios.allocation.AP04.ap04t003Thread.run(ap04t003.java:201)
>>>>>>>> Caused by:
>>>>>>>> java.lang.ClassNotFoundException:
>>>>>>>> java.lang.Threadd
>>>>>>>> at
>>>>>>>> java.base/jdk.internal.loader.BuiltinClassLoader.loadClass(BuiltinClassLoader.java:583)
>>>>>>>> at
>>>>>>>> java.base/jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(ClassLoaders.java:178)
>>>>>>>> at
>>>>>>>> java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:521)
>>>>>>>> ... 3 more
>>>>>>>> FATAL ERROR in native method: JNI
>>>>>>>> method FindClass : internal error
>>>>>>>> from ap04t003.cpp:343
>>>>>>>> at
>>>>>>>> nsk.jvmti.scenarios.allocation.AP04.ap04t003.runIterateOverHeap(Native
>>>>>>>>
>>>>>>>> Method)
>>>>>>>> at
>>>>>>>> nsk.jvmti.scenarios.allocation.AP04.ap04t003HeapIterator.runIteration(ap04t003.java:140)
>>>>>>>> at
>>>>>>>> nsk.jvmti.scenarios.allocation.AP04.ap04t003Thread.run(ap04t003.java:201)
>>>>>>>>
>>>>>>>> Questions/comments I have about
>>>>>>>> this are:
>>>>>>>> - Do we want to force fatal errors
>>>>>>>> when a JNI call fails in general?
>>>>>>>> Most of these tests do the right
>>>>>>>> thing and test the return of the JNI
>>>>>>>> calls, for example:
>>>>>>>> thrClass =
>>>>>>>> jni->FindClass("java/lang/Threadd",
>>>>>>>> TRACE_JNI_CALL);
>>>>>>>> if (thrClass == NULL) {
>>>>>>>>
>>>>>>>> but now the wrapper actually would do
>>>>>>>> a fatal if the FindClass call would
>>>>>>>> return a nullptr, so we could remove
>>>>>>>> that test altogether. What do you
>>>>>>>> think?
>>>>>>>> - I prefer to leave them as the
>>>>>>>> tests then become closer to what real
>>>>>>>> users would have in their code and is
>>>>>>>> the "recommended" way of doing it
>>>>>>>>
>>>>>>>> - The alternative is to use the
>>>>>>>> NonFatalError I added which then just
>>>>>>>> prints out that something went wrong,
>>>>>>>> letting the test continue. Question
>>>>>>>> will be what should be the default?
>>>>>>>> The fatal or the non-fatal error
>>>>>>>> handling?
>>>>>>>>
>>>>>>>> On a different subject:
>>>>>>>> - On the new tests, I've removed
>>>>>>>> the NSK_JNI_VERIFY since the JNI
>>>>>>>> wrapper handles the tracing and the
>>>>>>>> verify in almost the same way; only
>>>>>>>> difference I can really tell is that
>>>>>>>> the complain method from NSK has a
>>>>>>>> max complain before stopping to
>>>>>>>> "complain"; I have not added that
>>>>>>>> part of the code in this webrev
>>>>>>>>
>>>>>>>> Once we decide on these, I can
>>>>>>>> continue on the files from
>>>>>>>> JDK-8212884 and then do both the
>>>>>>>> assignment in an if extraction
>>>>>>>> followed-by this type of webrev in an
>>>>>>>> easier fashion. Depending on
>>>>>>>> decisions here, NSK*VERIFY can be
>>>>>>>> deprecated as well as we go forward.
>>>>>>>>
>>>>>>>> Thank you for the reviews/comments :)
>>>>>>>> Jc
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> --
>>>>>>>> Thanks,
>>>>>>>> Jc
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> --
>>>>>>> Thanks,
>>>>>>> Jc
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> --
>>>>>> Thanks,
>>>>>> Jc
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> --
>>>>> Thanks,
>>>>> Jc
>>>>
>>>>
>>>>
>>>>
>>>> --
>>>> Thanks,
>>>> Jc
>>>
>>
>>
>>
>> --
>> Thanks,
>> Jc
>>
>>
>>
>> --
>>
>> Thanks,
>> Jc
More information about the hotspot-dev
mailing list