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