On Wed, 24 May 2023 13:46:59 GMT, Erik Gahlin <egahlin@openjdk.org> wrote:

...

Could I have a review of an enhancement that adds a view command to jfr.

Testing: tier1, tier2 + jdk/jdk/jfr

For the change to work properly when streaming, fix of 8307738 needs to be applied.

To simplify the review, changes not relevant to the feature, but that can use classes in jdk.jfr.internal.util package, will be refactored after this change has been integrated. Possibly in JDK 22.

Thanks Erik

Erik Gahlin has updated the pull request incrementally with two additional commits since the last revision:

- Update comments and remove resetCache() duplicate - Update

src/jdk.jfr/share/classes/jdk/jfr/internal/query/FieldFormatter.java line 94:

92: } 93: if (object instanceof RecordedClass clazz) { 94: String text = ValueFormatter.formatClass(clazz);

Suggestion: String text = ValueFormatter.formatClass(clazz); src/jdk.jfr/share/classes/jdk/jfr/internal/query/Function.java line 38:

36: abstract class Function { 37: 38: abstract public void add(Object value);

let's use blessed modifiers order Suggestion: public abstract void add(Object value); src/jdk.jfr/share/classes/jdk/jfr/internal/query/QueryParser.java line 230:

228: private String text() throws ParseException { 229: if (tokenizer.peekChar() != '\'') { 230: throw new ParseException("Expected text to start with a single quote character", position());

Suggestion: throw new ParseException("Expected text to start with a single quote character", position()); src/jdk.jfr/share/classes/jdk/jfr/internal/query/QueryParser.java line 270:

268: private Property property() throws ParseException { 269: String text = tokenizer.next(); 270: Consumer<Field> style = switch (text.toLowerCase()) {

Suggestion: Consumer<Field> style = switch (text.toLowerCase()) { src/jdk.jfr/share/classes/jdk/jfr/internal/query/QueryParser.java line 333:

331: 332: @Override 333: public void close() throws ParseException {

Suggestion: public void close() throws ParseException { src/jdk.jfr/share/classes/jdk/jfr/internal/query/QueryPrinter.java line 167:

165: out.println(); 166: for (ValueDescriptor f : type.getFields()) { 167: String typeName = Utils.makeSimpleName(f.getTypeName());

Suggestion: String typeName = Utils.makeSimpleName(f.getTypeName()); src/jdk.jfr/share/classes/jdk/jfr/internal/util/ValueFormatter.java line 62:

60: if (roundedDuration.equals(Duration.ZERO)) { 61: return "0 s"; 62: } else if(roundedDuration.isNegative()){

Suggestion: } else if (roundedDuration.isNegative()){ src/jdk.jfr/share/classes/jdk/jfr/internal/util/ValueFormatter.java line 73:

71: // 0.000001 ms - 0.000999 ms 72: double outputMs = (double) d.toNanosPart() / 1_000_000; 73: return String.format("%.6f ms", outputMs);

Suggestion: return String.format("%.6f ms", outputMs); src/jdk.jfr/share/classes/jdk/jfr/internal/util/ValueFormatter.java line 79:

77: int outputDigit = NANO_SIGNIFICANT_FIGURES - valueLength; 78: double outputMs = (double) d.toNanosPart() / 1_000_000; 79: return String.format("%." + outputDigit + "f ms", outputMs);

Suggestion: return String.format("%." + outputDigit + "f ms", outputMs); src/jdk.jfr/share/classes/jdk/jfr/internal/util/ValueFormatter.java line 85:

83: int outputDigit = MILL_SIGNIFICANT_FIGURES - valueLength; 84: double outputSecond = d.toSecondsPart() + (double) d.toMillisPart() / 1_000; 85: return String.format("%." + outputDigit + "f s", outputSecond);

Suggestion: return String.format("%." + outputDigit + "f s", outputSecond); src/jdk.jfr/share/classes/jdk/jfr/internal/util/ValueFormatter.java line 88:

86: } else if (d.compareTo(HOUR) < 0) { 87: // 1 m 0 s - 59 m 59 s 88: return String.format("%d m %d s", d.toMinutesPart(), d.toSecondsPart());

Suggestion: return String.format("%d m %d s", d.toMinutesPart(), d.toSecondsPart()); src/jdk.jfr/share/classes/jdk/jfr/internal/util/ValueFormatter.java line 91:

89: } else if (d.compareTo(DAY) < 0) { 90: // 1 h 0 m - 23 h 59 m 91: return String.format("%d h %d m", d.toHoursPart(), d.toMinutesPart());

Suggestion: return String.format("%d h %d m", d.toHoursPart(), d.toMinutesPart()); src/jdk.jfr/share/classes/jdk/jfr/internal/util/ValueFormatter.java line 94:

92: } else { 93: // 1 d 0 h - 94: return String.format("%d d %d h", d.toDaysPart(), d.toHoursPart());

Suggestion: return String.format("%d d %d h", d.toDaysPart(), d.toHoursPart()); src/jdk.jfr/share/classes/jdk/jfr/internal/util/ValueFormatter.java line 105:

103: if (d.equals(Duration.ZERO)) { 104: return d; 105: } else if(d.isNegative()){

Suggestion: } else if (d.isNegative()){ test/jdk/jdk/jfr/tool/TestView.java line 59:

57: output.shouldContain("could not open file "); 58: 59: Path file = Utils.createTempFile("faked-file", ".jfr");

Suggestion: Path file = Utils.createTempFile("faked-file", ".jfr"); ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/14104#discussion_r1204551894 PR Review Comment: https://git.openjdk.org/jdk/pull/14104#discussion_r1204552796 PR Review Comment: https://git.openjdk.org/jdk/pull/14104#discussion_r1204548720 PR Review Comment: https://git.openjdk.org/jdk/pull/14104#discussion_r1204548978 PR Review Comment: https://git.openjdk.org/jdk/pull/14104#discussion_r1204549215 PR Review Comment: https://git.openjdk.org/jdk/pull/14104#discussion_r1204552179 PR Review Comment: https://git.openjdk.org/jdk/pull/14104#discussion_r1204549570 PR Review Comment: https://git.openjdk.org/jdk/pull/14104#discussion_r1204549775 PR Review Comment: https://git.openjdk.org/jdk/pull/14104#discussion_r1204550437 PR Review Comment: https://git.openjdk.org/jdk/pull/14104#discussion_r1204550910 PR Review Comment: https://git.openjdk.org/jdk/pull/14104#discussion_r1204551139 PR Review Comment: https://git.openjdk.org/jdk/pull/14104#discussion_r1204551400 PR Review Comment: https://git.openjdk.org/jdk/pull/14104#discussion_r1204551505 PR Review Comment: https://git.openjdk.org/jdk/pull/14104#discussion_r1204551639 PR Review Comment: https://git.openjdk.org/jdk/pull/14104#discussion_r1204546888

On Thu, 25 May 2023 10:34:45 GMT, Erik Gahlin <egahlin@openjdk.org> wrote:

...

Could I have a review of an enhancement that adds a view command to jfr.

Testing: tier1, tier2 + jdk/jdk/jfr

For the change to work properly when streaming, fix of 8307738 needs to be applied.

To simplify the review, changes not relevant to the feature, but that can use classes in jdk.jfr.internal.util package, will be refactored after this change has been integrated. Possibly in JDK 22.

Thanks Erik

Erik Gahlin has updated the pull request incrementally with one additional commit since the last revision:

Updates

This is great work Erik! It is going to help folks better understand what is important, and the extension capabilities are numerous. Thank you very much! ------------- Marked as reviewed by mgronlun (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/14104#pullrequestreview-1443769068