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

[Per Liden]

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.

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