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

[Roman Kennke]

Hi Christoph,

yes, the big Shenandoah patch caused a several JFR tests to fail
(that's why I changed the bug synopsis), and I verified that this patch
fixes them all. I've run jdk/jfr tests in Shenandoah disabled builds
with no failures, and jdk/jfr tests in Shenandoah enabled builds with
and without Shenandoah, without failures. We should be good.

Thanks for the review!

Cheers,
Roman


On Fri, 2020-08-14 at 04:30 +0000, Langer, Christoph wrote:
> Hi Roman,
> 
> I think your proposed change is ok.
> 
> While building without Shenandoah, we see an error in test
> jdk/jfr/event/metadata/TestDefaultConfigurations.java.
> 
> This is the log:
> java.lang.Exception: Preset 'default' reference unknown event
> 'jdk.ShenandoahHeapRegionInformation'
> Preset 'default' reference unknown event
> 'jdk.ShenandoahHeapRegionStateChange'
> Preset 'profile' reference unknown event
> 'jdk.ShenandoahHeapRegionInformation'
> Preset 'profile' reference unknown event
> 'jdk.ShenandoahHeapRegionStateChange'
> 
> 	at
> jdk.jfr.event.metadata.TestDefaultConfigurations.throwExceptionWithEr
> rors(TestDefaultConfigurations.java:117)
> 	at
> jdk.jfr.event.metadata.TestDefaultConfigurations.main(TestDefaultConf
> igurations.java:78)
> 	at
> java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Nativ
> e Method)
> 	at
> java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(Native
> MethodAccessorImpl.java:62)
> 	at
> java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(De
> legatingMethodAccessorImpl.java:43)
> 	at java.base/java.lang.reflect.Method.invoke(Method.java:566)
> 	at
> com.sun.javatest.regtest.agent.MainWrapper$MainThread.run(MainWrapper
> .java:127)
> 	at java.base/java.lang.Thread.run(Thread.java:834)
> 
> JavaTest Message: Test threw exception: java.lang.Exception: Preset
> 'default' reference unknown event
> 'jdk.ShenandoahHeapRegionInformation'
> Preset 'default' reference unknown event
> 'jdk.ShenandoahHeapRegionStateChange'
> Preset 'profile' reference unknown event
> 'jdk.ShenandoahHeapRegionInformation'
> Preset 'profile' reference unknown event
> 'jdk.ShenandoahHeapRegionStateChange'
> 
> I guess your change introduces these events for builds with
> Shenandoah turned off, so I hope this is fixed then, too ��
> 
> Best regards
> Christoph
> 
> > -----Original Message-----
> > From: jdk-updates-dev <jdk-updates-dev-retn at openjdk.java.net> On
> > Behalf Of Roman Kennke
> > Sent: Montag, 10. August 2020 17:28
> > To: jdk-updates-dev <jdk-updates-dev at openjdk.java.net>
> > Subject: RFR: 8251354: Shenandoah: Fix
> > jdk/jfr/tool/TestPrintJSON.java test
> > failure
> > 
> > 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, 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?
> > 
> > Roman