Hi Martin, On 11/19/2014 12:23 PM, Martin Buchholz wrote:
The latest version looks good, but here are some more nitpicks:I'd prefer the name "deadline" to "endTime" since we already use that convention in j.u.c., e.g
Ok, applied to the webrev:
final long deadline = System.nanoTime() + nanosTimeout;
With the "deadline" style of checking, variable remainingNanos becomes
unnecessary, and you can just do
do { ... } while (deadline - System.nanoTime() > 0)
Roger
On Tue, Nov 18, 2014 at 2:28 PM, roger riggs <[email protected]> wrote:Hi, Done, added the test and immediate return after the wait. On Windows, the Thread.interrupted check needs to be done first to be consistent with Unix. Refactored a bit to remove a local variable and extra arithmetic computing the remainingNanos. Updated: http://cr.openjdk.java.net/~rriggs/webrev-basic-notenough-8064932/ Roger On 11/18/2014 4:58 PM, Martin Buchholz wrote:I think we're all 3 in agreement on the general direction, even if we disagree on the details. Once we properly round to millis, it is no longer necessary to use Math.max - just wait(NANOSECONDS.toMillis(rem + 999_999L)); As in my proposed change, I would like every call to wait immediately followed by a check if (hasExited) return true; to avoid the extra call to nanoTime. On Tue, Nov 18, 2014 at 12:09 PM, roger riggs <[email protected]> wrote:Hi, Work on Object.wait is an issue to be taken up separately. I agree that the timeout values should not be truncated to milliseconds, likely causing an additional cycle through the while loop and waiting. The Process.waitFor methods are expected to wait for the specified time to elapse. The webrev has been updated for both Windows and Unix to use a ceiling function on milliseconds and ensure that at least the requested time has elapsed. Please review: http://cr.openjdk.java.net/~rriggs/webrev-basic-notenough-8064932/ Thanks, Roger On 11/18/2014 11:08 AM, Martin Buchholz wrote:On Mon, Nov 17, 2014 at 8:54 PM, David Holmes <[email protected]> wrote:On 18/11/2014 2:43 PM, Martin Buchholz wrote: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.I don't really see the point of adding the extra math, plus an extra call (the two arg wait will call the one arg version) for no actual gain.The idea was to code to the Object.wait API, not its current implementation with only millisecond resolution. Even if we code to the current implementation, we should round UP not down, to avoid the problem of needing to call wait twice in case of early return. That would also be progress.If you want to add a fast exit path that's fine but the rest seems superfluous to me.
