RFR: 8200729: Conditional compilation of GCs

[Erik Helin]

On 05/02/2018 04:37 PM, Stefan Karlsson wrote:
 > Hi all,

Hi Stefan,

thanks for taking on this work, much appreciated!

On 05/02/2018 04:37 PM, Stefan Karlsson wrote:
 > The first patch simply cleans up some INCLUDE_ALL_GCS usages in 
platform-specific files. Some of these changes are already being cleaned 
up by other RFEs:
 >
 > 
http://cr.openjdk.java.net/~stefank/8200729/webrev.04/00.removeUnneededIncludeAllGCs/ 


Looks good, Reviewed.

On 05/02/2018 04:37 PM, Stefan Karlsson wrote:
 > The second patch pre-cleans some include files:
 >
 > http://cr.openjdk.java.net/~stefank/8200729/webrev.04/01.fixIncludes/

Also looks good, Reviewed.

On 05/02/2018 04:37 PM, Stefan Karlsson wrote:
 > The following is the main patch, which include all relevant HotSpot 
changes. For a while now, we have been actively working on abstracting 
away GC specific code from non-GC directories, but as can be seen in 
this patch, there are still a few places left. Hopefully, we will get 
rid of most of these in the near future.
 >
 > http://cr.openjdk.java.net/~stefank/8200729/webrev.04/02.mainPatch/

Very nice, just one small nit:

- barrierSetConfig.hpp:
   could you move "+  G1GC_ONLY(f(G1BarrierSet))" into
   FOR_EACH_CONCRETE_BARRIER_SET_DO ? As in

    // Do something for each concrete barrier set part of the build.
    #define FOR_EACH_CONCRETE_BARRIER_SET_DO(f)          \
      f(CardTableBarrierSet)                             \
      G1GC_ONLY(f(G1BarrierSet))

As a note for people following along this thread (and to a future 
version of myself), the following work is ongoing to further clean up 
the GC specific bits and pieces sprinkled throughout the rest of the VM:
- 8202377: Modularize C2 GC barriers
- 8202547: Move G1 runtime calls used by generated code to 
G1BarrierSetRuntime
- 8201436: Replace oop_ps_push_contents with oop_iterate and closure

In addition to the above work, this patch highlights a few more places 
that needs to be taken care of:
- get rid of #if INCLUDE_CMS in defNewGeneration.cpp
- split collector specific parts of gcTrace.hpp into collector
   specific tracers
- rework Threads::create_thread_roots_tasks into something more generic
   that parallel then can use to implement its own
   create_thread_roots_task
- remove marksweep_init() and set up MarkSweep::_gc_timer and
   MarkSweep::_gc_tracer in SerialHeap and ParallelScavengeHeap
- benchmark (and measure file sizes) to see if it is still worthwhile to
   keep the INCLUDE_OOP_OOP_ITERATE_BACKWARDS guard (i.e. can we always
   compile the oop_iterate_backwards methods?)
- need to do some work to encapsulate the G1 and CDS code
   (see e.g. oop.cpp, metaspaceShared.cpp and file.cpp)
- move VM_CollectForMetadataAllocation::initiate_concurrent_GC to
   something like
   CollectedHeap::initiate_concurrent_GC_for_metadata_allocation
   (preferably with a shorter name

(the above list is _not_ complete, there will always be a need for 
cleanups :D)

 > The last patch adds the make file support to turn on and off the 
different GCs. The content of this patch has evolved from versions 
written by myself, Per, and Magnus Ihse Bursie. Magnus last proposed 
version used the names gc-cms, gc-g1, gc-parallel, and gc-serial, but 
I've changed them to cmsgc, g1gc, parallelgc, and serialgc, so that they 
match the INCLUDE_<GC> defines.
 >
 > 
http://cr.openjdk.java.net/~stefank/8200729/webrev.04/03.selectIndivudualGCsMakePatch/ 


My Makefile knowledge is limited to smaller hacks, I will leave this 
patch for Magnus to review (which I see he already has done).

Overall, very nice work Stefan, thanks!
Erik