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

[Leonid Mesnik]

Thank you for review.

I still waiting for one more review.

Leonid

On 4/1/20 9:18 PM, David Holmes wrote:
> 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