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

[Thomas Schatzl]

Hi,

On Fri, 2016-07-15 at 09:48 -0700, Jon Masamitsu wrote:
> Thomas,
> 
> What do you think of the rational for the VM executing the
> task?
> 
> "share/vm/gc/cms/concurrentMarkSweepGeneration.cpp"
> 
> 4888   // It turns out that even when we're using 1 thread, doing the
> work in a
> 4889   // separate thread causes wide variance in run ti
> mes.  We can't help this
> 4890   // in the multi-threaded case, but we special-case n=1 here to
> get
> 4891   // repeatable measurements of the 1-thread overhead of the 
> parallel code.
> 4892   if (n_workers > 1) {
> 4893     // Make refs discovery MT-safe, if it isn't already: it may
> not
> 4894     // necessarily be so, since it's possible that we are doing
> 4895     // ST marking.
> 4896     ReferenceProcessorMTDiscoveryMutator mt(ref_processor(),
> true);
> 4897     workers->run_task(&tsk);
> 4898   } else {
> 4899     ReferenceProcessorMTDiscoveryMutator mt(ref_processor(),
> false);
> 4900     tsk.work(0);
> 4901   }
> 
> Worth keeping the special case?  A similar comment was made for
> ParNewGenTask.  I'm experimenting with removing the special cases.
> 
> src/share/vm/gc/cms/parNewGeneration.cpp
> 
>       ParNewGenTask tsk(this, _old_gen, reserved().end(), 
> &thread_state_set, &srs);
>       gch->rem_set()->prepare_for_younger_refs_iterate(true);
> -    // It turns out that even when we're using 1 thread, doing the
> work 
> in a
> -    // separate thread causes wide variance in run times.  We can't 
> help this
> -    // in the multi-threaded case, but we special-case n=1 here to
> get
> -    // repeatable measurements of the 1-thread overhead of the
> parallel 
> code.
> -    // Might multiple workers ever be used?  If yes, initialization
> -    // has been done such that the single threaded path should not
> be used.
> -    if (workers->total_workers() > 1) {
>         workers->run_task(&tsk);
> -    } else {
> -      tsk.work(0);
> -    }
>     }

  I understand the rationale, and it may actually be still worth doing
it. I do not know, as the synchronization mechanism changed.

The question to me, is kind of: can't we make that optimization a
little more generic (i.e. put it into a lower level, e.g. run-task()
directly) so that all phases can benefit from it?

Further, some code is also shared by serial gc like the reference
processing one, and for that one we might need some special casing
somewhere anyway.  

(In a different CR of course).

Thanks,
  Thomas