Proposed sibling change http://cr.openjdk.java.net/~martin/webrevs/openjdk9/UNIXProcess.waitFor/ - don't unconditionally call nanoTime when the wait ends - use the millis/nanos form of Object.wait in case sub-millisecond waits are ever supported.
On Mon, Nov 17, 2014 at 6:28 PM, Martin Buchholz <marti...@google.com> wrote: > I was staring at that old process code I wrote many years ago, and I > think it can be improved. I'll post a patch later. > > On Mon, Nov 17, 2014 at 2:02 PM, roger riggs <roger.ri...@oracle.com> wrote: >> Hi, >> >> The technique used in the Linux version of Process.waitFor() is applied to >> the Windows version. The duration of the native wait is measured >> using nanoTime and the wait is repeated as necessary. >> For most uses, some jitter is expected due to workload, clock resolution, >> etc. >> >> Webrev: >> http://cr.openjdk.java.net/~rriggs/webrev-basic-notenough-8064932/ >> >> Thanks, Roger >> >> >> >> >> On 11/17/2014 2:54 PM, Martin Buchholz wrote: >>> >>> Returning early is EVIL. >>> ("""What part of 'wait for NNN nanoseconds' did you not understand??""") >>> Unfortunately, Object.wait may do so. And perhaps also >>> waitForMultipleObjects. >>> HIgher level libraries need to be paranoid and compensate. >>> >>> On Mon, Nov 17, 2014 at 11:27 AM, roger riggs <roger.ri...@oracle.com> >>> wrote: >>>> >>>> Hi, >>>> >>>> I need to go back and identify the platform of the failure; it is >>>> failing >>>> on Windows >>>> so the correct code is in ProcessImpl_md.c in waitForTimeoutInterruptibly >>>> in which the timeout is in milliseconds. >>>> >>>> If waitForMultipleObjects can return 'early' then the same kind of loop >>>> used on Linux for testing nanoTime may be needed. >>>> >>>> Roger >>>> >>>> >>>> >>>> >>>> On 11/14/2014 7:38 PM, Martin Buchholz wrote: >>>>> >>>>> Hi Roger, >>>>> >>>>> I keep staring at the code in UNIXProcess.java and am having trouble >>>>> imagining how waitFor could possibly return early - that loop can only >>>>> terminate when elapsed time as measured by System.nanoTime exceeds >>>>> timeoutAsNanos. It's true that we might have truncation when >>>>> converting to millis, but then the wait should get called a second >>>>> time with arg 1 (which is suboptimal, yes - we should improve waitFor >>>>> (and very sadly, we should also improve Object.wait)). >>>>> >>>>> Is there a way for me to repro this test failure? The JDK tries hard >>>>> to provide monotonic nanoTime(). >>>>> >>>>> On Fri, Nov 14, 2014 at 2:15 PM, roger riggs <roger.ri...@oracle.com> >>>>> wrote: >>>>>> >>>>>> Hi Martin, >>>>>> >>>>>> The artifact is by-product of using System.nanoTime to measure the >>>>>> duration >>>>>> in UNIXProcess and the conversion to milliseconds to call >>>>>> Object.wait(): >>>>>> >>>>>> The low order bits of nanoseconds are truncated in the conversion. >>>>>> >>>>>> long timeoutAsNanos = unit.toNanos(timeout); >>>>>> long startTime = System.nanoTime(); >>>>>> long rem = timeoutAsNanos; >>>>>> >>>>>> while (!hasExited && (rem > 0)) { >>>>>> wait(Math.max(TimeUnit.NANOSECONDS.toMillis(rem), 1)); >>>>>> rem = timeoutAsNanos - (System.nanoTime() - startTime); >>>>>> } >>>>>> >>>>>> The issue may be further complicated by the test logic also doing the >>>>>> timing in nanoSeconds when the best Object.wait can do is milliseconds. >>>>>> >>>>>> If System.nanoTime is to be used, perhaps it should be modulo >>>>>> milliseconds. >>>>>> >>>>>> Roger >>>>>> >>>>>> >>>>>> >>>>>> On 11/14/2014 2:57 PM, Martin Buchholz wrote: >>>>>>> >>>>>>> This sort of change may be a necessary concession to reality, but >>>>>>> before we go there ... as I said in a previous review, this test may >>>>>>> demonstrate a real bug in the jdk implementation. Is this >>>>>>> system-dependent? Is it easy to reproduce? Can we fix the JDK? >>>>>>> >>>>>>> On Fri, Nov 14, 2014 at 11:48 AM, roger riggs <roger.ri...@oracle.com> >>>>>>> wrote: >>>>>>>> >>>>>>>> Please review this test change to make the wait time in >>>>>>>> ProcessBuilder/Basic >>>>>>>> a bit more lenient. >>>>>>>> >>>>>>>> Webrev: >>>>>>>> http://cr.openjdk.java.net/~rriggs/webrev-basic-notenough-8064932/ >>>>>>>> >>>>>>>> Issue: >>>>>>>> 8064932: java/lang/ProcessBuilder/Basic.java: waitFor didn't >>>>>>>> take >>>>>>>> long >>>>>>>> enough >>>>>>>> >>>>>>>> Thanks, Roger >>>>>>>> >>
