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

[Jon Masamitsu]

Pages intentionally blank?  Just don't want to miss anything.
It happens.

http://cr.openjdk.java.net/~brutisso/8076314/webrev.02/src/share/vm/gc_implementation/g1/g1CollectedHeap.hpp.udiff.html

http://cr.openjdk.java.net/~brutisso/8076314/webrev.02/src/share/vm/gc_implementation/g1/vm_operations_g1.hpp.udiff.html

Otherwise, looks good.

Reviewed.

Jon

On 4/1/2015 8:54 AM, Bengt Rutisson wrote:
>
> 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
>>
>