RFR(M): 8077413: Avoid use of Universe::heap() inside collectors

[Kim Barrett]

On Apr 10, 2015, at 10:45 AM, Per Liden <per.liden at oracle.com> wrote:
> 
> Hi,
> 
> This patch cleans up the usage of Universe::heap() in our collectors. A typical pattern we have is code like this:
> 
>  ParallelScavengeHeap* heap = (ParallelScavengeHeap*)Universe::heap();
>  assert(heap->kind() == CollectedHeap::ParallelScavengeHeap, "Sanity");
> 
> This patch changes this to:
> 
>  ParallelScavengeHeap* heap = ParallelScavengeHeap::heap();
> 
> There are also other places where Universe::heap() is used, even when it's clear what we are inside, e.g. ParallelScavenge-related code. We should be using ParallelScavengeHeap::heap() here instead. Universe::heap() should (for clarity) only be used by code that doesn't know/care which collector we are currently using.
> 
> Bug: https://bugs.openjdk.java.net/browse/JDK-8077413
> 
> Complete webrev:
> http://cr.openjdk.java.net/~pliden/8077413/webrev.0/
> 
> If someone finds it easier to review in smaller chunks, I've split it in three parts.
> 
> Changes made to G1:
> http://cr.openjdk.java.net/~pliden/8077413/webrev.0-g1_cleanup/
> 
> Changes made to GenCollectedHeap-type collectors:
> http://cr.openjdk.java.net/~pliden/8077413/webrev.0-gencollected_heap_cleanup/
> 
> Changes made to ParallelScavenge:
> http://cr.openjdk.java.net/~pliden/8077413/webrev.0-parallelscavenge_cleanup/
> 
> 
> I have two more cleanups in this area, which will be sent out separately. These are:
> JDK-8077415 : Remove duplicate variables holding the CollectedHeap
> JDK-8077417 : Cleanup of Universe::initialize_heap()
> 
> cheers,
> /Per

------------------------------------------------------------------------------ 
src/share/vm/gc_implementation/concurrentMarkSweep/vmCMSOperations.cpp 
src/share/vm/gc_implementation/g1/g1OopClosures.inline.hpp
src/share/vm/gc_implementation/g1/g1ParScanThreadState.inline.hpp
src/share/vm/gc_implementation/parallelScavenge/psPromotionLAB.cpp

Copyright.  I may have forgotten to note some others.

------------------------------------------------------------------------------ 

I spent some time thinking about this, from the description of the
change set:

  There are also other places where Universe::heap() is used, even when
  it's clear what we are inside, e.g. ParallelScavenge-related code. We
  should be using ParallelScavengeHeap::heap() here instead.
  Universe::heap() should (for clarity) only be used by code that
  doesn't know/care which collector we are currently using.

I was initially skeptical of this change, since it seems to me that
CollectedHeap is supposed to provide the general heap protocol, and so
Universe::heap() ought to be used everywhere where the more specific
heap type isn't important. And in a world where different collector
subparts can be mixed and matched together, binding to a specific heap
type can be actively detrimental. But we seem to have mostly moved
away from such a world, eliminating a bunch of the combinations that
used to be (at least purportedly) supported, so that might not be much
of a problem now.

And I do see the point of making all the code that is part of a
specific collector consistently use the same heap accessor.

So I've come around to thinking this is ok after all.  But I did have
to think about it for a while.

------------------------------------------------------------------------------

So looks good, other than copyrights.  I don't need a new webrev for
that.