RFR (L) 8213501 : Deploy ExceptionJniWrapper for a few tests
David Holmes
david.holmes at oracle.com
Mon Dec 3 06:29:08 UTC 2018
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