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

[Ivan Gerasimov]

Thank you Roger!

The fix addresses the issue with System.currentTimeMillis() and 
returning early from sleep()/join(), so it looks good!

I think it's better to wrap the line 1304 up to improve readability.  
Maybe import j.u.c.TimeUnit.NANOSECONDS, so that `TimeUnit.' could be 
omitted?

No further comments from me.

With kind regards,

Ivan


On 9/10/18 9:54 AM, Roger Riggs wrote:
> 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
>>>>
>>>>
>>>
>>>
>>
>
>

-- 
With kind regards,
Ivan Gerasimov