RFR (XS): 8033443: Test8000311 fails after latest changes to parallelize string and symbol table unlink

[Thomas Schatzl]

Hi,

On Tue, 2014-02-04 at 17:48 +0100, Bengt Rutisson wrote:
> 
> Hi Thomas,
> 
> This looks good to me.
> 
> One minor thing that I leave up to you to fix if you want to.
> 
I would like to keep it for now - see below.

> It seems more natural to me to pass an "is parallel" value to the
> constructor of G1StringSymbolTableUnlinkTask rather than have it look
> at G1CollectedHeap::use_parallel_gc_threads() itself to set the
> _do_in_parallel value.

Then you did not look too closely on implementations of AbstractGangTask
yet ;)

Typical pattern:

MyTask x(...);

void x::work() {
  if (G1CollectedHeap::use_parallel_gc_threads()) {
    ...
  } else {
    ...
  }
}

num_workers =
calculate_number_of_workers(G1CollectedHeap::use_parallel_gc_threads())

if (G1CollectedHeap::use_parallel_gc_threads()) {
   g1h->set_par_threads();  //
   workers()->run_task(&g1_par_task);
   g1h->set_par_threads(0); //
} else {
   g1_par_task.set_for_termination(n_workers); //
   g1_par_task.work(0);
}

(Line marked with // are purely optional it seems ;) (they are not - and
sometimes you need another call to workgang()->set_n_workers() or so).

I think it would be more natural that the AbstractGangTask provided this
information in whatever form instead of the programmer taking care of
that every time...

I.e. either there are different methods to override (e.g. the parallel
one just defaults to the serial one), or the task dispatch sets a flag
that can be queried.

Also there could be some method that contains the boiler-plate code,
i.e. given

MyTask x(...);
num_workers = ...

[bool ran_in_parallel =] possibly_run_parallel(AbstractGangTask* task,
uint num_workers);

(the bool result because some code needs to know whether we actually
performed the code in parallel for eg. asserts).

where possibly_run_parallel() or whatever is is called looks as follows:

bool G1CollectedHeap::possibly_run_parallel(AbstractGangTask* task, uint
num_workers) {
if (num_workers > 1) {
   [...]
   workers()->run_task(&task);
   [...]
} else {
   g1_par_task.work(0);
}

What do you think?

Thomas