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

[Derek White]

Hi Jon,

Diff looks good!

  - Derek

On 5/6/16 3:15 PM, Jon Masamitsu wrote:
> 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/4a1f5a24/attachment.htm>