RFR: 8200729: Conditional compilation of GCs

[Stefan Karlsson]

On 2018-05-03 15:19, Erik Helin wrote:
> 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))
> 

Yes. That's better.

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

I agree.

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

Thanks Erik!

StefanK

> Erik