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

[Ivan Gerasimov]

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