RFR (L): 8076454, 8076289 and 8076452: removing SharedHeap

Bengt Rutisson bengt.rutisson at oracle.com
Thu Apr 2 14:10:31 UTC 2015


Thanks StefanK, StefanJ and David for the reviews!

Bengt

On 2015-04-02 16:08, Stefan Johansson wrote:
> Hi Bengt,
>
> 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!
>
> Stefan
>> 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