RFR: 6979279: remove special-case code for ParallelGCThreads==0

Kim Barrett kim.barrett at oracle.com
Thu Oct 16 17:32:52 UTC 2014

On Oct 15, 2014, at 8:31 AM, Marcus Larsson <marcus.larsson at oracle.com> wrote:
> Sorry for the noise. Forgot to fix some additional cases where checks if ParallelGCThreads > 0 could be removed.
> New webrev:
> http://cr.openjdk.java.net/~mlarsson/6979279/webrev.03/
> Incremental:
> http://cr.openjdk.java.net/~mlarsson/6979279/webrev.02-03/

g1/g1HardCardCache.hpp - no changes?

 277 void SATBMarkQueueSet::set_closure(int i, ObjectClosure* closure) {
 278   assert(_closures != NULL, "Precondition");
 279   _closures[i] = closure;

Perhaps add an assert that i is in proper range?  That seems like a
more important check.

Also, wouldn't it be better if i were size_t rather than int?  Since
you are changing the signature anyway.

 108   // Register "blk" as "the closure" for all queues.  Only one such closure
 109   // is allowed.  The "apply_closure_to_completed_buffer" method will apply
 110   // this closure to a completed buffer, and "iterate_closure_all_threads"
 111   // applies it to partially-filled buffers (the latter should only be done
 112   // with the world stopped).
 113   void set_closure(int i, ObjectClosure* closure);

I see no "blk" in the signature.  [Pre-existing problem.]

The emphasis on "the closure" and "only one such closure" is left over
from the non-parallel version, and seems like it should be updated.


New implementation always involves a function call (to an empty
implementation #ifndef ASSERT), rather than being optimizable to
nothing #ifdef PRODUCT.  So I think the part of the change to this
function around the use of PRODUCT/ASSERT is incorrect, and should be

  71   if ((UseParNewGC ||
  72       (UseConcMarkSweepGC && (CMSParallelInitialMarkEnabled ||
  73                               CMSParallelRemarkEnabled)) ||
  74        UseG1GC) &&
  75       use_parallel_gc_threads()) {

Seems like we shouldn't need call to use_parallel_gc_threads() here.

[not in current webrev]

Do we still need CollectedHeap::use_parallel_gc_threads() at all?


More information about the hotspot-gc-dev mailing list