Hi Thank you for review and feedback. See my comments inline.
> On Mar 19, 2020, at 6:03 PM, [email protected] wrote: > > Hi Leonid, > > It looks good in general. > Just a couple of comments. > > > http://cr.openjdk.java.net/~lmesnik/8241123/webrev.01/test/hotspot/jtreg/vmTestbase/nsk/share/Wicket.java.frames.html > > <http://cr.openjdk.java.net/~lmesnik/8241123/webrev.01/test/hotspot/jtreg/vmTestbase/nsk/share/Wicket.java.frames.html> > 168 public int waitFor(long timeout) { > 169 if (timeout < 0) > 170 throw new IllegalArgumentException( > 171 "timeout value is negative: " + timeout); > 172 > 173 long id = System.currentTimeMillis(); > 174 > 175 try { > 176 lock.lock(); > 177 --waiters; > 178 if (debugOutput != null) { > 179 debugOutput.printf("Wicket %d %s: waitFor(). There are > %d waiters totally now.\n", id, name, waiters); > 180 } > 181 > 182 long waitTime = timeout; > 183 long startTime = System.currentTimeMillis(); > 184 > 185 while (count > 0 && waitTime > 0) { > 186 try { > 187 condition.await(waitTime, TimeUnit.MILLISECONDS); > 188 } catch (InterruptedException e) { > 189 } > 190 waitTime = timeout - (System.currentTimeMillis() - > startTime); > 191 } > 192 --waiters; > 193 return count; > 194 } finally { > 195 lock.unlock(); > 196 } > 197 } > > The waiters probably needs to be incremented instead of decremented at line: > 177 --waiters; Thank you, fixed. > > http://cr.openjdk.java.net/~lmesnik/8241123/webrev.01/test/hotspot/jtreg/vmTestbase/nsk/share/runner/ThreadsRunner.java.udiff.html > > <http://cr.openjdk.java.net/~lmesnik/8241123/webrev.01/test/hotspot/jtreg/vmTestbase/nsk/share/runner/ThreadsRunner.java.udiff.html> > private void waitForOtherThreads() { > if (shouldWait) { > shouldWait = false; > - finished.unlock(); > - finished.waitFor(); > + finished.decrementAndGet(); > + while (finished.get() != 0) { > + try { > + Thread.sleep(1000); > + } catch (InterruptedException ie) { > + } > + } > } else { > throw new TestBug("Waiting a second time is not premitted"); > } > } > > Should we use a shorter sleep, something like Thread.sleep(100)? > These tests executed 30 or 60 seconds now by default, so sleeping 1 sec doesn't increase overall time. But tI am fine to change it 100, it also should works fine. Leonid > > Thanks, > Serguei > > > On 3/18/20 15:18, Leonid Mesnik wrote: >> >> On 3/18/20 2:30 PM, Igor Ignatyev wrote: >>>> I need more time to get grasp of Wicket and your changes in it; will come >>>> back to you after I understand them. >>> ok, now when I believe that I have enough understanding of Wicket, I have a >>> few comments: >>> 1. >>>> 68 private Lock lock = new ReentrantLock(); >>>> 69 private Condition condition = lock.newCondition(); >>> it's better to make these fields final. >>> >>> 2. as all writes and reads of Wicket::count are guarded by lock.lock, there >>> is no need for it to be atomic. >>> 3. adding lock to getWaiters will also remove need for Wicket::waiters to >>> be atomic. >> All 3 are fixed. Thanks for your suggestions. >> >> Updated version: >> >> http://cr.openjdk.java.net/~lmesnik/8241123/webrev.01/ >> <http://cr.openjdk.java.net/~lmesnik/8241123/webrev.01/> >> Leonid >> >>> >>> the rest looks good to me. >>> >>> Thanks, >>> -- Igor >>> >>> >>> >>>> On Mar 18, 2020, at 12:48 PM, Igor Ignatyev <[email protected] >>>> <mailto:[email protected]>> wrote: >>>> >>>> Hi Leonid, >>>> >>>> I've started looking at your webrev, and so far have a couple questions: >>>> >>>>> Test >>>>> vmTestbase/nsk/jdi/ObjectReference/referringObjects/referringObjects003/referringObjects003a.java >>>>> was updated to don't use Wicket. (The lock has a reference to thread >>>>> which affects test.) >>>> can't you use just a volatile boolean field? >>>> >>>>> Wicket "finished" in class ThreadsRunner was changed to atomicInt/sleep >>>>> to avoid OOME in j.u.c.l.Condition::await() which might happened in >>>>> stress GC tests. >>>> won't j.u.c.CountDownLatch be more appropriate and cleaner solution here? >>>> >>>> I need more time to get grasp of Wicket and your changes in it; will come >>>> back to you after I understand them. >>>> >>>> -- Igor >>>> >>>>> On Mar 18, 2020, at 12:37 PM, Leonid Mesnik <[email protected] >>>>> <mailto:[email protected]>> wrote: >>>>> >>>>> Hi >>>>> >>>>> Could you please review following fix which slightly refactor vmTestbase >>>>> stress test harness. This refactoring helps to add virtual threads >>>>> testing support. >>>>> >>>>> The Wicket uses plain sync/wait/notify mechanism which cause carrier >>>>> thread starvation and should not be used in virtual threads. The >>>>> ManagedThread is a subclass of Thread so it couldn't be virtual thread. >>>>> >>>>> >>>>> Following fix changes Wicket to use locks/conditions to don't pin vthread >>>>> to carrier thread while starting testing. >>>>> >>>>> ManagedThread is fixed to keep execution thread as the thread variable >>>>> and isolate it's creation. >>>>> >>>>> Test >>>>> vmTestbase/nsk/jdi/ObjectReference/referringObjects/referringObjects003/referringObjects003a.java >>>>> was updated to don't use Wicket. (The lock has a reference to thread >>>>> which affects test.) >>>>> >>>>> Wicket "finished" in class ThreadsRunner was changed to atomicInt/sleep >>>>> to avoid OOME in j.u.c.l.Condition::await() which might happened in >>>>> stress GC tests. >>>>> >>>>> webrev: http://cr.openjdk.java.net/~lmesnik/8241123/webrev.00/ >>>>> <http://cr.openjdk.java.net/~lmesnik/8241123/webrev.00/> >>>>> >>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8241123 >>>>> <https://bugs.openjdk.java.net/browse/JDK-8241123> >>>>> >>>>> >>>>> Leonid >>>>> >>>> >>> >
