RFR (L): 8076454, 8076289 and 8076452: removing SharedHeap
Stefan Karlsson
stefan.karlsson at oracle.com
Thu Apr 2 11:33:47 UTC 2015
On 2015-04-02 13:11, Bengt Rutisson wrote:
>
> Hi StefanK,
>
> Thanks for looking at this!
>
> Comments inline.
>
> On 2015-04-01 17:14, Stefan Karlsson wrote:
>> Hi Bengt,
>>
>> On 2015-04-01 14:30, Bengt Rutisson wrote:
>>>
>>> Hi everyone,
>>>
>>> Could I have a couple of reviews for these three patches to remove
>>> SharedHeap?
>>>
>>> I'm sending all three patches in the same email because I think they
>>> are closely related. But I am planning to push them at three
>>> separate changes.
>>>
>>> JDK-8076454: Clean up/move things out of SharedHeap
>>> https://bugs.openjdk.java.net/browse/JDK-8076454
>>> http://cr.openjdk.java.net/~brutisso/8076454/webrev.00/
>>>
>>>
>>> JDK-8076289: Move the StrongRootsScope out of SharedHeap
>>> https://bugs.openjdk.java.net/browse/JDK-8076289
>>> http://cr.openjdk.java.net/~brutisso/8076289/webrev.00/
>>>
>>>
>>> JDK-8076452: Remove SharedHeap
>>> https://bugs.openjdk.java.net/browse/JDK-8076452
>>> http://cr.openjdk.java.net/~brutisso/8076452/webrev.00/
>>>
>>>
>>>
>>> Some more details about the changes:
>>>
>>> JDK-8076454 contains many small changes. Here's the steps I took to
>>> complete the patch. It may be easier to review each step of the way:
>>>
>>> http://cr.openjdk.java.net/~brutisso/8076454/webrev.00-remove_SharedHeap_gcprologue/
>>>
>>> This removes the pure virtual implementations of
>>> SharedHeap::gc_prologue() and SharedHeap::gc_epilogue() since there
>>> are no callers of these methods. All calls are directly to the
>>> concrete subclasses.
>>>
>>> http://cr.openjdk.java.net/~brutisso/8076454/webrev.00-remove_SharedHeap_set_barrier_set/
>>>
>>> This removes SharedHeap::set_barrier() and instead inlines the code
>>> in G1CollectedHeap and GenCollectedHeap similarly to what
>>> ParallelScavengeHeap already does.
>>>
>>> http://cr.openjdk.java.net/~brutisso/8076454/webrev.00-move_post_initialized_and_ref_init/
>>>
>>> SharedHeap::post_initialize() was just forwarding to
>>> CollectedHeap::post_initialize() and adding a call to
>>> ref_processing_init(). Moved this down to the concrete sub classes.
>>>
>>> http://cr.openjdk.java.net/~brutisso/8076454/webrev.00-remove_SharedHeap_no_gc_in_progress/
>>>
>>> no_gc_in_progress() is only used in GenCollectedHeap so it is enough
>>> that it is declared there.
>>>
>>> http://cr.openjdk.java.net/~brutisso/8076454/webrev.00-remove_SharedHeap_space_iterate/
>>>
>>> space_iterate() was dead code.
>>>
>>> http://cr.openjdk.java.net/~brutisso/8076454/webrev.00-remove_SharedHeap_oop_iterate/
>>>
>>> oop_iterate() is already declared pure virtual in CollectedHeap, so
>>> no need to have it in SharedHeap too.
>>>
>>> http://cr.openjdk.java.net/~brutisso/8076454/webrev.00-remove_SharedHeap_space_containing/
>>>
>>> There were no callers of SharedHeap::space_containing(). No need to
>>> have a virtual method in SharedHeap. Enough to declare the methods
>>> in the concrete sub classes.
>>>
>>> http://cr.openjdk.java.net/~brutisso/8076454/webrev.00-cleanup_includes_and_friends/
>>>
>>> Just some cleanups of includes and friend declarations. Not really
>>> relevant as the file will be completely removed by JDK-8076452.
>>>
>>> http://cr.openjdk.java.net/~brutisso/8076454/webrev.00-remove_SharedHeap_set_par_threads/
>>>
>>> SharedHeap::set_par_threads() only introduced an assert compared to
>>> what CollectedHeap::set_par_threads() already does. The assert is
>>> only relevant in the GenCollectedHeap case. So I moved the assert
>>> down to that class.
>>>
>>> http://cr.openjdk.java.net/~brutisso/8076454/webrev.00-move_workers/
>>> Move the instance variable to hold the FlexibleWorkGang (_workers)
>>> down to the concrete sub classes. Only two lines of common code, so
>>> hardly worth sharing.
>>>
>>> http://cr.openjdk.java.net/~brutisso/8076454/webrev.00-product-fix/
>>> I had a mistake for product builds regarding
>>> set_heap_lock_held_for_gc().
>>>
>>>
>>>
>>> JDK-8076289 moves the StrongRootsScope class out to its own file.
>>>
>>> http://cr.openjdk.java.net/~brutisso/8076289/webrev.00-remove_StrongRootsScope_sh/
>>>
>>> The StrongRootsScope no longer makes use of its SharedHeap instance
>>> variable, so it can be removed and the constructor does not have to
>>> take it as a parameter.
>>>
>>> http://cr.openjdk.java.net/~brutisso/8076289/webrev.00-move_StrongRootsScope/
>>>
>>> Move the StrongRootsScope out to its own files and to the global
>>> name space.
>>>
>>> http://cr.openjdk.java.net/~brutisso/8076289/webrev.00-move_MarkScope/
>>> Move the super class of StrongRootsScope, MarkScope, into the same
>>> files.
>>>
>>> I would like to follow this change up with a couple of things. I
>>> would like to rename StrongRootsScope to something like
>>> RootProcessingScope since we no longer use it purely for strong
>>> roots. I would like to rename MarkScope to something like
>>> NMethodScope. And I think ParallelScavenge could use MarkScope
>>> directly instead of introducing ParStrongRootsScope. But I would
>>> like to do these changes as a separate patch.
>>>
>>>
>>> JDK-8076452 is pretty much just one patch to get rid of all
>>> reference to SharedHeap now that it is empty after the above changes.
>>
>> Thanks for providing the incremental patches, it made the patches
>> much easier to review.
>>
>> http://cr.openjdk.java.net/~brutisso/8076454/webrev.00-remove_SharedHeap_set_barrier_set/
>>
>>
>> All three heap implementations perform the same sequence:
>> + _barrier_set = bs;
>> + oopDesc::set_bs(bs);
>>
>> I'd prefer if we could move that to a function in CollectedHeap.
>
> Done.
>
> http://cr.openjdk.java.net/~brutisso/8076454/webrev-remove_SharedHeap_set_barrier_set.01/
>
> http://cr.openjdk.java.net/~brutisso/8076454/webrev-remove_SharedHeap_set_barrier_set.00-01.diff/
>
OK.
>
>>
>>
>> http://cr.openjdk.java.net/~brutisso/8076454/webrev.00-move_post_initialized_and_ref_init/
>>
>>
>> http://cr.openjdk.java.net/~brutisso/8076454/webrev.00-move_post_initialized_and_ref_init/src/share/vm/gc_implementation/g1/g1CollectedHeap.hpp.udiff.html
>>
>>
>> + // Does operations required after initialization has been done.
>> + void post_initialize();
>> +
>> // Initialize weak reference processing.
>> virtual void ref_processing_init();
>>
>> The ref_processing_init doesn't have to be virtual anymore.
>
> Done.
>
> http://cr.openjdk.java.net/~brutisso/8076454/webrev-move_post_initialized_and_ref_init.01/
>
> http://cr.openjdk.java.net/~brutisso/8076454/webrev-move_post_initialized_and_ref_init.00-01.diff/
>
OK.
>
>>
>>
>> http://cr.openjdk.java.net/~brutisso/8076454/webrev.00-remove_SharedHeap_space_iterate/
>>
>>
>> http://cr.openjdk.java.net/~brutisso/8076454/webrev.00-remove_SharedHeap_space_iterate/src/share/vm/gc_implementation/g1/g1CollectedHeap.cpp.udiff.html
>>
>>
>> -void G1CollectedHeap::space_iterate(SpaceClosure* cl) {
>> - SpaceClosureRegionClosure blk(cl);
>> - heap_region_iterate(&blk);
>> -}
>>
>> SpaceClosureRegionClosure isn't used anymore and can be removed.
>
> Done.
>
> http://cr.openjdk.java.net/~brutisso/8076454/webrev-remove_SharedHeap_space_iterate.01/
>
> http://cr.openjdk.java.net/~brutisso/8076454/webrev-remove_SharedHeap_space_iterate.00-01.diff/
>
OK.
>
>>
>>
>> http://cr.openjdk.java.net/~brutisso/8076454/webrev.00-remove_SharedHeap_oop_iterate/
>>
>>
>> CollectedHeap::oop_iterate and CollectedHeap::oop_iterate_no_header
>> are only used by CMS. We might want to move those functions to
>> GenCollectedHeap.
>
> Done.
>
> http://cr.openjdk.java.net/~brutisso/8076454/webrev-remove_SharedHeap_oop_iterate.01/
>
> http://cr.openjdk.java.net/~brutisso/8076454/webrev-remove_SharedHeap_oop_iterate.00-01.diff/
>
OK.
>
>>
>>
>> http://cr.openjdk.java.net/~brutisso/8076454/webrev.00-remove_SharedHeap_space_containing/
>>
>>
>> The few usages of space_containing in G1 could be replaced with
>> heap_region_containing.
>
> Done.
>
> http://cr.openjdk.java.net/~brutisso/8076454/webrev-remove_SharedHeap_space_containing.01/
>
> http://cr.openjdk.java.net/~brutisso/8076454/webrev-remove_SharedHeap_space_containing.00-01.diff/
>
OK.
>
>>
>>
>> http://cr.openjdk.java.net/~brutisso/8076454/webrev.00-move_workers/
>>
>> + if (UseConcMarkSweepGC) {
>> + _workers = new FlexibleWorkGang("GC Thread", ParallelGCThreads,
>> + /* are_GC_task_threads */true,
>> + /* are_ConcurrentGC_threads */false);
>> + _workers->initialize_workers();
>> + } else {
>> + _workers = NULL;
>> + }
>> }
>>
>> Could you add a comment that the else statement is for UseSerialGC,
>> which doesn't use workers.
>
> Done.
>
> http://cr.openjdk.java.net/~brutisso/8076454/webrev-move_workers.01/
> http://cr.openjdk.java.net/~brutisso/8076454/webrev-move_workers.00-01.diff/
>
Could you end the sentence with a period? :)
>
>>
>>
>> http://cr.openjdk.java.net/~brutisso/8076454/webrev.00-product-fix/
>>
>> This is an unrelated change.
>
> Absolutely. This was a mistake. Sorry about that. I removed this patch
> from my patch queue.
OK.
>
>>
>>
>> http://cr.openjdk.java.net/~brutisso/8076289/webrev.00-move_StrongRootsScope/
>>
>>
>> http://cr.openjdk.java.net/~brutisso/8076289/webrev.00-move_StrongRootsScope/src/share/vm/runtime/thread.hpp.udiff.html
>>
>>
>> + // If the client meats this spec, then "thread claim parity" will
>> have
>> + // the following properties:
>> + // a) to return a different value than was returned before the last
>> + // call to change_strong_roots_parity, and
>> + // c) to never return a distinguished value (zero) with which such
>> + // task-claiming variables may be initialized, to indicate
>> "never
>> + // claimed".
>>
>> Could you remove this section? The comments already describe what
>> needs to be known.
>>
>> http://cr.openjdk.java.net/~brutisso/8076289/webrev.00-move_StrongRootsScope/src/share/vm/utilities/workgroup.hpp.udiff.html
>>
>>
>> The comment you moved is too unfocused to aid in the understanding of
>> the different parts of the code, and other comments already explain a
>> lot of what's said in this file. Could you either remove it, reword
>> it or move parts of it to the files where they belong?
>>
>> http://cr.openjdk.java.net/~brutisso/8076289/webrev.00-move_StrongRootsScope/src/share/vm/memory/strongRootsScope.hpp.html
>>
>> -
>> 30 // Some collectors will perform "process_strong_roots" in parallel.
>> 31 // Such a call will involve claiming some fine-grained tasks,
>> such as
>> 32 // scanning of threads and code blobs.
>> 33 // Claiming of these tasks requires that sequential code calls
>> 34 // initialization methods to set the claiming code in the right
>> 35 // state for parallel task claiming.
>> 36 // StrongRootsScope is a way to capture such setup code to make
>> 37 // sure that it is executed in the correct way.
>>
>> Could this comment be reworded into one paragraph?
>
> Removed and updated the comments as you suggested.
>
> http://cr.openjdk.java.net/~brutisso/8076289/webrev-move_StrongRootsScope.01/
>
> http://cr.openjdk.java.net/~brutisso/8076289/webrev-move_StrongRootsScope.00-01.diff/
>
OK.
>
>>
>>
>> http://cr.openjdk.java.net/~brutisso/8076289/webrev.00-move_MarkScope/
>>
>> http://cr.openjdk.java.net/~brutisso/8076289/webrev.00-move_MarkScope/src/share/vm/memory/strongRootsScope.hpp.udiff.html
>>
>>
>> Add include allocation.hpp.
>>
>> Could you move the initialization lists up one line?
>
> Done.
>
> http://cr.openjdk.java.net/~brutisso/8076289/webrev-move_MarkScope.01/
> http://cr.openjdk.java.net/~brutisso/8076289/webrev-move_MarkScope.00-01.diff/
>
OK.
>
>>
>>
>> http://cr.openjdk.java.net/~brutisso/8076452/webrev.00/
>>
>> This is missing changes to the SA.
>
> Good catch! Fixed.
>
> http://cr.openjdk.java.net/~brutisso/8076452/webrev.01/
> http://cr.openjdk.java.net/~brutisso/8076452/webrev.00-01.diff/
OK.
>
>>
>> I think a few of the includes to collectedHeap.hpp are unnecessary.
>
> That is probably true. I would prefer to clean that up separately
> since this patch already contains a lot of changes. My proposed patch
> only replaces includes of sharedHeap.hpp with collectedHeap.hpp, which
> I think makes it easier to review.
Fine.
Looks good!
Thanks,
StefanK
>
> Thanks,
> Bengt
>
>>
>> Thanks,
>> StefanNK
>>>
>>> Thanks,
>>> Bengt
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>
>
More information about the hotspot-gc-dev
mailing list