[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    Re: RFR(M): 8028474: sun/jvmstat/monitor/MonitoredVm/MonitorVmStartTerminate.sh
From:       Staffan Larsen <staffan.larsen () oracle ! com>
Date:       2014-07-03 17:46:11
Message-ID: E1F16302-FC12-4AF2-8201-FFBBCB77477B () oracle ! com
[Download RAW message or body]

I’m still ok with this.

/Staffan

On 3 jul 2014, at 16:42, Jaroslav Bachorik <jaroslav.bachorik@oracle.com> wrote:

> On 07/03/2014 04:44 PM, Erik Gahlin wrote:
>> Hi Jaroslav,
>> 
>> Here is an updated webrev:
>> 
>> http://cr.openjdk.java.net/~egahlin/8028474_7/
> 
> Thanks. No more comments.
> 
> -JB-
> 
>> 
>> See comments inline.
>> 
>> Jaroslav Bachorik skrev 2014-07-03 16:03:
>>> Hi Erik,
>>> 
>>> nice cleanup!
>>> 
>>> L160 "break" statement after this line would save unnecessary
>>> iterating when a process has already been found
>>> 
>> I'm not a big fan of break statements in nested loops, so refactored out
>> a separate method that returns when the process has been found. Also
>> updated releaseStarted
>> 
>>> L172 You could use "return true". -
>>> 'MonitoredVmUtil.mainArgs(target).contains(args)' has been asserted to
>>> be true on L171
>>> 
>> 
>> Ops, not sure how it got there.
>> 
>> Thanks
>> Erik
>> 
>>> -JB-
>>> 
>>> On 07/03/2014 01:06 PM, Erik Gahlin wrote:
>>>> Hi Roger,
>>>> 
>>>> The test has been updated. It now uses System.nanoTime.
>>>> 
>>>> http://cr.openjdk.java.net/~egahlin/8028474_6/
>>>> 
>>>> Thanks
>>>> Erik
>>>> 
>>>> roger riggs skrev 2014-07-01 14:35:
>>>>> Hi Erik,
>>>>> 
>>>>> Consider switching to System.nanoTime;  it is not sensitive to clock
>>>>> changes
>>>>> and avoids leaving a land mine that may cause a spurious
>>>>> non-repeatable test failure.
>>>>> 'Deducing it from the log' means there is a failure and creates
>>>>> probably an hour or two of work
>>>>> for some quality engineer and burns a couple of hours re-running the
>>>>> test run.
>>>>> 
>>>>> Roger
>>>>> 
>>>>> 
>>>>> 
>>>>> On 7/1/2014 3:37 AM, Erik Gahlin wrote:
>>>>>> 
>>>>>> 
>>>>>>> JavaProcess.waitForRemoval: How about using timestamps
>>>>>>> (currentTimeMillis()) before the loop and for each ite
>>>>>>> ration to determine if the timeout has expired (instead of
>>>>>>> "time+=100”)?
>>>>>>> 
>>>>>> The code now uses currentTimeMillis(). Premature timeouts due to
>>>>>> clock changes can be deduced from the log.
>>>>>> 
>>>>> 
>>>> 
>>> 
>> 
> 

[prev in list] [next in list] [prev in thread] [next in thread]