Hi Martin,
I'm a bit confused about exactly what pieces need review here. Since you
mentioned me with respect to 8150523, I took a look at the webrev that adds the
timeout factors:
http://cr.openjdk.java.net/~martin/webrevs/openjdk9/jsr166-jdk9-integration/timeoutFactor/
Do other webrevs still need review as well? I haven't looked at them. But there
are others' names on them already....
Otherwise, overall, it looks fine, just a few minor questions/comments:
------------------------------------------------------------
In the following files, various delays of 7 sec, 20 sec, 30 sec, 60 sec, 100
sec, 120 sec, and 1000 sec were changed to 10 sec (exclusive of timeout factor
adjustment). Was that intentional? I guess making the "backstop" timeouts (e.g.,
waiting for an executor service to terminate) be uniform is reasonable, but
there were some very long timeouts that are now much shorter. At least,
something to keep an eye on.
test/java/util/concurrent/BlockingQueue/Interrupt.java:
test/java/util/concurrent/BlockingQueue/ProducerConsumerLoops.java
test/java/util/concurrent/BlockingQueue/SingleProducerMultipleConsumerLoops.java
test/java/util/concurrent/CompletableFuture/Basic.java
test/java/util/concurrent/ConcurrentHashMap/MapLoops.java
test/java/util/concurrent/ConcurrentQueues/ConcurrentQueueLoops.java
test/java/util/concurrent/Exchanger/ExchangeLoops.java
test/java/util/concurrent/Executors/AutoShutdown.java
test/java/util/concurrent/ScheduledThreadPoolExecutor/ZeroCorePoolSize.java
test/java/util/concurrent/ThreadPoolExecutor/Custom.java
test/java/util/concurrent/ThreadPoolExecutor/SelfInterrupt.java
test/java/util/concurrent/ThreadPoolExecutor/ThreadRestarts.java
test/java/util/concurrent/locks/Lock/FlakyMutex.java
test/java/util/concurrent/locks/LockSupport/ParkLoops.java
test/java/util/concurrent/locks/ReentrantLock/LockOncePerThreadLoops.java
test/java/util/concurrent/locks/ReentrantLock/SimpleReentrantLockLoops.java
test/java/util/concurrent/locks/ReentrantLock/TimeoutLockLoops.java
test/java/util/concurrent/locks/StampedLock/Basic.java
------------------------------------------------------------
test/java/util/concurrent/FutureTask/CancelledFutureLoops.java
test/java/util/concurrent/ThreadPoolExecutor/TimeOutShrink.java
It's slightly odd to see an additional multiplier at the use site of
LONG_DELAY_MS, when this doesn't occur in most of the other tests that
(formerly) had different timeouts. Put another way, why do these tests have
different timeouts, whereas the tests above that had widely differing timeouts
were all changed to 10 sec?
------------------------------------------------------------
In various scheduled thread pool executor tests, as well as in
test/java/util/concurrent/ThreadPoolExecutor/ThreadRestarts.java
should delays for scheduled tasks also be scaled as well? If the tests are
running in a slow environment, and some timeouts are scaled but not others, it
might result in some tasks executing too soon. I guess this depends on the
semantics of what's being tested.
------------------------------------------------------------
test/java/util/concurrent/ConcurrentQueues/GCRetention.java
- extra commented-out call to forceFullGc() ?
- probably would be wise to scale the timeout in finalizeDone.await()
------------------------------------------------------------
test/java/util/concurrent/Exchanger/ExchangeLoops.java
test/java/util/concurrent/ExecutorCompletionService/ExecutorCompletionServiceLoops.java
The number of iterations was reduced from 100,000 to 2,000, particularly the
initial "warm up" run (at least in ExchangeLoops). IIRC the C2 compiler kicks in
at 10,000 iterations. The reduced the number of iterations (particularly in the
initial "warm up" runs) doesn't meet this threshold. Could that be a problem?
s'marks
On 2/29/16 9:52 AM, Martin Buchholz wrote:
I added
8150523: improve jtreg test timeout handling, especially -timeout:
to this wave, inspired by smarks.
I stress tested this with the flags that caused JDK-8150523 to fail,
and they now seem robust, as long as a reasonable -timeout flag is
provided to jtreg.
On Tue, Feb 23, 2016 at 6:58 PM, Martin Buchholz <marti...@google.com> wrote:
A very boring jsr166 integration, focused on reliability.
This one has the promised "even more unnecessarily robust" ThreadLocalRandom.
http://cr.openjdk.java.net/~martin/webrevs/openjdk9/jsr166-jdk9-integration/
