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

Stefan Karlsson stefan.karlsson at oracle.com
Wed Apr 1 15:14:34 UTC 2015 

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.


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.


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.


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.


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.


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.


http://cr.openjdk.java.net/~brutisso/8076454/webrev.00-product-fix/

This is an unrelated change.


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?


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?


http://cr.openjdk.java.net/~brutisso/8076452/webrev.00/

This is missing changes to the SA.

I think a few of the includes to collectedHeap.hpp are unnecessary.

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

More information about the hotspot-gc-dev mailing list