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

[Derek White]

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();


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