RFR 8098798: Thread.join(ms) on Linux still affected by changes to the time-of-day clock

[Roger Riggs]

Hi Ivan,

Thanks for the suggestion.
Webrev updated:

    http://cr.openjdk.java.net/~rriggs/webrev-thread-early-8098798/

Thanks, Roger

On 9/4/2018 3:38 PM, Ivan Gerasimov wrote:
> Thank you Roger!
>
> I'm not sure if it plays any significant role, but an unnecessary call 
> to nanoTime() after wait(delay) could be avoided with the code like this:
>
>     if (millis > 0) {
>         if (isAlive()) {
>             final long startTime = System.nanoTime();
>             long delay = millis;
>             do {
>                 wait(delay);
>             } while (isAlive() && (delay = millis - 
> TimeUnit.NANOSECONDS.toMillis(System.nanoTime() - startTime)) > 0);
>         }
>     } else if (millis == 0) {
>
>
> With kind regards,
> Ivan
>
>
> On 9/4/18 12:02 PM, Roger Riggs wrote:
>> Hi Martin, Ivan,
>>
>> Thanks for the suggestions.
>>
>> Update in place:
>> http://cr.openjdk.java.net/~rriggs/webrev-thread-early-8098798/
>>
>> On 8/29/2018 5:36 PM, Martin Buchholz wrote:
>>> Thanks for taking this on.
>>> Wait loops are notoriously hard to get right...
>>>
>>> One sharp corner is that wait(0) waits forever, and TimeUnit 
>>> conversion truncates.  You can probably avoid those problems via 
>>> TimeUnit.timedWait.
>>>
>> Not trivial since a long cannot hold the combined time of 
>> millis(long) and nanos (long) in a TimeUnit(Nanos)
>> and the cumulative wait time needs to be measured by System.nanoTime().
>>
>>> In code like this in j.u.c. we try to optimize away the call to 
>>> nanoTime when waiting is not necessary, by using a special 
>>> "uninitialized" initial value for remaining nanos, e.g. in 
>>> FutureTask.awaitDone
>>>
>>>         long startTime = 0L;    // Special value 0L means not yet 
>>> parked
>> ok
>>>
>>> (I prefer the variable name "startTime")
>> ok
>>>
>>> (j.u.c. code can also be improved)
>>>
>>> (there's a pre-existing buglet - we should probably check for 
>>> overflow when millis = MAX_VALUE and nanos > 0 (sigh))
>> ok,
>>>
>>> (I would reorder clauses to first check the common case millis > 0, 
>>> then millis == 0, last the error case millis < 0)
>> ok
>>>
>>> (it's a bad API that millis < 0 is an error)
>> too late for a behavior change though I suppose its in the direction 
>> of not getting error instead of the opposite....
>>
>>
>> An Observation:
>>
>> Join(ms) and join(ms, ns) might wait a bit longer than strictly 
>> necessary because the bottom out in Object.wait(ms).
>> It might be better if both ended up calling Object.wait(ms, ns).
>> But since Object.wait(ms,ns) rounds up to Object.wait(ms) that won't 
>> make a difference and to take advantage
>> of a finer clock resolution would mean native/vm changes to support 
>> object.wait(ms, ns).
>>
>> I'm inclined to address only the immediate issues raised in the early 
>> return and the clock dependency now.
>>
>> (BTW, I found no tests for Thread.sleep or .join.)
>>
>> Thanks, Roger
>>
>>>
>>>
>>> On Wed, Aug 29, 2018 at 1:06 PM, Roger Riggs <Roger.Riggs at oracle.com 
>>> <mailto:Roger.Riggs at oracle.com>> wrote:
>>>
>>>     Please review changes for:
>>>
>>>     8098798: Thread.join(ms) on Linux still affected by changes to the
>>>     time-of-day clock
>>>          Use System.nanoTime to compute any remaining delay
>>>
>>>     8210004: Thread.sleep(millis, nanos) timeout returns early
>>>          Avoid an early return by rounding the milliseconds up if
>>>     there are any nanoseconds as was done in Object.wait(ms, ns).
>>>
>>>     (If its not appropriate to do the two reviews together, I can
>>>     split them).
>>>
>>>     Webrev:
>>> http://cr.openjdk.java.net/~rriggs/webrev-thread-early-8098798/
>>> <http://cr.openjdk.java.net/%7Erriggs/webrev-thread-early-8098798/>
>>>
>>>     Thanks, Roger
>>>
>>>
>>
>>
>