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