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