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

[Per Liden]

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

s/it's/isn't/

/Per

> the same thing, but I'm thinking it's enough.
>
> A few other cosmetic comments:
>
> genCollectedHeap.cpp
> --------------------
>   609   bool is_par =n_par_threads() > 0;
>
> 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.
>
> /Per