RFR: 8306703: JFR: Summary views [v2]

Andrey Turbanov aturbanov at openjdk.org
Wed May 24 17:37:12 UTC 2023 

On Wed, 24 May 2023 13:46:59 GMT, Erik Gahlin <egahlin at 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

More information about the hotspot-jfr-dev mailing list