RFR(s): 8077415: Remove duplicate variables holding the CollectedHeap

[Per Liden]

Hi Kim,

On 2015-04-12 07:21, Kim Barrett wrote:
> On Apr 11, 2015, at 3:05 PM, Kim Barrett <kim.barrett at oracle.com> wrote:
>>
>> On Apr 10, 2015, at 10:51 AM, Per Liden <per.liden at oracle.com> wrote:
>>>
>>> Hi,
>>>
>>> Universe::_collectedHeap points to the current CollectedHeap. However, collectors also have their own static instances for no real good reason. This patch removes all but the Universe::_collectedHeap and let users call Universe::heap() where needed.
>>>
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8077415
>>>
>>> Webrev: http://cr.openjdk.java.net/~pliden/8077415/webrev.0/
>>>
>>> (This patch build on top of http://cr.openjdk.java.net/~pliden/8077413/webrev.0/ which is currently out for review)
>>>
>>> cheers,
>>> /Per
>>
>> ------------------------------------------------------------------------------
>> src/share/vm/gc_implementation/g1/g1CollectedHeap.cpp
>> 3311 G1CollectedHeap* G1CollectedHeap::heap() {
>> 3312   G1CollectedHeap* heap = (G1CollectedHeap*)Universe::heap();
>> 3313   assert(heap != NULL, "Uninitialized access to G1CollectedHeap::heap()");
>> 3314   assert(heap->kind() == CollectedHeap::G1CollectedHeap, "Not a G1CollectedHeap");
>> 3315   return heap;
>> 3316 }
>>
>> Please don't do this.  If the heap isn't of the expected kind, then
>> the cast invokes undefined behavior and the heap->kind() check could
>> crash.  So the assert actually provides a false sense of safety.
>>
>> It was for this kind of thing that I introduced FakeRttiSupport and
>> used it for BarrierSet downcasts.  I was intending to use it similarly
>> for CollectedHeap downcasts, but have not gotten around to doing so.
>> See barrier_set_cast<>().

Good point. I wouldn't mind using FakeRtti here, but I'd like us to 
massage its interface a bit first. My main concern is that using it 
today requires a fair bit of ceremony. But let me continue that 
discussion in a separate mail thread, where I can expand a bit more on 
what I'm thinking. For now I'd like to just do the kind() check before 
the cast, to avoid doing something with undefined behavior, like this:

G1CollectedHeap* G1CollectedHeap::heap() {
   CollectedHeap* heap = Universe::heap();
   assert(heap != NULL, "Uninitialized access to G1CollectedHeap::heap()");
   assert(heap->kind() == CollectedHeap::G1CollectedHeap, "Not a 
G1CollectedHeap");
   return (G1CollectedHeap*)heap;
}

Ok?

>
> The idea with FakeRttiSupport was to make the kind() test so cheap that it would be reasonable
> to use it in a guarantee rather than an assert.
>

cheers,
/Per