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

[David Holmes]

Looks good - thanks Leonid.

David

On 2/04/2020 12:17 pm, Leonid Mesnik wrote:
> 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