RFR: 8251354: Shenandoah: Fix jdk/jfr/tool/TestPrintJSON.java test failure

[Thomas Stüfe]

Hi Roman,

On Mon, Aug 10, 2020 at 5:28 PM Roman Kennke <rkennke at redhat.com> wrote:

> Please review the following issue and fix. This is a 11u-only and
> Shenandoah-only bug, but affects shared code.
>
> With the recent Shenandoah backport to jdk11u, we decided to optionally
> split metadata.xml so that Shenandoah can provide its own metadata-
> shenandoah.xml in a separate file. This avoids generating code for
> Shenandoah when Shenandoah is not present in the build.
>
> However, the metadata.xml is also read at run-time,


by whom?


> but metadata-
> shenandoah.xml is not present at run-time, nor would it be read with
> the current implementation of MetadataHandler.java. This makes the test
> jdk/jfr/tool/TestPrintJSON.java fail.
>
> I propose to revert the parts of the Shenandoah patch that allows
> breaking-up metadata.xml. The downside is that it spills a tiny amount
> of (empty) Shenandoah-related code in builds without Shenandoah.



> However, it is the exact same thing that also happens for any other GC.

On the positive side, this seems the most conservative approach because
> that is what has been tested for a very long time before we integrated
> Shenandoah into 11u.
>
> Alternatively, we could further extend the makefile machinery and the
> MetadataHandler.java to also (optionally) read metadata-shenandoah.xml
> at run-time. It would have the advantage of cleanliness in non-
> Shenandoah builds, and the disadvangate of further divergence from
> upstream jdk16, and risk of rather untested code, and uncertainty
> whether or not this would be the last such issue.
>
> Bug:https://bugs.openjdk.java.net/browse/JDK-8251354
> Webrev: http://cr.openjdk.java.net/~rkennke/JDK-8251354/webrev.00/
> Testing: build with and without Shenandoah, JFR tests with and without
> Shenandoah
>
> What do you think?
>

Had a look at the fix, and it looks reasonable to me. To me this minimal
shenandoah-related tainting of shared code seems acceptable. You guys went
through great lengths keeping out of shared code, I think this is fine.


>
> Roman
>
>
..Thomas