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

Bengt Rutisson bengt.rutisson at oracle.com
Wed Apr 1 15:08:29 UTC 2015

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.

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

This code got removed. :)

Updated webrev:

Diff compared to last version:


> /Per

More information about the hotspot-gc-dev mailing list