I’m still ok with this. /Staffan
On 3 jul 2014, at 16:42, Jaroslav Bachorik <jaroslav.bacho...@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. >>>>>> >>>>> >>>> >>> >> >
