Hi Alexey, Thanks for the review. I still do have to send a webrev up because I would need someone to sponsor it, test it, and push it :-) (I think Serguei said he would do it or was doing it so we might have to wait for his return).
So here it is with the reviewed-by filled in: http://cr.openjdk.java.net/~jcbeyler/8208303/webrev.04/ But for reference here, I actually changed this to: - fprintf(stderr, "fill_native_frames: Exception in jni NewWeakGlobalRef\n"); + JNI_ENV_PTR(jni)->FatalError( + JNI_ENV_ARG2(jni, "Error in event_storage_add: Exception in jni NewWeakGlobalRef")); which is consistent with what we would want : fail in the native code if there is a problem that is unexpected via FatalError. Thanks for the review! Jc On Wed, Aug 8, 2018 at 2:43 PM Alex Menkov <alexey.men...@oracle.com> wrote: > Hi Jc, > > The fix looks good to me too. > > minor note: > 564 if (JNI_ENV_PTR(jni)->ExceptionOccurred(JNI_ENV_ARG(jni))) { > 565 fprintf(stderr, "fill_native_frames: Exception in jni > NewWeakGlobalRef\n"); > 566 } > > I'd expect error message mention "event_storage_add" instead > "fill_native_frames" > No need to resent webrev. > > --alex > > On 08/08/2018 10:56, JC Beyler wrote: > > Hi Serguei, > > > > Sounds good to me. If someone else could review it, I could add the > > metadata to the webrev and, once testing (as you say) confirms nothing > > is broken, we could push it in. > > > > If someone is motivated, that would be awesome and much appreciated! > > Jc > > > > On Wed, Aug 8, 2018 at 12:08 AM serguei.spit...@oracle.com > > <mailto:serguei.spit...@oracle.com> <serguei.spit...@oracle.com > > <mailto:serguei.spit...@oracle.com>> wrote: > > > > Hi Jc, > > > > This looks good to me especially because we discussed it a lot > > internally. > > The testing looks pretty good too. > > I submitted mach5 jobs for HepMonitor tests repeating 100 times and > > also normal tier1, tier2, hs-tier2-5. > > But it was not tested with the tiers 6 & 7 yet (most likely, need to > > make sure they run well too). > > > > Thanks, > > Serguei > > > > > > On 8/6/18 08:27, JC Beyler wrote: > >> Hi all, > >> > >> I updated the version and Serguei tested it for me so this change > >> should not change the stability of the tests. This has the > >> advantage of not having to complicate the Java tests at all, while > >> adding the post-JNI call checks. > >> > >> Webrev: http://cr.openjdk.java.net/~jcbeyler/8208303/webrev.04/ > >> <http://cr.openjdk.java.net/%7Ejcbeyler/8208303/webrev.04/> > >> Bug: https://bugs.openjdk.java.net/browse/JDK-8208303 > >> > >> Thanks all for your reviews! > >> Jc > >> > >> On Fri, Jul 27, 2018 at 4:36 PM JC Beyler <jcbey...@google.com > >> <mailto:jcbey...@google.com>> wrote: > >> > >> Hi all, > >> > >> I did the new version that calls FatalError if JNI fails a > >> call. This has the advantage of not having to complicate the > >> Java tests at all, while adding the post-JNI call checks. > >> > >> Webrev: > >> http://cr.openjdk.java.net/~jcbeyler/8208303/webrev.03/ > >> <http://cr.openjdk.java.net/%7Ejcbeyler/8208303/webrev.03/> > >> Bug: https://bugs.openjdk.java.net/browse/JDK-8208303 > >> > >> Thanks all! > >> Jc > >> > >> On Thu, Jul 26, 2018 at 12:52 PM Chris Plummer > >> <chris.plum...@oracle.com <mailto:chris.plum...@oracle.com>> > >> wrote: > >> > >> I'm pretty sure changes that only affect tests can be any > >> priority. But still, be a lot more cautious the closer we > >> get to release. > >> > >> Chris > >> > >> On 7/26/18 12:15 PM, Daniel D. Daugherty wrote: > >>> We entered RDP2 today (07.26). So only P1 and P2 bug > >>> fixes allowed. > >>> > >>> Dan > >>> > >>> > >>> On 7/26/18 3:14 PM, serguei.spit...@oracle.com > >>> <mailto:serguei.spit...@oracle.com> wrote: > >>>> Yes, of course it has to be well tested before the push. > >>>> Does it make sense to plan it to push to 11 (after th > >>>> testing is done)? > >>>> > >>>> Thanks, > >>>> Serguei > >>>> > >>>> > >>>> On 7/26/18 12:08, Daniel D. Daugherty wrote: > >>>>> Please make sure this fix is well tested in Mach5 prior > >>>>> to pushing. > >>>>> In particular, I'm focused on reducing the noise in > >>>>> Mach5 tier[1-3] > >>>>> so adding any new failures there will make me grumpy :-) > >>>>> > >>>>> Dan > >>>>> > >>>>> > >>>>> On 7/26/18 3:03 PM, JC Beyler wrote: > >>>>>> Hi all, > >>>>>> > >>>>>> With the FatalError idea, here is the webrev to > >>>>>> consider, note it no longer changes the tests. If a > >>>>>> JNI call fails, then we call FatalError. > >>>>>> > >>>>>> Let me know what you think: > >>>>>> > >>>>>> Webrev: > >>>>>> http://cr.openjdk.java.net/~jcbeyler/8208303/webrev.01/ > <http://cr.openjdk.java.net/%7Ejcbeyler/8208303/webrev.01/> > >>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8208303 > >>>>>> > >>>>>> Thanks! > >>>>>> Jc > >>>>>> > >>>>>> On Thu, Jul 26, 2018 at 10:46 AM > >>>>>> serguei.spit...@oracle.com > >>>>>> <mailto:serguei.spit...@oracle.com> > >>>>>> <serguei.spit...@oracle.com > >>>>>> <mailto:serguei.spit...@oracle.com>> wrote: > >>>>>> > >>>>>> Hi Jc, > >>>>>> > >>>>>> Good idea. > >>>>>> I was thinking about something like this. > >>>>>> > >>>>>> Thanks, > >>>>>> Serguei > >>>>>> > >>>>>> > >>>>>> On 7/26/18 10:40, JC Beyler wrote: > >>>>>>> Hi Serguei, > >>>>>>> > >>>>>>> As I was looking at another test bug > >>>>>>> (https://bugs.openjdk.java.net/browse/JDK-8191519 > ); > >>>>>>> the proposal for that bug is to have a JNI call > >>>>>>> to FatalError to provoke a failure. > >>>>>>> > >>>>>>> If we went down that route, this webrev is > >>>>>>> simpler, no? Instead of setting failure_status > >>>>>>> and checking it later; just fail fatally and be > >>>>>>> done with it, no? That way, the tests in Java > >>>>>>> land don't have to be changed actually, no? > >>>>>>> > >>>>>>> What would we prefer for tests? Remember there > >>>>>>> was a failure and test it later or fail fast via > >>>>>>> JNI's FatalError? > >>>>>>> > >>>>>>> Thanks, > >>>>>>> Jc > >>>>>>> > >>>>>>> > >>>>>>> On Thu, Jul 26, 2018 at 10:04 AM > >>>>>>> serguei.spit...@oracle.com > >>>>>>> <mailto:serguei.spit...@oracle.com> > >>>>>>> <serguei.spit...@oracle.com > >>>>>>> <mailto:serguei.spit...@oracle.com>> wrote: > >>>>>>> > >>>>>>> Hi Jc, > >>>>>>> > >>>>>>> It looks good to me. > >>>>>>> > >>>>>>> Thanks, > >>>>>>> Serguei > >>>>>>> > >>>>>>> > >>>>>>> On 7/26/18 09:58, JC Beyler wrote: > >>>>>>>> Hi all, > >>>>>>>> > >>>>>>>> The tests in the HeapMonitor subsystem has a > >>>>>>>> lot of JNI calls. There is a need for > >>>>>>>> verification and testing if anything in the > >>>>>>>> JNI subsystem failed unexpectedly. > >>>>>>>> > >>>>>>>> Here is a webrev that tracks if a JNI call > >>>>>>>> does fail and the tests will fail if any JNI > >>>>>>>> call does fail. > >>>>>>>> > >>>>>>>> Could I have a few reviews please for: > >>>>>>>> Webrev: > >>>>>>>> > http://cr.openjdk.java.net/~jcbeyler/8208303/webrev.00/ > >>>>>>>> < > http://cr.openjdk.java.net/%7Ejcbeyler/8208303/webrev.00/> > >>>>>>>> Bug: > >>>>>>>> > https://bugs.openjdk.java.net/browse/JDK-8208303 > >>>>>>>> > >>>>>>>> Thanks, > >>>>>>>> Jc > >>>>>>> > >>>>>>> > >>>>>>> > >>>>>>> -- > >>>>>>> > >>>>>>> Thanks, > >>>>>>> Jc > >>>>>> > >>>>>> > >>>>>> > >>>>>> -- > >>>>>> > >>>>>> Thanks, > >>>>>> Jc > >>>>> > >>>> > >>> > >> > >> > >> > >> -- > >> > >> Thanks, > >> Jc > >> > >> > >> > >> -- > >> > >> Thanks, > >> Jc > > > > > > > > -- > > > > Thanks, > > Jc > -- Thanks, Jc
