Re: RFR JDK-8216386: vmTestbase/nsk/jvmti/PopFrame/popframe005/TestDescription.java fails
Hi Alex, It looks good to me. Thanks, Serguei On 1/17/19 12:21 PM, Alex Menkov wrote: Hi Gary, On 01/17/2019 10:53, Gary Adams wrote: I like the fact that test.timeout.factor is applied as a multiplier. It's not clear why an upper limit had to be added. As you noted there 3 cases where Thread.join() is called, but the expected behavior is different. objWaiterThr2 and popFrameClsThr are expected to exit, but objWaiterThr1 is expected to be alive after join (i.e. the call is expected to take the time specified). I don't want to increase the test run time (for timeout.factor == 10 it would take 18 extra seconds), so I restricted the timeout for the case. Is that 50 or 5 seconds? Thank you for the catch. It should be 5 seconds (5000 ms) Updated webrev: http://cr.openjdk.java.net/~amenkov/popframe005_wait_time/webrev.02/ --alex 148 objWaiterThr1.join(Math.min(WAIT_TIME, 5)); Why are the other wait times not limited? 136 objWaiterThr2.join(WAIT_TIME); ... 169 popFrameClsThr.join(WAIT_TIME); If you need to apply the upper limit, then it would be better to apply it at the beginning. 38 static final long WAIT_TIME = Math.min(Utils.adjustTimeout(2000),5000); On 1/16/19, 6:28 PM, Alex Menkov wrote: Hi all, please review a fix for https://bugs.openjdk.java.net/browse/JDK-8216386 webrev: http://cr.openjdk.java.net/~amenkov/popframe005_wait_time/webrev/ The fix updates WAIT_TIME to depend on test.timeout.factor system property. WAIT_TIME value is used as argument of Thread.join(). For the case when the thread is expected to be alive (i.e. Thread.join() exits by timeout) the timeout value is restricted by 5 seconds to avoid long run time with big timeout.factor values. --alex
Re: RFR JDK-8216386: vmTestbase/nsk/jvmti/PopFrame/popframe005/TestDescription.java fails
Hi Gary, On 01/17/2019 10:53, Gary Adams wrote: I like the fact that test.timeout.factor is applied as a multiplier. It's not clear why an upper limit had to be added. As you noted there 3 cases where Thread.join() is called, but the expected behavior is different. objWaiterThr2 and popFrameClsThr are expected to exit, but objWaiterThr1 is expected to be alive after join (i.e. the call is expected to take the time specified). I don't want to increase the test run time (for timeout.factor == 10 it would take 18 extra seconds), so I restricted the timeout for the case. Is that 50 or 5 seconds? Thank you for the catch. It should be 5 seconds (5000 ms) Updated webrev: http://cr.openjdk.java.net/~amenkov/popframe005_wait_time/webrev.02/ --alex 148 objWaiterThr1.join(Math.min(WAIT_TIME, 5)); Why are the other wait times not limited? 136 objWaiterThr2.join(WAIT_TIME); ... 169 popFrameClsThr.join(WAIT_TIME); If you need to apply the upper limit, then it would be better to apply it at the beginning. 38 static final long WAIT_TIME = Math.min(Utils.adjustTimeout(2000),5000); On 1/16/19, 6:28 PM, Alex Menkov wrote: Hi all, please review a fix for https://bugs.openjdk.java.net/browse/JDK-8216386 webrev: http://cr.openjdk.java.net/~amenkov/popframe005_wait_time/webrev/ The fix updates WAIT_TIME to depend on test.timeout.factor system property. WAIT_TIME value is used as argument of Thread.join(). For the case when the thread is expected to be alive (i.e. Thread.join() exits by timeout) the timeout value is restricted by 5 seconds to avoid long run time with big timeout.factor values. --alex
Re: RFR JDK-8216386: vmTestbase/nsk/jvmti/PopFrame/popframe005/TestDescription.java fails
I like the fact that test.timeout.factor is applied as a multiplier. It's not clear why an upper limit had to be added. Is that 50 or 5 seconds? 148 objWaiterThr1.join(Math.min(WAIT_TIME, 5)); Why are the other wait times not limited? 136 objWaiterThr2.join(WAIT_TIME); ... 169 popFrameClsThr.join(WAIT_TIME); If you need to apply the upper limit, then it would be better to apply it at the beginning. 38 static final long WAIT_TIME = Math.min(Utils.adjustTimeout(2000),5000); On 1/16/19, 6:28 PM, Alex Menkov wrote: Hi all, please review a fix for https://bugs.openjdk.java.net/browse/JDK-8216386 webrev: http://cr.openjdk.java.net/~amenkov/popframe005_wait_time/webrev/ The fix updates WAIT_TIME to depend on test.timeout.factor system property. WAIT_TIME value is used as argument of Thread.join(). For the case when the thread is expected to be alive (i.e. Thread.join() exits by timeout) the timeout value is restricted by 5 seconds to avoid long run time with big timeout.factor values. --alex
Re: RFR JDK-8216386: vmTestbase/nsk/jvmti/PopFrame/popframe005/TestDescription.java fails
Hi Alex, Looks good to me. Thanks! Best regards, Daniil On 1/17/19, 9:47 AM, "Alex Menkov" wrote: Hi Daniil, On 01/16/2019 22:27, Daniil Titov wrote: > Hi Alex, > > The change looks good to me but I think the copyright comment needs to be updated for year 2019. Updated the webrev in-place. --alex > > Thanks. > > Best regards, > Daniil > > On 1/16/19, 3:29 PM, "serviceability-dev on behalf of Alex Menkov" wrote: > > Hi all, > > please review a fix for > https://bugs.openjdk.java.net/browse/JDK-8216386 > webrev: > http://cr.openjdk.java.net/~amenkov/popframe005_wait_time/webrev/ > > The fix updates WAIT_TIME to depend on test.timeout.factor system property. > WAIT_TIME value is used as argument of Thread.join(). > For the case when the thread is expected to be alive (i.e. Thread.join() > exits by timeout) the timeout value is restricted by 5 seconds to avoid > long run time with big timeout.factor values. > > --alex > > > >
Re: RFR JDK-8216386: vmTestbase/nsk/jvmti/PopFrame/popframe005/TestDescription.java fails
Hi Daniil, On 01/16/2019 22:27, Daniil Titov wrote: Hi Alex, The change looks good to me but I think the copyright comment needs to be updated for year 2019. Updated the webrev in-place. --alex Thanks. Best regards, Daniil On 1/16/19, 3:29 PM, "serviceability-dev on behalf of Alex Menkov" wrote: Hi all, please review a fix for https://bugs.openjdk.java.net/browse/JDK-8216386 webrev: http://cr.openjdk.java.net/~amenkov/popframe005_wait_time/webrev/ The fix updates WAIT_TIME to depend on test.timeout.factor system property. WAIT_TIME value is used as argument of Thread.join(). For the case when the thread is expected to be alive (i.e. Thread.join() exits by timeout) the timeout value is restricted by 5 seconds to avoid long run time with big timeout.factor values. --alex
Re: RFR JDK-8216386: vmTestbase/nsk/jvmti/PopFrame/popframe005/TestDescription.java fails
Hi Alex, The change looks good to me but I think the copyright comment needs to be updated for year 2019. Thanks. Best regards, Daniil On 1/16/19, 3:29 PM, "serviceability-dev on behalf of Alex Menkov" wrote: Hi all, please review a fix for https://bugs.openjdk.java.net/browse/JDK-8216386 webrev: http://cr.openjdk.java.net/~amenkov/popframe005_wait_time/webrev/ The fix updates WAIT_TIME to depend on test.timeout.factor system property. WAIT_TIME value is used as argument of Thread.join(). For the case when the thread is expected to be alive (i.e. Thread.join() exits by timeout) the timeout value is restricted by 5 seconds to avoid long run time with big timeout.factor values. --alex
RFR JDK-8216386: vmTestbase/nsk/jvmti/PopFrame/popframe005/TestDescription.java fails
Hi all, please review a fix for https://bugs.openjdk.java.net/browse/JDK-8216386 webrev: http://cr.openjdk.java.net/~amenkov/popframe005_wait_time/webrev/ The fix updates WAIT_TIME to depend on test.timeout.factor system property. WAIT_TIME value is used as argument of Thread.join(). For the case when the thread is expected to be alive (i.e. Thread.join() exits by timeout) the timeout value is restricted by 5 seconds to avoid long run time with big timeout.factor values. --alex
