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

[Jon Masamitsu]

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/20160505/749a1f30/attachment.htm>