<html>
<head>
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<font face="Times New Roman, Times, serif">Derek,<br>
<br>
Thanks for the review. Responses in-line.<br>
</font><br>
<div class="moz-cite-prefix">On 05/04/2016 02:45 PM, Derek White
wrote:<br>
</div>
<blockquote cite="mid:572A6D78.1090804@oracle.com" type="cite">
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
<div class="moz-cite-prefix">Hi Jon,<br>
<br>
On 4/28/16 6:40 PM, Jon Masamitsu wrote:<br>
</div>
<blockquote cite="mid:57229179.9080606@oracle.com" type="cite">Changes:
<br>
<br>
In order to reduce the dependency of WorkGang on
AdaptiveSizePolicy, <br>
removed AdaptiveSizePolicy::initial_number_of_workers() and
duplicated the code <br>
in GCTaskManager::initialize() and
AbstractWorkGang::initialize_workers(). <br>
<br>
Moved AdaptiveSizePolicy::add_workers() to
WorkerManager::add_workers() in <br>
file shared/workerManager.hpp <br>
<br>
Removed guard DisableStartThread from add_workers() and always
start the <br>
workers. <br>
<br>
Removed the bool return value from WorkerManager::add_workers
(and let that rippled down <br>
to the callers). <br>
<br>
Changed WorkerManager::add_workers() to return the number of
created workers <br>
(removed inout on parameters) and let the callers of
WorkerManager::add_workers() <br>
adjust their _active_workers to be consistent with the number of
created workers. <br>
<br>
Changed number of workers created at initialization from 2 to 1.
<br>
<br>
Please let me know if I forgot anything. <br>
<br>
Delta: <a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Ejmasa/6858051/webrev_delta_01_02/">http://cr.openjdk.java.net/~jmasa/6858051/webrev_delta_01_02/</a>
<br>
Full:<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Ejmasa/6858051/webrev.02/">http://cr.openjdk.java.net/~jmasa/6858051/webrev.02/</a>
<br>
<br>
Thanks. <br>
<br>
Jon <br>
<br>
<br>
<br>
On 04/26/2016 10:06 PM, Jon Masamitsu wrote: <br>
<blockquote type="cite">6858051: Create GC worker threads
dynamically <br>
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="https://bugs.openjdk.java.net/browse/JDK-6858051">https://bugs.openjdk.java.net/browse/JDK-6858051</a>
<br>
<br>
This change creates a minimal number of GC workers at JVM
initialization <br>
and then adds additional workers as they are needed. This has
been made part of the <br>
UseDynamicNumberOfGCThreads feature. When a parallel task is
about to <br>
be executed, the number of workers needed to execute the task
is calculated (call <br>
that number N). If that number of workers already exist, then
no additional workers <br>
are created. If fewer than N exists, then additional workers
are created. Workers <br>
are not destroyed if not needed for the task. <br>
<br>
The UseParallelGC way of executing parallel tasks is different
from the other <br>
collectors so it has a somewhat separate implementation. <br>
<br>
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Ejmasa/6858051/webrev.00/">http://cr.openjdk.java.net/~jmasa/6858051/webrev.00/</a>
<br>
<br>
Testing: gc_test_suite with and without
UseDynamicNumberOfGCThreads. <br>
Performance testing with the prototype did not show any
regression with <br>
UseDynamicNumberOfGCThreads turned on. RBT testing for
hotspot_gc <br>
is in progress. <br>
<br>
Thanks. <br>
<br>
Jon <br>
</blockquote>
</blockquote>
<br>
Great change!<br>
I have a few comments, mostly minor. The last one <i>may</i> be
more serious.<br>
<br>
------------------------------<br>
gc/shared/workgroup.cpp<br>
<br>
- Line 38, update comment about
AbstractWorkGang::initialize_workers() returning a boolean.<br>
</blockquote>
<br>
fixed<br>
<blockquote cite="mid:572A6D78.1090804@oracle.com" type="cite"> <br>
------------------------------<br>
gc/shared/workerManager.hpp<br>
<br>
- Line 33: "dynamic_workers" -> "active_workers"<br>
Lines 38, 39, similarly<br>
</blockquote>
fixed<br>
<br>
<blockquote cite="mid:572A6D78.1090804@oracle.com" type="cite"> <br>
------------------------------<br>
gc/shared/adaptiveSizePolicy.hpp<br>
<br>
Line 34, 42, 43:<br>
- Are these changes still needed?<br>
</blockquote>
<br>
Probably not. I'll remove them and rebuild to check.<br>
<br>
<blockquote cite="mid:572A6D78.1090804@oracle.com" type="cite"> <br>
Line 355: Does anyone call initial_number_of_workers()?<br>
</blockquote>
<br>
No! Removed.<br>
<br>
<blockquote cite="mid:572A6D78.1090804@oracle.com" type="cite"> <br>
------------------------------<br>
gc/parallel/gcTaskThread.hpp<br>
<br>
Line 69: I think start() is no longer called?<br>
- WorkerManager::add_workers() calls os::start_thread()
directly. <br>
</blockquote>
<br>
Removed.<br>
<br>
<blockquote cite="mid:572A6D78.1090804@oracle.com" type="cite"> <br>
------------------------------<br>
gc/parallel/gcTaskThread.cpp<br>
<br>
Line 57: I think start() is no longer called?<br>
- WorkerManager::add_workers() calls os::start_thread()
directly.<br>
</blockquote>
<br>
Removed.<br>
<br>
<blockquote cite="mid:572A6D78.1090804@oracle.com" type="cite"> <br>
------------------------------<br>
gc/parallel/gcTaskManager.hpp<br>
Line 1: Copyright.<br>
</blockquote>
<br>
fixed<br>
<br>
<blockquote cite="mid:572A6D78.1090804@oracle.com" type="cite"> <br>
------------------------------<br>
gc/parallel/gcTaskManager.cpp<br>
<br>
Line 430: The threads get started here now, not at the end of<br>
initialize(). Is there any danger due to threads starting before:<br>
443 reset_busy_workers();<br>
444 set_unblocked();<br>
445 for (uint w = 0; w < workers(); w += 1) {<br>
446 set_resource_flag(w, false);<br>
447 }<br>
448 reset_delivered_tasks();<br>
449 reset_completed_tasks();<br>
450 reset_barriers();<br>
451 reset_emptied_queue();<br>
</blockquote>
<br>
Let me look into this a bit. It does look suspicious. I'll let you
know what I think<br>
when I post an updated webrev.<br>
<br>
Jon<br>
<blockquote cite="mid:572A6D78.1090804@oracle.com" type="cite"> <br>
<br>
- Derek<br>
</blockquote>
<br>
</body>
</html>