RFR (11u, XXL): Upstream/backport Shenandoah to JDK11u

[Roman Kennke]

Hi Kelvin,

On Thu, 2020-07-23 at 01:56 +0000, Nilsen, Kelvin wrote:
> This is an unofficial review (I am not an official reviewer) of the
> shared-code changes described in 
> http://cr.openjdk.java.net/~rkennke/shenandoah-jdk11u-upstream/webrev.09-shared/
>  which were posted by "Roman Kennke" (rkennke at redhat.com) on Mon
> Jul 13 16:06:12 UTC 2020.  See 
> https://mail.openjdk.java.net/pipermail/jdk-updates-dev/2020-July/003483.html
>  for the original message.

Thank you, Kelvin for reviewing it! You could have replied to the
original email that you mentioned :-)

> For the most part, these all look good and I concur that these do not
> impact non-Shenandoah deployments except for the possible concerns
> identified below:
> 
> 1.  In make/hotspot/gensrc/GensrcJfr.gmk, around line 68, it might be
> good to list the following ExecuteWithLog arguments in the same order
> as when Shenandoah is not enabled to make it more clear that these
> parameters are "not affected": $(METADATA_XML) $(METADATA_XSD)
> $(JFR_OUTPUTDIR)
> 

This is not really possible. The patch also changes
GenerateJfrFiles.java to accept multiple - and optional - arguments for
metadata.xml, which means that it's most reasonable to place them at
the end, and not first.

BTW, I intend to upstream those build changes to jdk16 too.

> 2. In src/hotspot/share/gc/shared/gc_globals.hpp, around line 200, it
> looks like the lines:
>       product(bool, UseShenandoahGC, false,
>                      "Use the Shenandoah garbage collector")
>     should be included only under #if INCLUDE_SHENANDOAHGC
> conditional compilation control.

That is not directly possible because I can't put an ifdef in the
middle of a huge define. It can be done though: define the block under
#if INCLUDE_SHENANDOAHGC above, and empty otherwise, and put that in
the huge #define. I am doing that in the webrev.10 (see below). With
that, the last remnants of Shenandoah are purged from libjvm.so when
building without Shenandoah :-) On the (somewhat) downside, I also had
to remove -XX:-UseShenandoahGC from TestDisableDefaultGC.java, that
test would otherwise fail. (It does print 'unrecognized VM option' now,
instead of "Garbage collector not selected").

>  3. In src/hotspot/share/opto/loopnode.cpp, around line 4514, it is
> not clear from context that the Shenandoah change still includes all
> of the same code that was present before the Shenandoah change.  In
> particular, it is not clear that !defined(PRODUCT) is universally
> true.
> 

Before the change, the rpo() method was only included when !PRODUCT.
Shenandoah requires this method in PRODUCT-builds too, hence the
change. I am not sure how to make this much clearer?

>  4. Likewise, in src/hotspot/share/opto/loopnode.hpp, around line
> 1327, the prototype for function void is introduced by Shenandoah
> enhancements to the code even #if INCLUDE_SHENANDOAHGC in the case of
> ifndef PRODUCT.

See comment above.

> 5. In src/hotspot/share/opto/type.hpp, around lines 1775 and 1822, do
> we want to #if INCLUDE_SHENANDOAHGC the definitions of LoadXNode and
> StoreXNode?  Should be harmless as is.

We could, but as you say it's harmless and doesn't generate any code
per-se. I did it anyway in webrev.00.

> 6. In src/hotspot/share/runtime/vmOperations.hpp, around line 108, do
> we want to #if INCLUDE_SHENANDOAHGC the mentions of ShenandoahFullGC,
> ShenandoahInitMark, ShenandoahFinalMarkStartEvac,
> ShenadoahInitUpdateRefs, ShenandoahFinalUpdateRefs,
> ShenandoahDegeneratedGC?  Should be harmless as is.

That is exactly what the block above with #if INCLUDE_SHENANDOAHGC
does. We cannot #if INCLUDE_SHENANDOAHGC in the middle of a large
#define. Hence the little stunt. (BTW, this is exactly the pattern how
we could exclude the global UseShenandoahGC flag too, see above).

> 7. In src/hotspot/share/utilities/globalDefinitions.hpp, around line
> 98, do we want to put the UINT64_FORMAT_X_W(width) macro under #if
> INCLUDE_SHENANDOAHGC?  Harmless as is.

We could, but as you say it's harmless and doesn't generate any code
per-se. I did it anyway in webrev.00.

> 8. In src/hotspot/share/utilities/macros.hpp, around line 238, should
> the definition of NOT_SHENANDOAHGC_RETURN be { return; }?

I don't think that is necessary. The definitions of other GCs are not
doing it either, so let's keep it consistent with those, yes?

> 9. I am not confident I understand how the file
> src/jdk.jfr/share/conf/jfr/default.jfc is compiled.  Should lines 423
> - 431 be under #if INCLUDE_SHENANDOAHGC control?  Does this cause
> unwanted Shenandoah code to be compiled in even when it's not
> supposed to be? 

This is JFR profiles, it would not compile any code. We ensure that
Shenandoah events are only compiled when Shenandoah is build-enabled
(via the metadata.xml machinery mentioned above). Running JFR with such
a profile on a JDK without Shenandoah support does work as usual. Those
events are enabled, but simply not present.

> 10. I am not confident I understand how regression suites are
> configured.  In test/hotspot/jtreg/TEST.ROOT and test/jdk/TEST.ROOT,
> it appears that we are requiring vm.gc.Shenandoah in order to run
> certain tests.  If the JVM is compiled without support for
> Shenandoah, will this cause these regression tests to not run.  That
> would not be good for someone who desires to build JDK 11 without
> Shenandoah support.

The test is specifically there to test if Shenandoah is built-in. Tests
that execute Shenandoah code by enabling -XX:+UseShenandoahGC are all
guarded by this, so they would not be run when Shenandoah is not built-
in. See e.g.:

http://cr.openjdk.java.net/~rkennke/shenandoah-jdk11u-upstream/webrev.09-shared/test/hotspot/jtreg/gc/TestHumongousReferenceObject.java.udiff.html

> 11. In test/hotspot/jtreg/TEST.groups, it appears that there have
> been many Shenandoah-specific tests added to the regression
> suites.  If someone builds the JDK11 without Shenandoah support and
> runs the regression suite, will all of these tests now fail?  I don't
> see how the running of these tests is made conditional.  (Perhaps my
> issue 10 and issue 11 are complementary and between them, all of my
> concerns are fully addressed.)

See comment for #10. It is safe to run those suites with Shenandoah-
build-disabled, it will only run a handful of tests then, and exclude
all run configurations that have -XX:+UseShenandoahGC in them.

Here's the updated webrevs, with UseShenandoahGC flag excluded on non-
Shenandoah builds, and the two exclusions of macros:

Shared-only:
http://cr.openjdk.java.net/~rkennke/shenandoah-jdk11u-upstream/webrev.10-shared/
Full:
http://cr.openjdk.java.net/~rkennke/shenandoah-jdk11u-upstream/webrev.10-all/

I did run hotspot_gc_shenandoah again on builds with and without
Shenandoah, and it worked fine.