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

[Thomas Schatzl]

Hi Jon,

On Tue, 2016-04-26 at 22:06 -0700, 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.

  I only looked briefly at the change, but looks good so far.

Some questions:

- wouldn't it be useful (as part of a different change maybe) to always
dynamically create the worker threads? I mean, always start up with a
single gc thread and take the hit on the first time the threads are
used instead? (Or maybe just make UseDynamicNumberOfGCThreads=true
default).

- in AdaptiveSizePolicy::initial_numberOf_workers(), why is the lower
bound in line 358 two? I.e.

initial_workers = MIN2(2U, ParallelGCThreads);

- the indentation for the parameter list for
AdaptiveSizePolicy::add_workers() is always wrong :)

- the changeset does not seem to apply cleanly to the latest hs tree.

- I would prefer if the dependencies on AdaptiveSizePolicy to WorkGang
(at least) were minimized. The initial_number_of_workers() result could
be passed in to AbstractWorkGang::initialize_workers() instead of
adding the dependency.

- the AdaptiveSizePolicy::add_workers() method seems to be completely
unrelated to that class. I.e. instead of doing policy stuff, it
actually does "real work".
I would prefer if that method were put into a separate class in the
shared directory.

- the use of DisableStartThread in add_workers() seems odd. From the
description it only seems to be applicable to Java threads, not native
threads we are creating here.

- that is kind of my personal preference, but I would prefer if the
interface to add_workers were a bit simpler - mainly not having that
many inout parameters. Like "creates X new threads for the given
holder, starting from index Y and returns the number of created
threads."

Then again, you would have some code duplication in the callers.

Will continue looking.

Thanks,
  Thomas