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

[Leonid Mesnik]

Find new version (updated webrev.00 by mistake)

http://cr.openjdk.java.net/~lmesnik/8241456/webrev.00/

The moving lock.lock() outside of try is required to don't get 
IllegalStateException.

And avoiding locks in ThreadRunner is needed to avoid OOME.

Leonid

On 3/26/20 4:41 PM, Leonid Mesnik wrote:
>
> On 3/26/20 4:29 PM, David Holmes wrote:
>> 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.
> Ok, I will move lock.lock() outside of try {}. Thanks for explanation.
>>
>>>>
>>>> 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.
>
> The idea is to add some nsk/share stress tests for virtual threads. 
> Basically, there are the same tests as existing (gc, sysdict) but 
> running in virtual threads. And these tests are going to be executed 
> after loom is integrated. And I want to keep the difference as small 
> as possible between mainline and loom.
>
> Leonid
>
>>
>> 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