Request for Review (M) - 6858051: Create GC worker threads dynamically

[Jon Masamitsu]

Derek,

Thanks for the review.  Responses in-line.

On 05/04/2016 02:45 PM, Derek White wrote:
> Hi Jon,
>
> On 4/28/16 6:40 PM, Jon Masamitsu wrote:
>> Changes:
>>
>> In order to reduce the dependency of WorkGang on AdaptiveSizePolicy,
>> removed AdaptiveSizePolicy::initial_number_of_workers() and 
>> duplicated the code
>> in GCTaskManager::initialize() and 
>> AbstractWorkGang::initialize_workers().
>>
>> Moved AdaptiveSizePolicy::add_workers() to 
>> WorkerManager::add_workers() in
>> file shared/workerManager.hpp
>>
>> Removed guard DisableStartThread from add_workers() and always start the
>> workers.
>>
>> Removed the bool return value from WorkerManager::add_workers (and 
>> let that rippled down
>> to the callers).
>>
>> Changed WorkerManager::add_workers() to return the number of created 
>> workers
>> (removed inout on parameters) and let the callers of 
>> WorkerManager::add_workers()
>> adjust their _active_workers to be consistent with the number of 
>> created workers.
>>
>> Changed number of workers created at initialization from 2 to 1.
>>
>> Please let me know if I forgot anything.
>>
>> Delta: http://cr.openjdk.java.net/~jmasa/6858051/webrev_delta_01_02/
>> Full:http://cr.openjdk.java.net/~jmasa/6858051/webrev.02/
>>
>> Thanks.
>>
>> Jon
>>
>>
>>
>> On 04/26/2016 10:06 PM, Jon Masamitsu wrote:
>>> 6858051: Create GC worker threads dynamically
>>> https://bugs.openjdk.java.net/browse/JDK-6858051
>>>
>>> This change creates a minimal number of GC workers at JVM 
>>> initialization
>>> and then adds additional workers as they are needed.  This has been 
>>> made part of the
>>> UseDynamicNumberOfGCThreads feature.  When a parallel task is about to
>>> be executed, the number of workers needed to execute the task is 
>>> calculated (call
>>> that number N).  If that number of workers already exist, then no 
>>> additional workers
>>> are created.  If fewer than N exists, then additional workers are 
>>> created.  Workers
>>> are not destroyed if not needed for the task.
>>>
>>> The UseParallelGC way of executing parallel tasks is different from 
>>> the other
>>> collectors so it has a somewhat separate implementation.
>>>
>>> http://cr.openjdk.java.net/~jmasa/6858051/webrev.00/
>>>
>>> Testing: gc_test_suite with and without UseDynamicNumberOfGCThreads.
>>> Performance testing with the prototype did not show any regression with
>>> UseDynamicNumberOfGCThreads turned on.     RBT testing for hotspot_gc
>>> is in progress.
>>>
>>> Thanks.
>>>
>>> Jon
>
> Great change!
> I have a few comments, mostly minor. The last one /may/ be more serious.
>
> ------------------------------
> gc/shared/workgroup.cpp
>
>  - Line 38, update comment about 
> AbstractWorkGang::initialize_workers() returning a boolean.

fixed
>
> ------------------------------
> gc/shared/workerManager.hpp
>
>  - Line 33: "dynamic_workers" -> "active_workers"
>    Lines 38, 39, similarly
fixed

>
> ------------------------------
> gc/shared/adaptiveSizePolicy.hpp
>
> Line 34, 42, 43:
>  - Are these changes still needed?

Probably not.  I'll remove them and rebuild to check.

>
> Line 355: Does anyone call initial_number_of_workers()?

No!  Removed.

>
> ------------------------------
> gc/parallel/gcTaskThread.hpp
>
>  Line 69: I think start() is no longer called?
>    - WorkerManager::add_workers() calls os::start_thread() directly.

Removed.

>
> ------------------------------
> gc/parallel/gcTaskThread.cpp
>
>  Line 57: I think start() is no longer called?
>    - WorkerManager::add_workers() calls os::start_thread() directly.

Removed.

>
> ------------------------------
> gc/parallel/gcTaskManager.hpp
>  Line 1: Copyright.

fixed

>
> ------------------------------
> gc/parallel/gcTaskManager.cpp
>
> Line 430: The threads get started here now, not at the end of
> initialize(). Is there any danger due to threads starting before:
>  443   reset_busy_workers();
>  444   set_unblocked();
>  445   for (uint w = 0; w < workers(); w += 1) {
>  446     set_resource_flag(w, false);
>  447   }
>  448   reset_delivered_tasks();
>  449   reset_completed_tasks();
>  450   reset_barriers();
>  451   reset_emptied_queue();

Let me look into this a bit.  It does look suspicious.  I'll let you 
know what I think
when I post an updated webrev.

Jon
>
>
>  - Derek

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20160504/608d96f9/attachment.htm>