<html><head><meta http-equiv="content-type" content="text/html; charset=utf-8"></head><body dir="auto">Reviewed. <br><br><div dir="ltr">— Igor</div><div dir="ltr"><br><blockquote type="cite">On Mar 18, 2020, at 3:18 PM, Leonid Mesnik <Leonid.Mesnik@oracle.com> wrote:<br><br></blockquote></div><blockquote type="cite"><div dir="ltr">
<meta http-equiv="Content-Type" content="text/html;
charset=windows-1252">
<p><br>
</p>
<div class="moz-cite-prefix">On 3/18/20 2:30 PM, Igor Ignatyev
wrote:<br>
</div>
<blockquote type="cite" cite="mid:4E0F364A-47F3-428D-9C08-6B1ADFCB9D24@oracle.com">
<meta http-equiv="Content-Type" content="text/html;
charset=windows-1252">
<blockquote type="cite" class="">I need more time to get grasp of
Wicket and your changes in it; will come back to you after I
understand them. </blockquote>
ok, now when I believe that I have enough understanding of Wicket,
I have a few comments:
<div class="">1.<br class="">
<div class="">
<blockquote type="cite" class="">
<pre style="background-color: rgb(238, 238, 238);" class=""><span class="new" style="color: blue; font-weight: bold;"> 68 private Lock lock = new ReentrantLock();</span>
<span class="new" style="color: blue; font-weight: bold;"> 69 private Condition condition = lock.newCondition();</span></pre>
</blockquote>
<div class="">it's better to make these fields final.</div>
<div class=""><br class="">
</div>
<div class="">2. as all writes and reads of Wicket::count are
guarded by lock.lock, there is no need for it to be atomic.</div>
<div class="">3. adding lock to getWaiters will also remove
need for Wicket::waiters to be atomic.</div>
</div>
</div>
</blockquote>
<p>All 3 are fixed. Thanks for your suggestions.<br>
</p>
<p>Updated version:</p>
<p><a href="http://cr.openjdk.java.net/~lmesnik/8241123/webrev.01/">http://cr.openjdk.java.net/~lmesnik/8241123/webrev.01/</a></p>
<p>Leonid<br>
</p>
<blockquote type="cite" cite="mid:4E0F364A-47F3-428D-9C08-6B1ADFCB9D24@oracle.com">
<div class="">
<div class="">
<div class=""><br class="">
</div>
<div class="">the rest looks good to me.</div>
<div class=""><br class="">
</div>
<div class="">Thanks,</div>
<div class="">-- Igor</div>
<div class=""><br class="">
</div>
<div class=""><br class="">
</div>
<div><br class="">
<blockquote type="cite" class="">
<div class="">On Mar 18, 2020, at 12:48 PM, Igor Ignatyev
<<a href="mailto:igor.ignatyev@oracle.com" class="" moz-do-not-send="true">igor.ignatyev@oracle.com</a>>
wrote:</div>
<br class="Apple-interchange-newline">
<div class="">
<div class="">Hi Leonid,<br class="">
<br class="">
I've started looking at your webrev, and so far have a
couple questions:<br class="">
<br class="">
<blockquote type="cite" class="">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.)<br class="">
</blockquote>
can't you use just a volatile boolean field?<br class="">
<br class="">
<blockquote type="cite" class="">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.<br class="">
</blockquote>
won't j.u.c.CountDownLatch be more appropriate and
cleaner solution here?<br class="">
<br class="">
I need more time to get grasp of Wicket and your
changes in it; will come back to you after I
understand them. <br class="">
<br class="">
-- Igor<br class="">
<br class="">
<blockquote type="cite" class="">On Mar 18, 2020, at
12:37 PM, Leonid Mesnik <<a href="mailto:leonid.mesnik@oracle.com" class="" moz-do-not-send="true">leonid.mesnik@oracle.com</a>>
wrote:<br class="">
<br class="">
Hi<br class="">
<br class="">
Could you please review following fix which slightly
refactor vmTestbase stress test harness. This
refactoring helps to add virtual threads testing
support.<br class="">
<br class="">
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.<br class="">
<br class="">
<br class="">
Following fix changes Wicket to use locks/conditions
to don't pin vthread to carrier thread while
starting testing.<br class="">
<br class="">
ManagedThread is fixed to keep execution thread as
the thread variable and isolate it's creation.<br class="">
<br class="">
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.)<br class="">
<br class="">
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.<br class="">
<br class="">
webrev: <a href="http://cr.openjdk.java.net/~lmesnik/8241123/webrev.00/" class="" moz-do-not-send="true">http://cr.openjdk.java.net/~lmesnik/8241123/webrev.00/</a><br class="">
<br class="">
bug: <a href="https://bugs.openjdk.java.net/browse/JDK-8241123" class="" moz-do-not-send="true">https://bugs.openjdk.java.net/browse/JDK-8241123</a><br class="">
<br class="">
<br class="">
Leonid<br class="">
<br class="">
</blockquote>
<br class="">
</div>
</div>
</blockquote>
</div>
<br class="">
</div>
</div>
</blockquote>
</div></blockquote></body></html>