RFR (M): JDK-8076314: Remove the static instance variable SharedHeap:: _sh
Volker Simonis
volker.simonis at gmail.com
Thu Apr 2 15:33:35 UTC 2015
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"
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.
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