RFR (L) 8213501 : Deploy ExceptionJniWrapper for a few tests
JC Beyler
jcbeyler at google.com
Wed Nov 28 00:36:47 UTC 2018
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>
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 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>
> 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/
>> 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
>>
>> 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>
>> 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
>>>
>>> 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
>>> 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>
>>> 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 <
>>>> 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> 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/
>>>>>> 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
More information about the hotspot-dev
mailing list