RFR (M): JDK-8076314: Remove the static instance variable SharedHeap:: _sh
Stefan Karlsson
stefan.karlsson at oracle.com
Thu Apr 2 18:03:31 UTC 2015
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