RFR: JMC-7798: Make the frame type extensible [v2]
Christoph Langer
clanger at openjdk.java.net
Thu May 19 12:33:09 UTC 2022
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
More information about the jmc-dev
mailing list