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

[Per Liden]

Hi Kim,

On 2015-04-12 09:38, Kim Barrett wrote:
> 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.
>

Check, will fix.

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

I think the intent of the Universe interface have shifted a bit over the 
years. As far as I understand it used to be the main heap interface at 
some point, but these days all heap are CollectedHeaps and so that has 
sort of taken over the role of main heap interface.

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

Thanks for reviewing!

cheers,
/Per