RFR (M): JDK-8076314: Remove the static instance variable SharedHeap:: _sh

[Per Liden]

Hi,

On 2015-04-01 17:08, Bengt Rutisson wrote:
>
> Hi Per,
>
> Thanks for looking at this!
>
> On 2015-04-01 14:03, Per Liden wrote:
>> Hi,
>>
>> On 2015-03-31 15:14, Bengt Rutisson wrote:
>>>
>>> Hi all,
>>>
>>> Could I have a couple of reviews for this change?
>>>
>>> http://cr.openjdk.java.net/~brutisso/8076314/webrev.00/
>>> https://bugs.openjdk.java.net/browse/JDK-8076314
>>>
>>> This is another step towards getting rid of SharedHeap. There used to be
>>> an instance variable in SharedHeap to keep track of the actual instance
>>> being used (which would be of type GenCollectedHeap or G1CollectedHeap).
>>>
>>> The SharedHeap::heap() would return this value and was mostly used in
>>> asserts.
>>>
>>> I've changed so that we use the specific instance in most cases. That is
>>> either GenCollectedHeap::heap() or G1CollectedHeap::heap().
>>>
>>> A couple of other changes were needed to avoid the use of
>>> SharedHeap::heap():
>>>
>>> - The field  _thread_holds_heap_lock_for_gc was only used by G1 and was
>>> thus moved to G1CollectedHeap. It brought with it the method
>>> heap_lock_held_for_gc() and I introduced set_heap_lock_held_for_gc() to
>>> set the value. All of these are only used in asserts so I put them in an
>>> #ifndef PRODUCT section. This also meant that I had to implement
>>> doit_prologue() and doit_epilogue() for VM_G1OperationWithAllocRequest
>>> to make sure that the state of _thread_holds_heap_lock_for_gc got set
>>> correctly.
>>>
>>> - The ageTable::compute_tenuring_threshold() method used the
>>> SharedHeap::heap() only to look up the perf counters for GC. Instead of
>>> having compute_tenuring_threshold() look that up I now pass in the
>>> correct counters from the callers.
>>>
>>> - The Threads::possibly_parallel_oops_do() method used the
>>> SharedHeap::heap() only to check if we were to execute in parallel. I
>>> moved the lookup of this to the callers and now just pass in an is_par
>>> paramter.
>>>
>>> - I simplified the VM_CGC_Operation::doit() a bit since even before my
>>> change the heap returned could never be NULL.
>>
>> Overall the change looks good, nice cleanup!
>>
>> One question, how about removing heap_lock_held_for_gc() and friends
>> completely? There's quite a bit of code added just for this single
>> assert() and I'm not sure the added noise is justified here. The path
>> already has an assert_heap_locked_or_at_safepoint() which it's exactly
>> the same thing, but I'm thinking it's enough.
>
> Yes, good point. I was thinking the same but didn't really dare to
> remove the assert. But I agree that the extra code is too much compared
> to the value of the assert. I removed it.
>

Like!

>>
>> A few other cosmetic comments:
>>
>> genCollectedHeap.cpp
>> --------------------
>>  609   bool is_par =n_par_threads() > 0;
>
> Fixed.
>
>>
>> A space missing there.
>>
>> g1CollectedHeap.cpp:
>> --------------------
>>
>> 2161 bool G1CollectedHeap::heap_lock_held_for_gc() {
>> 2162   Thread* t = Thread::current();
>> 2163   return    Heap_lock->owned_by_self()
>> 2164          || (   (t->is_GC_task_thread() || t->is_VM_thread())
>> 2165              && _thread_holds_heap_lock_for_gc);
>> 2166 }
>>
>> A too many spaces here and there and lines are a bit misaligned.
>
> This code got removed. :)
>
> Updated webrev:
> http://cr.openjdk.java.net/~brutisso/8076314/webrev.01/
>
> Diff compared to last version:
> http://cr.openjdk.java.net/~brutisso/8076314/webrev.00-01.diff/

Looks good Bengt!

/Per