<html>
<head>
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
</head>
<body text="#000000" bgcolor="#FFFFFF">
On 2015-08-14 14:38, Stefan Johansson wrote:<br>
<blockquote cite="mid:55CDE143.20808@oracle.com" type="cite">
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
Hi Stefan,<br>
<br>
Nice change, looks really good,</blockquote>
<br>
Thanks.<br>
<br>
<blockquote cite="mid:55CDE143.20808@oracle.com" type="cite"> but I
have a few small comments.<br>
<br>
In workgroup.hpp:<br>
177 GangTaskDispatcher* dispatcher()const {<br>
Add a space before const.<br>
</blockquote>
<br>
Fixed.<br>
<br>
<blockquote cite="mid:55CDE143.20808@oracle.com" type="cite"> ---<br>
<br>
In workgroup.cpp:<br>
122 // Limit the semaphore value to the number of workers.<br>
123 _start_semaphore(new Semaphore()),<br>
124 _end_semaphore(new Semaphore())<br>
Seems like the limit has been lost. I would prefer to have it
added, but if that's somehow problematic just remove the comment.<br>
</blockquote>
<br>
Removed the comment.<br>
<br>
<blockquote cite="mid:55CDE143.20808@oracle.com" type="cite"> <br>
140 // Wait for the last worker to signal the coordinator.<br>
141 _end_semaphore->wait();<br>
142 <br>
143 // No workers are allowed to read the state variables
after the coordinator has been signaled.<br>
144 _task = NULL;<br>
145 _started = 0;<br>
146 _not_finished = 0;<br>
Do we need to set _not_finished to 0 or could we instead assert
that it already is 0 since that's when _end_semaphore should have
been signaled. <br>
</blockquote>
<br>
Changed into an assert.<br>
<br>
<blockquote cite="mid:55CDE143.20808@oracle.com" type="cite"> ---<br>
<br>
I don't need to see a new webrev for the above changes. Reviewed.<br>
</blockquote>
<br>
Thanks,<br>
StefanK<br>
<br>
<blockquote cite="mid:55CDE143.20808@oracle.com" type="cite"> <br>
Thanks,<br>
Stefan<br>
<br>
<div class="moz-cite-prefix">On 2015-08-13 19:50, Stefan Karlsson
wrote:<br>
</div>
<blockquote cite="mid:55CCD8D5.1070509@oracle.com" type="cite">
<meta content="text/html; charset=utf-8"
http-equiv="Content-Type">
<div class="moz-cite-prefix">Hi all,<br>
<br>
Could I get a second review for the patch below (plus the
suggested removal)?<br>
<br>
Thanks,<br>
StefanK<br>
<br>
On 2015-07-02 19:58, Stefan Karlsson wrote:<br>
</div>
<blockquote cite="mid:55957BCD.9020604@oracle.com" type="cite">
<meta content="text/html; charset=utf-8"
http-equiv="Content-Type">
<div class="moz-cite-prefix">On 2015-07-02 19:43, Jon
Masamitsu wrote:<br>
</div>
<blockquote cite="mid:5595784A.9050903@oracle.com" type="cite">
<meta content="text/html; charset=utf-8"
http-equiv="Content-Type">
<br>
<br>
<div class="moz-cite-prefix">On 6/29/2015 2:38 AM, Stefan
Karlsson wrote:<br>
</div>
<blockquote cite="mid:5591122F.2000704@oracle.com"
type="cite">
<meta content="text/html; charset=utf-8"
http-equiv="Content-Type">
Hi all,<br>
<br>
"8087322: Implement a Semaphore utility class" has now
been pushed, so I've updated the patch to reflect the
changes.<br>
<br>
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Estefank/8087324/webrev.01.delta">http://cr.openjdk.java.net/~stefank/8087324/webrev.01.delta</a><br>
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Estefank/8087324/webrev.01">http://cr.openjdk.java.net/~stefank/8087324/webrev.01</a><br>
</blockquote>
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Estefank/8087324/webrev.01/src/share/vm/gc/shared/workgroup.hpp.frames.html">http://cr.openjdk.java.net/~stefank/8087324/webrev.01/src/share/vm/gc/shared/workgroup.hpp.frames.html</a><br>
<br>
Are these used?<br>
<br>
<pre><span class="changed"> 194 void print_worker_started_task(AbstractGangTask* task, uint worker_id);</span>
<span class="changed"> 195 void print_worker_finished_task(AbstractGangTask* task, uint worker_id);
</span></pre>
</blockquote>
<br>
No. I'll remove them.<br>
<br>
<blockquote cite="mid:5595784A.9050903@oracle.com" type="cite">
<pre><span class="changed">
Rest looks good. Just one question (just for my education).
</span></pre>
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Estefank/8087324/webrev.01/src/share/vm/gc/shared/workgroup.cpp.frames.html">http://cr.openjdk.java.net/~stefank/8087324/webrev.01/src/share/vm/gc/shared/workgroup.cpp.frames.html</a><br>
<br>
<pre><span class="changed"> 336 void GangWorker::loop() {</span>
<span class="changed"> 337 while (true) {</span>
<span class="changed"> 338 WorkData data = wait_for_task();</span>
<span class="changed"> 339 </span>
<span class="changed"> 340 run_task(data);</span>
<span class="changed"> 341 </span>
<span class="changed"> 342 signal_task_done();</span>
343 }
344 }
Does this allow the same thread to execute more than 1
task ("data" here)? Meaning if 2 threads are requested but
1 thread is not scheduled to a cpu, will the other thread
do both chunks of work?</pre>
</blockquote>
<br>
Yes, that's correct.<br>
<br>
Thanks for reviewing!<br>
StefanK<br>
<br>
<blockquote cite="mid:5595784A.9050903@oracle.com" type="cite">
<pre>Jon
</pre>
<blockquote cite="mid:5591122F.2000704@oracle.com"
type="cite"> <br>
- The IMPLEMENTS_SEMAPHORE_CLASS define was removed, since
all platforms need to provide a Semaphore implementation.<br>
<br>
- Removed the need to pass down "max number of workers" to
the Semaphore constructor.<br>
<br>
- Updated semaphore.hpp include path<br>
<br>
Thanks,<br>
StefanK<br>
<br>
<br>
<div class="moz-cite-prefix">On 2015-06-12 16:52, Stefan
Karlsson wrote:<br>
</div>
<blockquote cite="mid:557AF21B.2090102@oracle.com"
type="cite">
<meta http-equiv="content-type" content="text/html;
charset=utf-8">
Hi all,<br>
<br>
The current implementation to distribute tasks to GC
worker threads often cause long latencies (multiple
milliseconds) when the threads are started and stopped.
<br>
<br>
The main reason is that the worker threads have to fight
over the Monitor lock when they are woken up from the
call to Monitor::wait. Another reason is that all worker
threads call notify_all when they finish a task and
there wakes all all sleeping worker threads, which will
yet again force the worker threads to fight over the
lock. <br>
<br>
I propose that we use semaphores instead, so that the
worker threads don't have to fight over a lock when they
are woken up.<br>
<br>
<br>
The patches build upon the following patch which
introduces a Semaphore utility class. This patch will
sent out for review on the hotspot-dev, since it affects
non-GC parts of the code:<br>
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Estefank/8087322/webrev.00/">http://cr.openjdk.java.net/~stefank/8087322/webrev.00/</a><br>
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="https://bugs.openjdk.java.net/browse/JDK-8087322">https://bugs.openjdk.java.net/browse/JDK-8087322</a><br>
<br>
<br>
The first patch that I would like to get reviewed is:<br>
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Estefank/8087323/webrev.00/">http://cr.openjdk.java.net/~stefank/8087323/webrev.00/</a><br>
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="https://bugs.openjdk.java.net/browse/JDK-8087323">https://bugs.openjdk.java.net/browse/JDK-8087323</a>
- Unify and split the work gang classes <br>
<br>
It prepares for JDK-8087324, by separating the generic
WorkGang implementation from the more elaborate
YieldingFlexibleWorkGang (CMS) implementation. By having
this part as a separate patch, I hope it will be easier
to review JDK-8087324. The patch changes the work gang
inheritance from:<br>
<br>
AbstractWorkGang<br>
WorkGang<br>
FlexibleWorkGang<br>
YieldingFlexibleWorkGang<br>
<br>
to:<br>
<br>
AbstractWorkGang<br>
WorkGang<br>
YieldingFlexibleWorkGang<br>
<br>
Parts of the FlexibleWorkGang and WorkGang code that is
going to be used by both concrete work gang classes, has
been moved into AbstractWorkGang. I've duplicated some
code in WorkGang and YieldingFlexibleWorkGang, but that
code will be removed from WorkGang in the following
patch.<br>
<br>
<br>
The second patch I'd like to get reviewed is:<br>
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Estefank/8087324/webrev.00/">http://cr.openjdk.java.net/~stefank/8087324/webrev.00/</a><br>
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="https://bugs.openjdk.java.net/browse/JDK-8087324">https://bugs.openjdk.java.net/browse/JDK-8087324</a>
- Use semaphores when starting and stopping GC task
threads <br>
<br>
It first simplifies the way we distribute the tasks to
the GC worker threads. For example, the coordinator
thread dispatches a task to a specific number of
workers, and then waits for all work to be completed.
There's no risk that multiple tasks will be scheduled
simultaneously, so there's no need for the sequences
number that is used in the current implementation.<br>
<br>
The patch contains two task dispatch / thread
synchronization implementations:<br>
<br>
The first implementation uses Monitors, similar to what
we did before the patch, but with a slightly lower
overhead since the code calls notify_all less often. It
still suffers from the "thundering heard" problem. When
the coordinator thread signals that the worker threads
should start, they all wake up from Monitor::wait and
they all try to lock the Monitor.<br>
<br>
The second, and the more interesting, implementation
uses semaphores. When the worker threads wake up from
the semaphore wait, they don't have to serialize the
execution by taking a lock. This greatly decreases the
time it takes to start and stop the worker threads.<br>
<br>
The semaphore implementation is used on all platforms
where the Semaphore class has been implemented in
JDK-8087322. So, on some OS:es the code will revert to
the Monitor-based solution until a Semaphore class has
been implemented for that OS. So, porters might want to
consider implementing the Sempahore class.<br>
<br>
There's also a diagnostic vm option
(-XX:+/-UseSemaphoreGCThreadsSynchronization) to turn
off the Semaphore-based implementation, which can be
used to debug this new code. It's mainly targeted
towards support and sustaining engineering.<br>
<br>
<br>
The patches have been performance tested on Linux,
Solaris, OSX, and Windows.<br>
<br>
The effects of the patch can be seen by running
benchmarks with small young gen sizes, which triggers
frequent and short GCs.<br>
<br>
For example, here are runs from the SPECjvm2008
xml.transform benchmark with:<br>
-Xmx1g -Xms1g -Xmn64m -XX:+PrintGC -XX:+UseG1GC -jar
SPECjvm2008.jar -ikv xml.transform -it 30 -wt 30<br>
<br>
I got the following GC times:<br>
<br>
<tt> Average Median 99.9 percentile
Max</tt><tt><br>
</tt><tt>Baseline: </tt>
<meta http-equiv="content-type" content="text/html;
charset=utf-8">
<title></title>
<meta name="generator" content="LibreOffice 4.4.0.3
(Linux)">
<style type="text/css">
body,div,table,thead,tbody,tfoot,tr,th,td,p { font-family:"Liberation Sans"; font-size:x-small }
</style><tt> </tt>
<meta http-equiv="content-type" content="text/html;
charset=utf-8">
<title></title>
<meta name="generator" content="LibreOffice 4.4.0.3
(Linux)">
<style type="text/css">
body,div,table,thead,tbody,tfoot,tr,th,td,p { font-family:"Liberation Sans"; font-size:x-small } </style><tt>8.76</tt><tt>
ms 8.44 ms 25.9 ms </tt><tt>3</tt><tt>4.7
ms</tt><tt><br>
</tt><tt>Monitor:</tt><tt> 6.1</tt><tt>7 </tt><tt>ms
</tt><tt>5.88 ms 26.0 ms </tt><tt>49.1 ms</tt><tt><br>
</tt><tt> </tt>
<meta http-equiv="content-type" content="text/html;
charset=utf-8">
<title></title>
<meta name="generator" content="LibreOffice 4.4.0.3
(Linux)">
<style type="text/css">
body,div,table,thead,tbody,tfoot,tr,th,td,p { font-family:"Liberation Sans"; font-size:x-small }
</style><tt>Semaphore: </tt><tt>3.</tt><tt>43 ms </tt><tt>3.26 </tt><tt>ms
13.4 ms 33.4 ms</tt><tt><br>
</tt><br>
If I run an empty GC task 10 times per GC, by running
the following code:<br>
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Estefank/8087324/timedTask/">http://cr.openjdk.java.net/~stefank/8087324/timedTask/</a><br>
<br>
I get the following numbers to complete the empty GC
tasks:<br>
<br>
<tt> Average Median 99.9 percentile
Max</tt><tt><br>
</tt><tt>Baseline: </tt><tt>1.43 ms 0.92 ms 3.43
ms 9.30</tt><tt> ms</tt><tt><br>
Monitor</tt><tt>:</tt><tt> 0.75</tt><tt> </tt><tt>ms
</tt><tt>0.72 ms 1.74 ms 2.78</tt><tt> ms</tt><tt><br>
</tt><tt> </tt><tt>Semaphore: </tt><tt>0.</tt><tt>07
ms </tt><tt>0.07 </tt><tt>ms 0.17 ms
0.26 ms</tt><tt><br>
</tt><br>
<br>
<br>
The code has been tested with JPRT and our nightly
testing suites. <br>
<br>
I've created a unit test to run a small test with both
the semaphore implementation and the monitor
implementation:<br>
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Estefank/8087324/workgangTest/">http://cr.openjdk.java.net/~stefank/8087324/workgangTest/</a><br>
<br>
But since we currently don't have code to shutdown
worker threads after they have been started, I don't
want to push this test (or clean it up) until we have
that in place. I created this bug for that:<br>
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="https://bugs.openjdk.java.net/browse/JDK-8087340">https://bugs.openjdk.java.net/browse/JDK-8087340</a><br>
<br>
Thanks,<br>
StefanK<br>
<br>
<br>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</body>
</html>