[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: Re: RFR: 8241123: Refactor vmTestbase stress framework to use j.u.c and make creation of threads mor
From: "serguei.spitsyn () oracle ! com" <serguei ! spitsyn () oracle ! com>
Date: 2020-03-20 2:05:24
Message-ID: afd2d820-82b2-6869-7622-d49b7736590c () oracle ! com
[Download RAW message or body]
<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<div class="moz-cite-prefix">Than you for fixing.<br>
Then I do not need to see another webrev.<br>
<br>
Thanks,<br>
Serguei<br>
<br>
<br>
On 3/19/20 18:10, Leonid Mesnik wrote:<br>
</div>
<blockquote type="cite"
cite="mid:E18735A7-ACB8-4AB3-8170-99CA1FF7DD70@oracle.com">Hi<br
class="">
<div><br class="">
</div>
<div>Thank you for review and feedback. See my comments inline.</div>
<div><br class="">
<blockquote type="cite" class="">
<div class="">On Mar 19, 2020, at 6:03 PM, <a
href="mailto:serguei.spitsyn@oracle.com" class=""
moz-do-not-send="true">serguei.spitsyn@oracle.com</a>
wrote:</div>
<br class="Apple-interchange-newline">
<div class="">
<div class="">
<div class="moz-cite-prefix">Hi Leonid,<br class="">
<br class="">
It looks good in general.<br class="">
Just a couple of comments.<br class="">
<br class="">
<br class="">
<a class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/~lmesnik/8241123/webrev.01/test/hotspot/jtreg/vmTestbase/nsk/share/Wicket.java.frames.html"
moz-do-not-send="true">http://cr.openjdk.java.net/~lmesnik/8241123/webrev.01/test/hotspot/jtreg/vmTestbase/nsk/share/Wicket.java.frames.html</a><br
class="">
<pre class=""><span class="changed"> 168 public int waitFor(long \
timeout) {</span> 169 if (timeout < 0)
170 throw new IllegalArgumentException(
171 "timeout value is negative: " + timeout);
<span class="changed"> 172 </span>
<span class="changed"> 173 long id = System.currentTimeMillis();</span>
<span class="changed"> 174 </span>
<span class="changed"> 175 try {</span>
<span class="changed"> 176 lock.lock();</span>
<span class="changed"> 177 --waiters;</span>
<span class="changed"> 178 if (debugOutput != null) {</span>
<span class="changed"> 179 debugOutput.printf("Wicket %d %s: \
waitFor(). There are %d waiters totally now.\n", id, name, waiters);</span> <span \
class="changed"> 180 }</span> <span class="changed"> 181 </span>
182 long waitTime = timeout;
183 long startTime = System.currentTimeMillis();
<span class="new"> 184 </span>
185 while (count > 0 && waitTime > 0) {
186 try {
<span class="changed"> 187 condition.await(waitTime, \
TimeUnit.MILLISECONDS);</span> <span class="changed"> 188 } catch \
(InterruptedException e) {</span> <span class="changed"> 189 }</span>
190 waitTime = timeout - (System.currentTimeMillis() - startTime);
191 }
192 --waiters;
<span class="changed"> 193 return count;</span>
<span class="changed"> 194 } finally {</span>
<span class="changed"> 195 lock.unlock();</span>
<span class="changed"> 196 }</span>
197 }</pre>
<br class="">
The waiters probably needs to be incremented instead of
decremented at line:<br class="">
<pre class=""><span class="changed"> 177 \
--waiters;</span></pre> </div>
</div>
</div>
</blockquote>
Thank you, fixed.<br class="">
<blockquote type="cite" class="">
<div class="">
<div class="">
<div class="moz-cite-prefix"> <br class="">
<a class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/~lmesnik/8241123/webrev.01/test/hotspot/jtreg/vmTestbase/nsk/share/runner/ThreadsRunner.java.udiff.html"
moz-do-not-send="true">http://cr.openjdk.java.net/~lmesnik/8241123/webrev.01/test/hotspot/jtreg/vmTestbase/nsk/share/runner/ThreadsRunner.java.udiff.html</a><br
class="">
<pre class=""> private void waitForOtherThreads() {
if (shouldWait) {
shouldWait = false;
<span class="removed">- finished.unlock();</span>
<span class="removed">- finished.waitFor();</span>
<span class="new">+ finished.decrementAndGet();</span>
<span class="new">+ while (finished.get() != 0) {</span>
<span class="new">+ try {</span>
<span class="new">+ Thread.sleep(1000);</span>
<span class="new">+ } catch (InterruptedException ie) {</span>
<span class="new">+ }</span>
<span class="new">+ }</span>
} else {
throw new TestBug("Waiting a second time is not premitted");
}
}
</pre>
Should we use a shorter sleep, something like<span
class="new"> Thread.sleep(100)</span><span class="new"></span><span
class="new"></span>?<br class="">
<br class="">
</div>
</div>
</div>
</blockquote>
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.</div>
<div><br class="">
</div>
<div>Leonid</div>
<div><br class="">
</div>
<div>
<blockquote type="cite" class="">
<div class="">
<div class="">
<div class="moz-cite-prefix"> <br class="">
Thanks,<br class="">
Serguei<br class="">
<br class="">
<br class="">
On 3/18/20 15:18, Leonid Mesnik wrote:<br class="">
</div>
<blockquote type="cite"
cite="mid:504b0902-9fd1-ea8c-399a-185a4ceaa9e0@oracle.com"
class="">
<p class=""><br class="">
</p>
<div class="moz-cite-prefix">On 3/18/20 2:30 PM, Igor
Ignatyev wrote:<br class="">
</div>
<blockquote type="cite"
cite="mid:4E0F364A-47F3-428D-9C08-6B1ADFCB9D24@oracle.com"
class="">
<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 class=""><span class="new"> 68 private Lock lock = \
new ReentrantLock();</span> <span class="new"> 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 class="">All 3 are fixed. Thanks for your
suggestions.<br class="">
</p>
<p class="">Updated version:</p>
<p class=""><a
href="http://cr.openjdk.java.net/~lmesnik/8241123/webrev.01/"
class="" \
moz-do-not-send="true">http://cr.openjdk.java.net/~lmesnik/8241123/webrev.01/</a></p> \
<p class="">Leonid<br class=""> </p>
<blockquote type="cite"
cite="mid:4E0F364A-47F3-428D-9C08-6B1ADFCB9D24@oracle.com"
class="">
<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 class=""><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>
</blockquote>
<br class="">
</div>
</div>
</blockquote>
</div>
<br class="">
</blockquote>
<br>
</body>
</html>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic