RFR (M): JDK-8076314: Remove the static instance variable SharedHeap:: _sh
Bengt Rutisson
bengt.rutisson at oracle.com
Thu Apr 2 18:48:31 UTC 2015
Hi Volker and Stefan,
Just like Stefan mentioned, we found this and I included the fix in the
push that completed about an hour ago. Let me know if you find any more
issue with the latest hotspot-gc repo.
I totally agree that we need JPRT to verify the builds without
precompiled headers. It is probably a good idea to bring that up in a
separate thread.
Thanks,
Bengt
On 02/04/15 20:03, Stefan Karlsson wrote:
> Hi Volker,
>
> On 2015-04-02 17:33, Volker Simonis wrote:
>> Hi Bengt,
>>
>> the bad news is that your change breaks the build without precompiled
>> headers (i.e. '--disable-precompiled-headers' but please notice that
>> there are platforms which don't support precompiled headers at all).
>> You'll get:
>>
>> /OpenJDK/jdk9-hs-gc/hotspot/src/share/vm/memory/cardTableModRefBS.cpp:
>> In member function ‘void
>> CardTableModRefBS::non_clean_card_iterate_possibly_parallel(Space*,
>> MemRegion, OopsInGenClosure*, CardTableRS*)’:
>> /OpenJDK/jdk9-hs-gc/hotspot/src/share/vm/memory/cardTableModRefBS.cpp:462:
>>
>> error: incomplete type ‘GenCollectedHeap’ used in nested name
>> specifier
>> /OpenJDK/jdk9-hs-gc/hotspot/src/share/vm/memory/cardTableModRefBS.cpp:466:
>>
>> error: incomplete type ‘GenCollectedHeap’ used in nested name
>> specifier
>> /OpenJDK/jdk9-hs-gc/hotspot/src/share/vm/memory/cardTableModRefBS.cpp:466:
>>
>> error: incomplete type ‘GenCollectedHeap’
>>
>>
>> But the (kind of) good new is that on of the follow-up changes in
>> hs-gc (probably "8075955: Replace the macro based implementation of
>> oop_oop_iterate with a template based solution" which is huge)
>> unintentionally fixed it again. Nevertheless I'd suggest to fix this
>> "the right way" by including "memory/genCollectedHeap.hpp" instead of
>> "memory/sharedHeap.hpp" into cardTableModRefBS.cpp:
>>
>> diff -r b88bb4de100e src/share/vm/memory/cardTableModRefBS.cpp
>> --- a/src/share/vm/memory/cardTableModRefBS.cpp Thu Apr 02 09:14:16
>> 2015 +0200
>> +++ b/src/share/vm/memory/cardTableModRefBS.cpp Thu Apr 02 17:29:28
>> 2015 +0200
>> @@ -26,7 +26,7 @@
>> #include "memory/allocation.inline.hpp"
>> #include "memory/cardTableModRefBS.inline.hpp"
>> #include "memory/cardTableRS.hpp"
>> -#include "memory/sharedHeap.hpp"
>> +#include "memory/genCollectedHeap.hpp"
>> #include "memory/space.hpp"
>> #include "memory/space.inline.hpp"
>> #include "memory/universe.hpp"
>
> We wound this earlier today, and I think Bengt pushed the fix together
> with this patch:
> http://hg.openjdk.java.net/jdk9/hs-gc/hotspot/rev/4e28fac1367b
>
>>
>> Moreover we REALLY need a non-precompiled-header build in JPRT as this
>> is the second such case in the last two weeks. I'll write a special
>> mail about this topic to hotspot-dev soon.
>
> I agree.
>
> Thanks,
> StefanK
>
>>
>> Thank you and best regards,
>> Volker
>>
>>
>>
>> On Wed, Apr 1, 2015 at 10:03 PM, Bengt Rutisson
>> <bengt.rutisson at oracle.com> wrote:
>>> 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