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

Bengt Rutisson bengt.rutisson at oracle.com
Wed Apr 1 20:03:55 UTC 2015


Hi Jon,

Thanks for looking at this!

On 01/04/15 20:11, Jon Masamitsu wrote:
> 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 
>

Sorry about that. Something must have gone wrong when I produced the 
webrev. Both of these files should be unchanged by my patch. The 
previous webrev looks correct:

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

The only thing I did for version 02 was to remove the two asserts.

>
> Otherwise, looks good.
>
> Reviewed.

Great! Thanks!

Bengt

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



More information about the hotspot-gc-dev mailing list