RFR: JMC-7798: Make the frame type extensible [v2]

[Christoph Langer]

On Tue, 17 May 2022 14:11:43 GMT, Jaroslav Bachorik <jbachorik at openjdk.org> wrote:

>> This change allows the JMC parser users to consume frames with the type set to non-standard values (eg. from async-profiler).
>> 
>> The hard-wired enum approach is replaced with a hybrid solution keeping the standard types in enum-like structure and using a cache for the non-standard types. This allows being fairly flexible when dealing with new frame types while keeping full backward compatibility.
>
> Jaroslav Bachorik has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Fix typo in javadoc

Generally looks good, I have some suggestions.

Also, the copyright years need to be updated in all the files you touched.

core/org.openjdk.jmc.common/src/main/java/org/openjdk/jmc/common/IMCFrame.java line 38:

> 36: 
> 37: import java.util.Objects;
> 38: 

The java.util.Objects import should go before org.openjdk...

core/org.openjdk.jmc.common/src/main/java/org/openjdk/jmc/common/IMCFrame.java line 66:

> 64: 			String key = MSG_PREFIX + this.id;
> 65: 			if (Messages.hasString(key)) {
> 66: 				this.name = Messages.getString(key);

Except for id, you could leave out `this` in the constructor

core/org.openjdk.jmc.common/src/main/java/org/openjdk/jmc/common/IMCFrame.java line 66:

> 64: 			String key = MSG_PREFIX + this.id;
> 65: 			if (Messages.hasString(key)) {
> 66: 				this.name = Messages.getString(key);

Except for id, you could leave out `this`

core/org.openjdk.jmc.common/src/main/java/org/openjdk/jmc/common/IMCFrame.java line 78:

> 76: 		 */
> 77: 		public static final Type JIT_COMPILED = new Type("JIT_COMPILED"); //$NON-NLS-1$
> 78: 		/**

I would like if the static type definitions could move up, directly behind `final class Type {` and were separated by a blank line.

core/org.openjdk.jmc.common/src/main/java/org/openjdk/jmc/common/IMCFrame.java line 78:

> 76: 		 */
> 77: 		public static final Type JIT_COMPILED = new Type("JIT_COMPILED"); //$NON-NLS-1$
> 78: 		/**

I would like if the static type definitions could move up, directly behind `final class Type {` and were separated by a blank line.

core/org.openjdk.jmc.flightrecorder/src/main/java/org/openjdk/jmc/flightrecorder/internal/util/ParserToolkit.java line 89:

> 87: 	 * zero, so the memory overhead of the cache stays negligible.
> 88: 	 */
> 89: 	private static final Map<String, IMCFrame.Type> TYPE_CACHE = new HashMap<>();

I would suggest to move the Type Cache to be part of the Type implementation in IMCFrame.

core/org.openjdk.jmc.flightrecorder/src/main/java/org/openjdk/jmc/flightrecorder/internal/util/ParserToolkit.java line 89:

> 87: 	 * zero, so the memory overhead of the cache stays negligible.
> 88: 	 */
> 89: 	private static final Map<String, IMCFrame.Type> TYPE_CACHE = new HashMap<>();

I would suggest to move the Type Cache to be part of the Type implementation in IMCFrame.

core/tests/org.openjdk.jmc.flightrecorder.test/src/test/java/org/openjdk/jmc/flightrecorder/test/internal/util/ParserToolkitTest.java line 1:

> 1: package org.openjdk.jmc.flightrecorder.test.internal.util;

This file needs a copyright header and it needs to be moved to src/main from src/test as the project structure has been modified a little lately with https://github.com/openjdk/jmc/commit/6aeee1375d8ea5739ba2a85675fb5e00f1eea9af

core/tests/org.openjdk.jmc.flightrecorder.test/src/test/java/org/openjdk/jmc/flightrecorder/test/internal/util/ParserToolkitTest.java line 1:

> 1: package org.openjdk.jmc.flightrecorder.test.internal.util;

This file needs a copyright header and it needs to be moved to src/main from src/test as the project structure has been modified a little lately with https://github.com/openjdk/jmc/commit/6aeee1375d8ea5739ba2a85675fb5e00f1eea9af

core/tests/org.openjdk.jmc.flightrecorder.test/src/test/java/org/openjdk/jmc/flightrecorder/test/internal/util/ParserToolkitTest.java line 13:

> 11: 		Assert.assertTrue(IMCFrame.Type.INTERPRETED == ParserToolkit.parseFrameType(ParserToolkit.INTERPRETED_TYPE_ID));
> 12: 		Assert.assertTrue(
> 13: 				IMCFrame.Type.JIT_COMPILED == ParserToolkit.parseFrameType(ParserToolkit.JIT_COMPILED_TYPE_ID));

Maybe make one line out of L12 and L13 ��

core/tests/org.openjdk.jmc.flightrecorder.test/src/test/java/org/openjdk/jmc/flightrecorder/test/internal/util/ParserToolkitTest.java line 13:

> 11: 		Assert.assertTrue(IMCFrame.Type.INTERPRETED == ParserToolkit.parseFrameType(ParserToolkit.INTERPRETED_TYPE_ID));
> 12: 		Assert.assertTrue(
> 13: 				IMCFrame.Type.JIT_COMPILED == ParserToolkit.parseFrameType(ParserToolkit.JIT_COMPILED_TYPE_ID));

Maybe make one line out of L12 and L13 ��

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

PR: https://git.openjdk.java.net/jmc/pull/401