RFR: JMC-7855: JFR Writer incorrectly uses epoch nanoseconds as the start ticks [v2]

[Christoph Langer]

On Fri, 15 Jul 2022 13:28:05 GMT, Jaroslav Bachorik <jbachorik at openjdk.org> wrote:

>> Currently, the JFR recordings created by the JMC writer will have the start ticks set to the same value as the start timestamp. This will cause problems with the duration calculation which is using directly `System.nanoTime()` and as such the resulting value will be rather nonsensical.
>> 
>> This PR adds two new recording settings - `startTicks` and `duration` which can be used to write a JFR recording with correct values set. If the user does not set those settings `startTicks` will be set to the value of `System.nanoTime()` at the moment the recording is started (a `Recording` instance is created) and `duration` will be computed as a diff between the current value of `System.nanoTime()` and `startTicks`.
>
> Jaroslav Bachorik has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains three additional commits since the last revision:
> 
>  - Update copyrights
>  - Merge branch 'master' into jb/JMC-7855
>  - JMC-7855: JFR Writer incorrectly uses epoch nanoseconds as the start ticks

Hi Jaroslav,

I went through your change. Generally the approach to more correctly calculate start ticks and add a duration field looks ok. Please find some comments inline.

When I discovered that in RecordingImpl:finalizeRecording the calculated duration wasn't passed to writeMetadataEvent, I tried to enhance RecordingImplTest to assert on metadata.duration > -1L. This doesn't work though, because the LEB128Writer would write a -1 as 10 slots of 7 bit but the readVarint() method of RecordingStream only reads 9 slots and will hence just read Long.MAX_VALUE. But when I change readVarint() to read 10 slots, I get other test failures. Your comment also suggests that the JMC parser assumes it is a Java unsigned long. Since I'm not so much into the intrinsics of JFR, I'm not sure what would be correct here.
I also did not review all details of testutils/parser.

Another, more general question which might sound dumb: Where is the flightrecorder.writer component used in JMC itself? Or is it just some auxiliary library used elsewhere? 😄

Best regards
Christoph

core/org.openjdk.jmc.flightrecorder.writer/src/main/java/org/openjdk/jmc/flightrecorder/writer/RecordingImpl.java line 101:

> 99: 	private final MetadataImpl metadata = new MetadataImpl(constantPools);
> 100: 	private final TypesImpl types;
> 101: 	private final long duration;

Maybe move this line to L90?

core/org.openjdk.jmc.flightrecorder.writer/src/main/java/org/openjdk/jmc/flightrecorder/writer/RecordingImpl.java line 291:

> 289: 		writeCheckpointEvent();
> 290: 		long metadataOffset = globalWriter.position();
> 291: 		writeMetadataEvent(duration);

shouldn't it then be recDuration here, too?

core/org.openjdk.jmc.flightrecorder.writer/src/main/java/org/openjdk/jmc/flightrecorder/writer/RecordingSettingsBuilderImpl.java line 42:

> 40: 	private long timestamp = -1;
> 41: 	private long startTicks = -1;
> 42: 

remove empty line?

core/org.openjdk.jmc.flightrecorder.writer/src/main/java/org/openjdk/jmc/flightrecorder/writer/RecordingSettingsBuilderImpl.java line 60:

> 58: 	@Override
> 59: 	public RecordingSettingsBuilder withDuration(long ticks) {
> 60: 		this.duration = duration;

must be ... `withDuration(long duration)` ...

core/org.openjdk.jmc.flightrecorder.writer/src/main/java/org/openjdk/jmc/flightrecorder/writer/api/RecordingSettings.java line 42:

> 40: 	private final long startTimestamp;
> 41: 	private final long startTicks;
> 42: 

remove empty line?

core/org.openjdk.jmc.flightrecorder.writer/src/main/java/org/openjdk/jmc/flightrecorder/writer/api/RecordingSettingsBuilder.java line 57:

> 55: 	 * @since 8.3.0
> 56: 	 */
> 57: 	default RecordingSettingsBuilder withStartTicks(long ticks) {

Do we really need a default method here? That is, could it be that we load an older implementation class of this interface that doesn't have this method implemented?

core/org.openjdk.jmc.flightrecorder.writer/src/main/java/org/openjdk/jmc/flightrecorder/writer/api/RecordingSettingsBuilder.java line 69:

> 67: 	 * @since 8.3.0
> 68: 	 */
> 69: 	default RecordingSettingsBuilder withDuration(long ticks) {

Same here about default...

core/tests/org.openjdk.jmc.flightrecorder.writer.test/pom.xml line 93:

> 91: 			<groupId>org.jctools</groupId>
> 92: 			<artifactId>jctools-core</artifactId>
> 93: 			<version>3.3.0</version>

Can you define the newly introduced dependency versions in [core/tests/pom.xml](https://github.com/openjdk/jmc/blob/master/core/tests/pom.xml) and only inherit here, like you've done it for mockito-junit-jupiter?

core/tests/org.openjdk.jmc.flightrecorder.writer.test/pom.xml line 103:

> 101: 			<groupId>org.slf4j</groupId>
> 102: 			<artifactId>slf4j-simple</artifactId>
> 103: 			<version>1.7.32</version>

Any reason for not using 1.7.36 here as well?

-------------

Changes requested by clanger (Committer).

PR: https://git.openjdk.org/jmc/pull/412