Reviewed. — Igor
> On Mar 18, 2020, at 3:18 PM, Leonid Mesnik <[email protected]> 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/ > > Leonid > >> >> the rest looks good to me. >> >> Thanks, >> -- Igor >> >> >> >>> On Mar 18, 2020, at 12:48 PM, Igor Ignatyev <[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]> >>>> 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/ >>>> >>>> bug: https://bugs.openjdk.java.net/browse/JDK-8241123 >>>> >>>> >>>> Leonid >>>> >>> >>
