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

Bengt Rutisson bengt.rutisson at oracle.com
Thu Apr 2 11:11:27 UTC 2015


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/

>
>
> 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/

>
>
> 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/

>
>
> 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/

>
>
> 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/

>
>
> 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/

>
>
> 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.

>
>
> 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/

>
>
> 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/

>
>
> 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/

>
> 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.

Thanks,
Bengt

>
> Thanks,
> StefanNK
>>
>> Thanks,
>> Bengt
>>
>>
>>
>>
>>
>>
>>
>



More information about the hotspot-gc-dev mailing list