RFR: 8241456: ThreadRunner shouldn't use Wicket for threads starting synchronization

[David Holmes]

Hi Leonid,

On 27/03/2020 7:39 am, Leonid Mesnik wrote:
> Replying with correct summary.
> 
> Leonid
> 
> On 3/23/20 8:55 PM, Leonid Mesnik wrote:
>> Hi
>>
>> Could you please review following fix which update ThreadsRunner to 
>> use AtomicInteger/spinOnWait instead of Wicket to synchronize starting 
>> of stress test threads.
>>
>> Failing tests allocated all memory by earlier started threads before 
>> Lock.unlock is called in the latest threads. So thread might get an 
>> OOME exception while trying to release lock and/or get into 
>> inconsistent state.

You have a bug in Wicket:

+        try {
+            lock.lock();
...
+        } finally {
+            lock.unlock();

The lock() has to go outside the try block. That is why you were getting 
IllegalMonitorStateExceptions when the lock() threw OOME.

But the OOME itself is still a problem as it means you can't use any 
proper synchronizer. I don't like seeing the spin-loops but in this code 
you may have no choice if memory may already be exhausted.

David
-----


>>
>> The bug was introduced by 
>> https://bugs.openjdk.java.net/browse/JDK-8241123 
>> <https://bugs.openjdk.java.net/browse/JDK-8241123>
>> The Atomic works fine for stress test finishing sync. I just didn't 
>> expect that tests might OOME while releasing start lock.
>> Verified that tests now don't fail with -Xcomp -server 
>> -XX:-TieredCompilation -XX:-UseCompressedOops.
>>
>> webrev: http://cr.openjdk.java.net/~lmesnik/8241456/webrev.00/ 
>> <http://cr.openjdk.java.net/~lmesnik/8241456/webrev.00/>
>> bug: https://bugs.openjdk.java.net/browse/JDK-8241456 
>> <https://bugs.openjdk.java.net/browse/JDK-8241456>
>>
>> Leonid