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

[Thomas Schatzl]

Hi again,

On Wed, 2016-04-27 at 13:10 +0200, Thomas Schatzl wrote:
> 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.
> 

Maybe rename the "initializing" parameter of add_workers() to
"exit_on_failure", as this would concisely describe what it does
without a long explanation.

Otherwise, in workgroup.hpp:166, the comment
" 163     add_workers(false /* exit_on_failure */);" should be adapted.

Thanks,
  Thomas