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

[Jon Masamitsu]

Testing with trace level logging found another bug.
The logging tried to print information for GCTaskThreads
that have not been created yet.

Delta
http://cr.openjdk.java.net/~jmasa/6858051/webrev_delta_03_04/

Complete
http://cr.openjdk.java.net/~jmasa/6858051/webrev.04/

Thanks.

Jon

On 5/5/2016 11:17 AM, Jon Masamitsu wrote:
> Derek,
>
> New webrev and webrev_delta.  One comment in-line.
>
> http://cr.openjdk.java.net/~jmasa/6858051/webrev.03/
> http://cr.openjdk.java.net/~jmasa/6858051/webrev_delta_02_03/
>
> Thanks.
>
> 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.
>>
>> ------------------------------
>> gc/shared/workerManager.hpp
>>
>>  - Line 33: "dynamic_workers" -> "active_workers"
>>    Lines 38, 39, similarly
>>
>> ------------------------------
>> gc/shared/adaptiveSizePolicy.hpp
>>
>> Line 34, 42, 43:
>>  - Are these changes still needed?
>>
>> Line 355: Does anyone call initial_number_of_workers()?
>>
>> ------------------------------
>> gc/parallel/gcTaskThread.hpp
>>
>>  Line 69: I think start() is no longer called?
>>    - WorkerManager::add_workers() calls os::start_thread() directly.
>>
>> ------------------------------
>> gc/parallel/gcTaskThread.cpp
>>
>>  Line 57: I think start() is no longer called?
>>    - WorkerManager::add_workers() calls os::start_thread() directly.
>>
>> ------------------------------
>> gc/parallel/gcTaskManager.hpp
>>  Line 1: Copyright.
>>
>> ------------------------------
>> 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();
>
> The set_resources_flag() call at least should precede the starting
> of the threads.  Probably also the set_unblocked().   I don't think
> anything would crash.  No threads would try to do work until after 
> tasks are
> added to the work queue, but there would likely have been some
> unexpected thread activity at startup that would have been confusing.
> In general that block of code should be executed before starting
> threads.
>
> Jon
>>
>>
>>  - Derek
>

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