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

[Thomas Schatzl]

Hi,

On Wed, 2016-04-27 at 09:04 -0700, Jon Masamitsu wrote:
> Thomas,
> 
> Thanks for looking at this.
> 
> On 04/27/2016 04:10 AM, 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).
> To preserve current behavior I'd would not always create the workers
> on demand.  For one thing if we can't create the workers at
> initialization, I think we exit.  That might be preferable to some
> people as opposed to limping along with a few workers.  Also the
> first GC would take a hit and we'd probably have to implement pre-
> touch like flag CreateGCWorkersAtStartUp.
> 
> I'd prefer to get to the point where we can turn
> UseDynamicNumberOfGCThreads
> on by default.
> > 
> > 
> > - in AdaptiveSizePolicy::initial_numberOf_workers(), why is the
> > lower bound in line 358 two? I.e.
> > 
> > initial_workers = MIN2(2U, ParallelGCThreads);
> That's the minimum I thought would make sense.   The amount of used
> in the heap (one metric for deciding how many GC threads to use) is
> at a value that doesn't reflect much about the application so is not
> useful.  I could start with something else if there is data that will
> point me in the right direction.

I would start up with one thread.

> > - the indentation for the parameter list for
> > AdaptiveSizePolicy::add_workers() is always wrong :)
>    return AdaptiveSizePolicy::add_workers(this,
> 
>                                           _active_workers,
> 
>                                           (uint) _workers,
> 
>                                           _created_workers,
> 
>                                           worker_type,
> 
>                                           initializing);
> 
> 
> What's the rule now?  All on one line?

No, the intention has been fine, aligned right below the first
parameter.

In the webrev and the code I applied the patch the parameters (in the
hpp file) are aligned below the "e" of add_workers.

It's odd because in the actual patch it is fine. Ignore this then,
sorry for the noise.

> > - the changeset does not seem to apply cleanly to the latest hs
> > tree.
> I woke up in the middle of the night and realized that :-).  I fixed
> it earlier today.
> 
> > 
> > 
> > - 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.
> Ok.  I'll make that change.
> 
> > 
> > 
> > - 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.
> Ok.
> > 
> > 
> > - 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.
> What you say is true and that might be a bug.  I use it in
> add_workers() to get the same effect as with the original code where
> all the workers are started at initialization.  I probably should
> guard its use under the "initializing" flag.   Would it be Ok if I
> remove its use for GC threads in a separate change?

That's fine with me.

> > - 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.
> I'll make some changes and see if I get what you're saying.

That's just a suggestion. If you do not like it, ignore this comment.

Thomas