RFR (11u, XXL): Upstream/backport Shenandoah to JDK11u
Roman Kennke
rkennke at redhat.com
Thu Jul 23 17:17:00 UTC 2020
Thanks for the reviews, Kelvin!
Cheers,
Roman
>
> On 7/23/20, 5:51 AM, "Roman Kennke" <rkennke at redhat.com> wrote:
>
> 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 :-)
>
> Would have been much better, I realize. There were some email server
> problems going on and I never received that original message.
>
> > 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.
>
> Thanks. I'm satisfied.
>
> > 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").
>
> Thanks. I'm satisfied.
>
> > 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?
>
> I see now. My side-by-side diffs were not showing me the condition
> that controlled the block newly terminated by the inserted
> #endif. Thanks for clarifying. I'm satisfied.
>
> > 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.
>
> Satisfied now that I understand it better.
>
> > 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.
>
> Thanks. I'm satisfied. (Above, I think you mean webrev.10.)
>
> > 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).
>
> I see. Thanks. I'm satisfied.
>
> > 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.
>
> Thanks. Am satisfied with webrev.10.
>
> > 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?
>
> I see. Am satisfied.
>
> > 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.
>
> Thanks for explaining. I am satisfied.
>
> > 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
>
> Ok. I was concerned that this might be guarding other tests that did
> not depend on Shenandoah. Thanks for explaining. I am satisfied.
>
> > 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.
>
> Thanks for generating the new webrev. Looks good to me.
>
> 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.
>
>
>
>
More information about the jdk-updates-dev
mailing list