RFR: 8200729: Conditional compilation of GCs

Thu May 3 17:22:23 UTC 2018

Hi Vladimir,

On 2018-05-03 18:56, Vladimir Kozlov wrote:
> I see that you don't guard Use*GC flags with _ONLY macros. It is hard to 
> figure out from gcConfig.cpp what happened if UseG1GC is specified on 
> command line for VM which does not include it. What happens?

Right. The current patch leaves the Use*GC flags available in all 
builds, so that we can accept the flag but give a warning:

$ build/slowdebug-no-g1/jdk/bin/java -XX:+UseG1GC
Java HotSpot(TM) 64-Bit Server VM warning: -XX:+UseG1GC not supported in 
this VM

This is handled in GCConfig::select_gc_ergonomically with this line:
   NOT_G1GC(      UNSUPPORTED_OPTION(UseG1GC);)

That expands into:
// Disable options not supported in this release, with a warning if they
// were explicitly requested on the command-line
#define UNSUPPORTED_OPTION(opt)                          \
do {                                                     \
   if (opt) {                                             \
     if (FLAG_IS_CMDLINE(opt)) {                          \
       warning("-XX:+" #opt " not supported in this VM"); \
     }                                                    \
     FLAG_SET_DEFAULT(opt, false);                        \
   }                                                      \
} while(0)

> 
> I don't see C1 changes but it looks like it was changed with 8201543.

Right. There's no INCLUDE_<GC> guards left in C1!

> C2 changes seems fine but they will be also affected by 8202377.

Exactly.

> 
> What is left is AOT and JVMCI/Graal. We thought to implement "cross" 
> compilation when we can specify for which configuration we should 
> generate AOT code. It includes GC barriers code. Currently I see that 
> you keep all GCs in build as before so it is not a issue. And we record 
> VM version so we will not generate and use G1 barriers if it is not in 
> VM used by jaotc.
> 
> So it seems to me compiler and AOT, Graal changes are fine.

OK. Thanks for taking a close look at this.

> 
> I would suggest in addition to hs-tier2 testing run hs-tier2-graal to 
> make sure Graal still works.

I'll make sure to test with hs-tier2-graal.

Thanks for reviewing!

StefanK

> 
> Thanks,
> Vladimir
> 
> On 5/2/18 7:37 AM, Stefan Karlsson wrote:
>> Hi all,
>>
>> Please review these patches to allow for conditional compilation of 
>> the GCs in HotSpot.
>>
>> Full patch:
>>
>> http://cr.openjdk.java.net/~stefank/8200729/webrev.04/all/
>>
>> (See below for a more fine-grained division into smaller patches)
>>
>> Today Parallel, G1, and CMS, are all guarded by INCLUDE_ALL_GCS. 
>> INCLUDE_ALL_GCS becomes defined to 1 for our server 
>> (--with-jvm-variants=server) builds, and is defined to 0 for the 
>> minimal (--with-jvm-variants=minimal) builds. There are also ways to 
>> forcefully remove these GCs from the compilation by configuring with, 
>> for example, --with-jvm-features=all-gcs.
>>
>> The proposed patch removes INCLUDE_ALL_GCS (and all-gcs) and replaces 
>> it with INCLUDE_CMSGC, INCLUDE_G1GC, and INCLUDE_PARALLELGC. In 
>> addition to that, INCLUDE_SERIALGC has been added to guard the Serial 
>> GC code.
>>
>> Future GCs should adopt this scheme before they get incorporated into 
>> the code base. Note, that there will be some files in gc/shared that 
>> are going to have to know about all GCs, but the goal is to have very 
>> few of these INCLUDE_<GC> checks in the non-GC parts of HotSpot.
>>
>> With this patch, it's also going to be easier to stop compiling CMS 
>> when the time as come for it to move from deprecated to removed.
>>
>> Note, that even though this adds great flexibility, and allows for 
>> easy inclusion / exclusion of GCs, there's no guarantee that a 
>> specific combination of GCs is going to be maintained in the future. 
>> Just like not all combinations of the different Runtime features (CDS, 
>> NMT, JFR) and Compiler features (AOT, COMPILER1, COMPILER2) are 
>> guaranteed to compile and be supported. I've sanity checked the 
>> different GC configurations build locally, but I've only fully tested 
>> the "server" variant, and "minimal" has only been built locally.
>>
>> There's a more high-level discussion around the flexibility the 
>> --with-jvm-features flag adds, and if we really should allow it. Since 
>> this patch only builds upon that existing flexibility, and I don't 
>> change the two defined jvm variants, I'd appreciate if that discussion 
>> could be kept out of this review thread. For further discussion around 
>> this, please see:
>>
>> http://mail.openjdk.java.net/pipermail/build-dev/2018-April/021663.html
>>
>> This is the patch queue:
>>
>> 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/ 
>>
>>
>>
>> The second patch pre-cleans some include files:
>>
>> http://cr.openjdk.java.net/~stefank/8200729/webrev.04/01.fixIncludes/
>>
>>
>> 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/
>>
>>
>> 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/ 
>>
>>
>> Thanks,
>> StefanK