LGTM -- Igor
> On Apr 2, 2020, at 12:51 PM, Leonid Mesnik <leonid.mes...@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
