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.


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.



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



-void G1CollectedHeap::space_iterate(SpaceClosure* cl) {
-  SpaceClosureRegionClosure blk(cl);
-  heap_region_iterate(&blk);

SpaceClosureRegionClosure isn't used anymore and can be removed.


CollectedHeap::oop_iterate and CollectedHeap::oop_iterate_no_header are 
only used by CMS. We might want to move those functions to GenCollectedHeap.


The few usages of space_containing in G1 could be replaced with 


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


This is an unrelated change.



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


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?

   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?



Add include allocation.hpp.

Could you move the initialization lists up one line?


This is missing changes to the SA.

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

> Thanks,
> Bengt

More information about the hotspot-gc-dev mailing list