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

[Roman Kennke]

On Mon, 2020-08-10 at 19:40 +0200, Thomas Stüfe wrote:
> 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?
>  

src/jdk.jfr/share/classes/jdk/jfr/internal/MetadataHandler.java
> > 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.

Ok, thanks for the review.

It is a bit ironic that it was actually all that work to make it
squeaky-clean that caused this regression. ;-)

Thanks,
Roman