JFRTest.jfrOptionsSmokeTest fails on latest GraalVM for JDK 24+24

[jerboaa]

The test fails in CI with:

 Error:  Failures: 
Error:    JFRTest.jfrOptionsSmokeTest:784->jfrOptionsSmoke:913 Command `./target/timezones -XX:+FlightRecorder -XX:StartFlightRecording=maxsize=10000c,filename=logs/flight-native.jfr -XX:FlightRecorderLogging=jfr' output did not match expected pattern `.* Started recording .* \{maxsize=9.8kB.*', it was: ./target/timezones -XX:+FlightRecorder -XX:StartFlightRecording=maxsize=10000c,filename=logs/flight-native.jfr -XX:FlightRecorderLogging=jfr
Command: ./target/timezones -XX:+FlightRecorder -XX:StartFlightRecording=maxsize=10000c,filename=logs/flight-native.jfr -XX:FlightRecorderLogging=jfr
[info][jfr] Added periodic task for EveryChunkPeriodEvents(11644)
[info][jfr] Added periodic task for EndChunkPeriodEvents(11626)
Error: Exception in thread "main" jdk.jfr.internal.dcmd.DCmdException: Parsing error memory size value: invalid value
	at jdk.jfr@24-beta/jdk.jfr.internal.dcmd.AbstractDCmd.execute(AbstractDCmd.java:93)
	at org.graalvm.nativeimage.builder/com.oracle.svm.core.jfr.JfrManager.initRecording(JfrManager.java:176)
	at org.graalvm.nativeimage.builder/com.oracle.svm.core.jfr.JfrManager.lambda$startupHook$0(JfrManager.java:90)
	at org.graalvm.nativeimage.builder/com.oracle.svm.core.jdk.RuntimeSupport.executeHooks(RuntimeSupport.java:169)
	at org.graalvm.nativeimage.builder/com.oracle.svm.core.jdk.RuntimeSupport.initialize(RuntimeSupport.java:100)
	at org.graalvm.nativeimage/org.graalvm.nativeimage.VMRuntime.initialize(VMRuntime.java:65)
	Suppressed: java.lang.IllegalArgumentException: Parsing error memory size value: invalid value
		at jdk.jfr@24-beta/jdk.jfr.internal.dcmd.ArgumentParser.parseMemorySize(ArgumentParser.java:331)
		at jdk.jfr@24-beta/jdk.jfr.internal.dcmd.ArgumentParser.value(ArgumentParser.java:232)
		at jdk.jfr@24-beta/jdk.jfr.internal.dcmd.ArgumentParser.addOption(ArgumentParser.java:147)
		at jdk.jfr@24-beta/jdk.jfr.internal.dcmd.ArgumentParser.parse(ArgumentParser.java:78)
		at jdk.jfr@24-beta/jdk.jfr.internal.dcmd.AbstractDCmd.execute(AbstractDCmd.java:82)
		... 5 more
 ==> expected: <true> but was: <false>

See: https://github.com/graalvm/mandrel/actions/runs/11944256969/job/33295878881#step:11:21640

Reproducible with a JDK 24+24 Linux x86_64 build from temurin and GraalVM master revision 5a5cb09814b17eb4986cc3fb680636fc31a661ba

[jerboaa]

@roberttoyonaga Do you mind taking a look? Thanks!

[roberttoyonaga]

Yes, I'll look into it! At first glance it looks like an issue related to JFR refactoring for the JCMD feature.

[roberttoyonaga]

That seems to be the problem. JFR shouldn't accept "c" as a unit of size. So -XX:StartFlightRecording=maxsize=10000c should fail.
When I try with regular Java it fails too:

rtoyonag@rtoyonag-thinkpadp1gen4i:~/IdeaProjects/graal/substratevm$ $JAVA_HOME/bin/java  -XX:+FlightRecorder -XX:StartFlightRecording=maxsize=10000c  SimpleStandalone
OpenJDK 64-Bit Server VM warning: Option FlightRecorder was deprecated in version 13.0 and will likely be removed in a future release.
[0.062s][error][jfr,startup] Parsing error memory size value: invalid value
Error occurred during initialization of VM
Failure when starting JFR on_create_vm_3

Previous GraalVM versions accepted any string as possible units and defaulted to "bytes" if they weren't recognized [1]. This is because we manually initialized recordings upon start up [2].

But since the new JCMD/Attach-API feature was merged, we now delegate the JFR startup to the JFR DCMD code reused from the JDK [3]. JFR in JDK does is less lenient and throws an error if unrecognized units are used [4]

So to summarize, Native Image previously was more lenient on unrecognized units than OpenJDK, but after the new JCMD changes, the behavior is the same, stricter.

Was there a reason that maxsize=10000c was set with "c" as a unit? If not, I think we should change it to an accepted unit (k, K, m, M, g, G).

[roberttoyonaga]

I think we should change it to an accepted unit (k, K, m, M, g, G).

Actually, we should probably just remove the "c", because the test expects bytes, not KB, MB, etc. [1]

[jerboaa]

I think we should change it to an accepted unit (k, K, m, M, g, G).

Actually, we should probably just remove the "c", because the test expects bytes, not KB, MB, etc. [1]

+1