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

[Bengt Rutisson]

One last update to this.

There were two assert in that I had to remove too. I had this in a patch 
but missed it for the webrev. The complete webrev is now here:

http://cr.openjdk.java.net/~brutisso/8076314/webrev.02/

The diff compared to the last version is this change in thread.cpp:

@@ -752,17 +752,13 @@
      jint res = Atomic::cmpxchg(strong_roots_parity, &_oops_do_parity, 
thread_parity);
      if (res == thread_parity) {
        return true;
      } else {
        guarantee(res == strong_roots_parity, "Or else what?");
-      assert(SharedHeap::heap()->workers()->active_workers() > 0,
-             "Should only fail when parallel.");
        return false;
      }
    }
-  assert(SharedHeap::heap()->workers()->active_workers() > 0,
-         "Should only fail when parallel.");
    return false;
  }

  void Thread::oops_do(OopClosure* f, CLDClosure* cld_f, 
CodeBlobClosure* cf) {
    active_handles()->oops_do(f);



The asserts check that we are parallel, but the 
Thread::claim_oops_do_par_case() method is only called when we are 
parallel. Now that I have removed SharedHeap::heap() there is not good 
way to keep these asserts.

Thanks,
Bengt

On 2015-04-01 17:36, Bengt Rutisson wrote:
>
> 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
>