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

[Bengt Rutisson]

Thanks for the review, Per!

Bengt


On 2015-04-01 17:39, Per Liden wrote:
> 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