RFR (L) 8213501 : Deploy ExceptionJniWrapper for a few tests
JC Beyler
jcbeyler at google.com
Mon Dec 3 05:24:31 UTC 2018
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> 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>
> 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
>
--
Thanks,
Jc
More information about the hotspot-dev
mailing list