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

[Igor Ignatyev]

LGTM

-- Igor

> On Apr 2, 2020, at 12:51 PM, Leonid Mesnik <leonid.mesnik at oracle.com> wrote:
> 
> 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