RFR (L): 8076454, 8076289 and 8076452: removing SharedHeap
David Lindholm
david.lindholm at oracle.com
Thu Apr 2 14:00:34 UTC 2015
On 2015-04-02 15:40, Bengt Rutisson wrote:
>
> Hi Stefan, Stefan and David,
>
> StefanK, I've added the period at the end of the comment that you
> suggested. :)
>
> Thanks for the review!
>
> StefanJ and David. Thanks for looking at this offiline. Here are the
> changes that you suggested when we went through the code:
>
> http://cr.openjdk.java.net/~brutisso/8076454/webrev.01-02.diff/
> http://cr.openjdk.java.net/~brutisso/8076289/webrev.01-02.diff/
> http://cr.openjdk.java.net/~brutisso/8076452/webrev.01-02.diff/
Looks good!
/David
> And here are the complete webrevs:
>
> http://cr.openjdk.java.net/~brutisso/8076454/webrev.02/
> http://cr.openjdk.java.net/~brutisso/8076289/webrev.02/
> http://cr.openjdk.java.net/~brutisso/8076452/webrev.02/
>
> Thanks,
> Bengt
>
>
> On 2015-04-02 13:33, Stefan Karlsson wrote:
>>
>>
>> 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