Re: RFR: 8200729: Conditional compilation of GCs
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_ 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_ 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
Re: RFR: 8200729: Conditional compilation of GCs
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_ 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
Re: RFR: 8200729: Conditional compilation of GCs
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_ 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
Re: RFR: 8200729: Conditional compilation of GCs
On 2018-05-03 11:06, Magnus Ihse Bursie wrote: On 2018-05-02 16:37, 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/ It's nice to see this cleanup in build logic! I spotted one issue with the build logic, otherwise it looks good. In JvmFeatures.gmk, remove the "JVM_EXCLUDE_FILES += none" lines. They are not needed and are technically incorrect (makes the build tries to exclude files named "none"). Thanks for reviewing this, and helping out writing the make files changes! I'll remove the += none lines. StefanK /Magnus (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_ 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_ defines. http://cr.openjdk.java.net/~stefank/8200729/webrev.04/03.selectIndivudualGCsMakePatch/ Thanks, StefanK
Re: RFR: 8200729: Conditional compilation of GCs
On 2018-05-02 16:37, 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/ It's nice to see this cleanup in build logic! I spotted one issue with the build logic, otherwise it looks good. In JvmFeatures.gmk, remove the "JVM_EXCLUDE_FILES += none" lines. They are not needed and are technically incorrect (makes the build tries to exclude files named "none"). /Magnus (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_ 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_ defines. http://cr.openjdk.java.net/~stefank/8200729/webrev.04/03.selectIndivudualGCsMakePatch/ Thanks, StefanK
RFR: 8200729: Conditional compilation of GCs
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_ 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_ defines. http://cr.openjdk.java.net/~stefank/8200729/webrev.04/03.selectIndivudualGCsMakePatch/ Thanks, StefanK
