RFR [9] - 8060052: FutureTask; fix underflow when timeout = Long.MIN_VALUE
This is a formal review request for pulling the latest FutureTask from the 166 CVS. Webrev: http://cr.openjdk.java.net/~chegar/8060052/webrev.00/webrev/ The most significant issue being addresses is that task.get(Long.MIN_VALUE, NANOSECONDS) does not timeout for a really really long time, when it should throw TimeoutException almost immediately. There is some other refactoring around consistent usage of Unsafe. I took the liberty of adding a trivial test to give addition coverage in OpenJDK, but there is a TCK test in the 166 CVS that exercises this, and in fact the sync'ing of that test into the JCK 9 ea is what promoted this RFR. -Chris.
Thanks and approved! Seems suitable for a backport to 8u if you want to do that. Personally I would not add the openjdk test, but use noreg-jck, but up to you. Your test is certainly more focused on the bug fix than our jsr166 tck test. On Fri, Oct 10, 2014 at 8:29 AM, Chris Hegarty <chris.hegarty@oracle.com> wrote:
This is a formal review request for pulling the latest FutureTask from the 166 CVS.
Webrev: http://cr.openjdk.java.net/~chegar/8060052/webrev.00/webrev/
The most significant issue being addresses is that task.get(Long.MIN_VALUE, NANOSECONDS) does not timeout for a really really long time, when it should throw TimeoutException almost immediately. There is some other refactoring around consistent usage of Unsafe.
I took the liberty of adding a trivial test to give addition coverage in OpenJDK, but there is a TCK test in the 166 CVS that exercises this, and in fact the sync'ing of that test into the JCK 9 ea is what promoted this RFR.
-Chris.
Thanks for the review Martin. Since this change may be backported to 8u, then I’ll keep the test, as the JCK8(a) may not contain the new TCK test from the 166 CVS. -Chris. On 10 Oct 2014, at 18:40, Martin Buchholz <martinrb@google.com> wrote:
Thanks and approved!
Seems suitable for a backport to 8u if you want to do that.
Personally I would not add the openjdk test, but use noreg-jck, but up to you. Your test is certainly more focused on the bug fix than our jsr166 tck test.
On Fri, Oct 10, 2014 at 8:29 AM, Chris Hegarty <chris.hegarty@oracle.com> wrote: This is a formal review request for pulling the latest FutureTask from the 166 CVS.
Webrev: http://cr.openjdk.java.net/~chegar/8060052/webrev.00/webrev/
The most significant issue being addresses is that task.get(Long.MIN_VALUE, NANOSECONDS) does not timeout for a really really long time, when it should throw TimeoutException almost immediately. There is some other refactoring around consistent usage of Unsafe.
I took the liberty of adding a trivial test to give addition coverage in OpenJDK, but there is a TCK test in the 166 CVS that exercises this, and in fact the sync'ing of that test into the JCK 9 ea is what promoted this RFR.
-Chris.
Chris wrote:
Webrev: http://cr.openjdk.java.net/~chegar/8060052/webrev.00/webrev/
An aside remark: Every time some nanos timeout needs to be kept track of, and that remaining time does not need to be returned, one could consider the timeout to be infinite (timed = false) above, say, Long.MAX_VALUE/2 (not Long.MAX_VALUE, because some (actual) durations might be subtracted from the specified timeout by intermediate treatments), to get a small (in absolute) but relatively noticeable performance boost on configurations where System.nanoTime() is slow. In the curent case, if not wanting to have System.nanoTime() calls, there is the get() method, but generic code might use get(long,TimeUnit) even for huge timeouts. Or maybe we don't want users to dangerously tinker around this hidden and huge threshold... -Jeff
On Sat, Oct 11, 2014 at 4:56 PM, Jeff Hain <jeffhain@rocketmail.com> wrote:
Chris wrote:
Webrev: http://cr.openjdk.java.net/~chegar/8060052/webrev.00/webrev/
An aside remark:
Every time some nanos timeout needs to be kept track of, and that remaining time does not need to be returned, one could consider the timeout to be infinite (timed = false) above, say, Long.MAX_VALUE/2 (not Long.MAX_VALUE, because some (actual) durations might be subtracted from the specified timeout by intermediate treatments), to get a small (in absolute) but relatively noticeable performance boost on configurations where System.nanoTime() is slow.
In the curent case, if not wanting to have System.nanoTime() calls, there is the get() method, but generic code might use get(long,TimeUnit) even for huge timeouts.
I thought about this. It's a tradeoff between the small extra complexity and the small penalty when you don't have huge timeouts vs the slightly bigger performance benefit when you do. Not worth it. An existing API that only accepts a finite user-specified timeout could have an optimized implementation to call untimed get() when the user-specified timeout is close to Long.MAX_VALUE. Hmmm.... but maybe we should always be rechecking the state between calling nanoTime and parking, since parking/unparking is a bigger performance killer.... leveraging the slowness of nanoTime to our benefit by extending the "spinloop". --- src/main/java/util/concurrent/FutureTask.java 22 Aug 2014 03:30:56 -0000 1.106 +++ src/main/java/util/concurrent/FutureTask.java 14 Oct 2014 06:29:57 -0000 @@ -411,7 +411,8 @@ } parkNanos = nanos - elapsed; } - LockSupport.parkNanos(this, parkNanos); + if (state < COMPLETING) + LockSupport.parkNanos(this, parkNanos); } else LockSupport.park(this);
Or maybe we don't want users to dangerously tinker around this hidden and huge threshold...
-Jeff
participants (3)
-
Chris Hegarty
-
Jeff Hain
-
Martin Buchholz