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?

============================================================================== 
g1/satbQueue.cpp
 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.

============================================================================== 
g1/satbQueue.hpp
 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.

==============================================================================
memory/freeList.hpp
memory/freeList.cpp
assert_proper_lock_protection()

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
reverted. 

============================================================================== 
memory/sharedHeap.cpp
  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.

==============================================================================
gc_interface/collectedHeap.hpp
[not in current webrev]

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

============================================================================== 



More information about the hotspot-gc-dev mailing list