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.ri...@oracle.com
<mailto:roger.ri...@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
