RFR (L) 8213501 : Deploy ExceptionJniWrapper for a few tests
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Tue Dec 4 04:40:49 UTC 2018
On 12/3/18 20:15, Chris Plummer wrote:
> 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.
In general, I'm Okay with get_dirname().
Just to mention dirname can be both short or full, so it is a little
confusing as well.
It is the reason why the get_basename() was suggested.
However, I do not insist on get_basename() nor get_full_dirname(). :)
Thanks,
Serguei
> 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