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

[David Holmes]

On 27/03/2020 9:16 am, Leonid Mesnik wrote:
> 
> On 3/26/20 4:06 PM, David Holmes wrote:
>> 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.
> Thanks for explanation. But anyway, as I understand locks use memory and 
> might be inconsistent if OOME happened.

They use memory and so lock() can throw OOME, but they are never 
inconsistent.

>>
>> 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.
> 
> It should be really short spin-loop, test only start thread during this 
> loop and don't do anything more. Also, it is done only once for all 
> stress test. The goal is to start thread completely before heap is 
> exhausted.

Okay. I'm somewhat dubious about making these changes in mainline now 
just to support loom. I don't see why we need to care about pinning 
threads in this kind of situation.

David

> Leonid
> 
>>
>> 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