Request for Review (xs) - 8159073: Error handling incomplete when creating GC threads lazily

[Thomas Schatzl]

Hi Jon,

On Tue, 2016-07-19 at 10:27 -0700, Jon Masamitsu wrote:
> 
> On 7/19/2016 2:03 AM, Thomas Schatzl wrote:
> > 
> 
[...]
> > 
> > 
> > > 
> > > > 
> > > > Further, if we consider doing that, I would prefer to have that
> > > > behavior centralized in the run_task() methods. Not have to
> > > > consider every time we create and run a gang task whether it
> > > > might
> > > > be useful to run it single-threaded (and add the boiler plate
> > > > code
> > > > to do it everywhere).
> > > I can see that point although I don't think the centralized point
> > > should be run_task().
> > Sure, this is open for discussion. Just a suggestion.
> OK.  No change here either for this patch.

Thanks.

[...]
> > > 
> > Yes, the caller limits the number of threads. Which means that the
> > caller (hopefully) properly prepares the AbstractGangTask to work
> > with that number of threads :)
> > 
> > The problem is the run_task(, uint) method potentially further
> > restricting the number of "workers". For that one the caller might
> > not have prepared the AbstractGangTask for.
> It seems to me that we don't want the caller of run_task() to have to
> know that not all the threads could be created (i.e., that
> _created_threads < _available_threads). 

Yes.

> As you say, some central authority knows that.
> Here the central place is the AbstractWorkGang.  That seems the right
> place to me.  The STW parallel WorkGang's and the concurrent parallel
> WorkGang's have a different number of threads that they wants to
> use.  It could be argued that the task (AbstractGangTask) know more
> about the work than the work gang so should decide how many
> threads to use, but there are/will be more global constraints on the
> number of parallel threads than the work to be done by the task.
> When the JVM becomes aware of the resource usage on the platform,
> it may want to limit the number of threads to use (so as not to
> hog the CPU).
> 

As mentioned before, when calling run_task(, uint), or set the
active_workers(), the caller actually specifies the amount of work
(e.g. number of work chunks) to be performed. Not necessarily the
number of threads (of course, in the normal case, it kind of expects,
but not requires that).

I.e. the AbstractGangTask may expect that its do_work() method is
actually called that number of times, independent of the number of
actual threads used.

If the WorkGang changed that, we might have to deal with the caller not
handling that well... I am sure that G1 does not care, but maybe CMS
does not like that (or the parallel reference processing).

I would like to avoid changing these semantics.

> If you still want the patch you proposed to save and restore
> available threads, I'll accept it as part of this change.

I would prefer if the run_task(, uint) method were simply a shortcut
for saving, setting and restoring the active "workers".

I see that proposed change too much of a risk at this time.

> > > > [...]
> > > > Overall, and as a future improvement, I think it might be
> > > > better to
> > > > move the management of "global" active_workers variable out
> > > > from
> > > > WorkGang. And maybe rename the parameters more appropriately.
> > > > ;)
> > > In my mind WorkGang is a place to store the number of active
> > > workers
> > > (and now created workers).  WorkGang knows the maximum number of
> > > workers that it can use so active_workers seemed to fit.  Are you
> > > suggesting removing the run_task(task) method and only have
> > > run_task(task, num_workers) so that
> > > the number of active workers is always feed to WorkGang?  I would
> > > not
> > > be opposed to that.
> > > 
> > Yes, that is the idea - as some future change.
> Ok.  Future change.
> 
> Thanks for the interesting discussion.  I think the only change
> still in question is the save/restore of available threads.  Let
> me know what you want me to do with that.  Also, let me
> know if there is something I've missed.

Just some other note: I think, in the future we will want some kind of
better WorkGang that gives more guarantees about how many and which
threads are run doing what work (e.g. for NUMA).

Again, I think some discussion for another day.

Thanks,
  Thomas