RFR 9: 8064932: test java/lang/ProcessBuilder/Basic.java: waitFor didn't take long enough

[roger riggs]

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)
No really expendable, the remainingNanos value is needed for the wait() 
call on the retry.

Roger

>
> On Tue, Nov 18, 2014 at 2:28 PM, roger riggs <roger.riggs at oracle.com> 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 <roger.riggs at oracle.com>
>>> 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 <david.holmes at oracle.com>
>>>>> 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.
>>>>