From 1e18d7a173e4e85fcc3457d0f4547ef97d701574 Mon Sep 17 00:00:00 2001 From: egahlin Date: Thu, 13 Dec 2018 14:21:04 +0100 Subject: [PATCH] 8215175: Inconsistencies in JFR event metadata Reviewed-by: mgronlun --- .../jdk/jfr/internal/MetadataHandler.java | 32 ++++- .../jdk/jfr/internal/PlatformRecording.java | 2 +- src/share/classes/jdk/jfr/internal/Utils.java | 49 ++++++- .../jdk/jfr/internal/dcmd/AbstractDCmd.java | 6 +- .../jdk/jfr/internal/dcmd/DCmdCheck.java | 3 +- .../jdk/jfr/internal/dcmd/DCmdConfigure.java | 8 +- .../jdk/jfr/internal/tool/PrettyWriter.java | 44 +++++-- .../annotations/TestFormatMissingValue.java | 124 ++++++++++++++++++ 8 files changed, 235 insertions(+), 33 deletions(-) create mode 100644 test/jdk/jfr/api/metadata/annotations/TestFormatMissingValue.java diff --git a/src/share/classes/jdk/jfr/internal/MetadataHandler.java b/src/share/classes/jdk/jfr/internal/MetadataHandler.java index c5387f04e..831b4444c 100644 --- a/src/share/classes/jdk/jfr/internal/MetadataHandler.java +++ b/src/share/classes/jdk/jfr/internal/MetadataHandler.java @@ -100,7 +100,7 @@ final class MetadataHandler extends DefaultHandler implements EntityResolver { final Map types = new LinkedHashMap<>(200); final Map xmlTypes = new HashMap<>(20); - final Map xmlContentTypes = new HashMap<>(20); + final Map> xmlContentTypes = new HashMap<>(20); final List relations = new ArrayList<>(); long eventTypeId = 255; long structTypeId = 33; @@ -148,11 +148,8 @@ final class MetadataHandler extends DefaultHandler implements EntityResolver { break; case "XmlContentType": String name = attributes.getValue("name"); - String type = attributes.getValue("annotationType"); - String value = attributes.getValue("annotationValue"); - Class annotationType = createAnnotationClass(type); - AnnotationElement ae = value == null ? new AnnotationElement(annotationType) : new AnnotationElement(annotationType, value); - xmlContentTypes.put(name, ae); + String annotation = attributes.getValue("annotation"); + xmlContentTypes.put(name, createAnnotationElements(annotation)); break; case "Relation": String n = attributes.getValue("name"); @@ -161,6 +158,27 @@ final class MetadataHandler extends DefaultHandler implements EntityResolver { } } + private List createAnnotationElements(String annotation) throws InternalError { + String[] annotations = annotation.split(","); + List annotationElements = new ArrayList<>(); + for (String a : annotations) { + a = a.trim(); + int leftParenthesis = a.indexOf("("); + if (leftParenthesis == -1) { + annotationElements.add(new AnnotationElement(createAnnotationClass(a))); + } else { + int rightParenthesis = a.lastIndexOf(")"); + if (rightParenthesis == -1) { + throw new InternalError("Expected closing parenthesis for 'XMLContentType'"); + } + String value = a.substring(leftParenthesis + 1, rightParenthesis); + String type = a.substring(0, leftParenthesis); + annotationElements.add(new AnnotationElement(createAnnotationClass(type), value)); + } + } + return annotationElements; + } + @SuppressWarnings("unchecked") private Class createAnnotationClass(String type) { try { @@ -255,7 +273,7 @@ final class MetadataHandler extends DefaultHandler implements EntityResolver { aes.add(new AnnotationElement(Unsigned.class)); } if (f.contentType != null) { - aes.add(Objects.requireNonNull(xmlContentTypes.get(f.contentType))); + aes.addAll(Objects.requireNonNull(xmlContentTypes.get(f.contentType))); } if (f.relation != null) { aes.add(Objects.requireNonNull(relationMap.get(f.relation))); diff --git a/src/share/classes/jdk/jfr/internal/PlatformRecording.java b/src/share/classes/jdk/jfr/internal/PlatformRecording.java index 98cfc52ca..2d908d73c 100644 --- a/src/share/classes/jdk/jfr/internal/PlatformRecording.java +++ b/src/share/classes/jdk/jfr/internal/PlatformRecording.java @@ -123,7 +123,7 @@ public final class PlatformRecording implements AutoCloseable { options.add("maxage=" + Utils.formatTimespan(maxAge, "")); } if (maxSize != 0) { - options.add("maxsize=" + Utils.formatBytes(maxSize, "")); + options.add("maxsize=" + Utils.formatBytesCompact(maxSize)); } if (dumpOnExit) { options.add("dumponexit=true"); diff --git a/src/share/classes/jdk/jfr/internal/Utils.java b/src/share/classes/jdk/jfr/internal/Utils.java index bcdff36ee..9c2a858fb 100644 --- a/src/share/classes/jdk/jfr/internal/Utils.java +++ b/src/share/classes/jdk/jfr/internal/Utils.java @@ -103,18 +103,53 @@ public final class Utils { } } - public static String formatBytes(long bytes, String separation) { - if (bytes == 1) { - return "1 byte"; - } + // Tjis method can't handle Long.MIN_VALUE because absolute value is negative + private static String formatDataAmount(String formatter, long amount) { + int exp = (int) (Math.log(Math.abs(amount)) / Math.log(1024)); + char unitPrefix = "kMGTPE".charAt(exp - 1); + return String.format(formatter, amount / Math.pow(1024, exp), unitPrefix); + } + + public static String formatBytesCompact(long bytes) { if (bytes < 1024) { + return String.valueOf(bytes); + } + return formatDataAmount("%.1f%cB", bytes); + } + + public static String formatBits(long bits) { + if (bits == 1 || bits == -1) { + return bits + " bit"; + } + if (bits < 1024 && bits > -1024) { + return bits + " bits"; + } + return formatDataAmount("%.1f %cbit", bits); + } + + public static String formatBytes(long bytes) { + if (bytes == 1 || bytes == -1) { + return bytes + " byte"; + } + if (bytes < 1024 && bytes > -1024) { return bytes + " bytes"; } - int exp = (int) (Math.log(bytes) / Math.log(1024)); - char bytePrefix = "kMGTPE".charAt(exp - 1); - return String.format("%.1f%s%cB", bytes / Math.pow(1024, exp), separation, bytePrefix); + return formatDataAmount("%.1f %cB", bytes); + } + + public static String formatBytesPerSecond(long bytes) { + if (bytes < 1024 && bytes > -1024) { + return bytes + " byte/s"; + } + return formatDataAmount("%.1f %cB/s", bytes); } + public static String formatBitsPerSecond(long bits) { + if (bits < 1024 && bits > -1024) { + return bits + " bps"; + } + return formatDataAmount("%.1f %cbps", bits); + } public static String formatTimespan(Duration dValue, String separation) { if (dValue == null) { return "0"; diff --git a/src/share/classes/jdk/jfr/internal/dcmd/AbstractDCmd.java b/src/share/classes/jdk/jfr/internal/dcmd/AbstractDCmd.java index 15549c400..030a97a1b 100644 --- a/src/share/classes/jdk/jfr/internal/dcmd/AbstractDCmd.java +++ b/src/share/classes/jdk/jfr/internal/dcmd/AbstractDCmd.java @@ -109,7 +109,7 @@ abstract class AbstractDCmd { try { print(" "); long bytes = SecuritySupport.getFileSize(file); - printBytes(bytes, " "); + printBytes(bytes); } catch (IOException e) { // Ignore, not essential } @@ -152,8 +152,8 @@ abstract class AbstractDCmd { println(); } - protected final void printBytes(long bytes, String separation) { - print(Utils.formatBytes(bytes, separation)); + protected final void printBytes(long bytes) { + print(Utils.formatBytes(bytes)); } protected final void printTimespan(Duration timespan, String separator) { diff --git a/src/share/classes/jdk/jfr/internal/dcmd/DCmdCheck.java b/src/share/classes/jdk/jfr/internal/dcmd/DCmdCheck.java index 78f4076fd..20c43fd70 100644 --- a/src/share/classes/jdk/jfr/internal/dcmd/DCmdCheck.java +++ b/src/share/classes/jdk/jfr/internal/dcmd/DCmdCheck.java @@ -39,6 +39,7 @@ import jdk.jfr.SettingDescriptor; import jdk.jfr.internal.LogLevel; import jdk.jfr.internal.LogTag; import jdk.jfr.internal.Logger; +import jdk.jfr.internal.Utils; /** * JFR.check - invoked from native @@ -117,7 +118,7 @@ final class DCmdCheck extends AbstractDCmd { long maxSize = recording.getMaxSize(); if (maxSize != 0) { print(" maxsize="); - printBytes(maxSize, ""); + print(Utils.formatBytesCompact(maxSize)); } Duration maxAge = recording.getMaxAge(); if (maxAge != null) { diff --git a/src/share/classes/jdk/jfr/internal/dcmd/DCmdConfigure.java b/src/share/classes/jdk/jfr/internal/dcmd/DCmdConfigure.java index 079ede60d..557128f63 100644 --- a/src/share/classes/jdk/jfr/internal/dcmd/DCmdConfigure.java +++ b/src/share/classes/jdk/jfr/internal/dcmd/DCmdConfigure.java @@ -193,25 +193,25 @@ final class DCmdConfigure extends AbstractDCmd { private void printGlobalBufferSize() { print("Global buffer size: "); - printBytes(Options.getGlobalBufferSize(), " "); + printBytes(Options.getGlobalBufferSize()); println(); } private void printThreadBufferSize() { print("Thread buffer size: "); - printBytes(Options.getThreadBufferSize(), " "); + printBytes(Options.getThreadBufferSize()); println(); } private void printMemorySize() { print("Memory size: "); - printBytes(Options.getMemorySize(), " "); + printBytes(Options.getMemorySize()); println(); } private void printMaxChunkSize() { print("Max chunk size: "); - printBytes(Options.getMaxChunkSize(), " "); + printBytes(Options.getMaxChunkSize()); println(); } } diff --git a/src/share/classes/jdk/jfr/internal/tool/PrettyWriter.java b/src/share/classes/jdk/jfr/internal/tool/PrettyWriter.java index bb1de603a..107d904ad 100644 --- a/src/share/classes/jdk/jfr/internal/tool/PrettyWriter.java +++ b/src/share/classes/jdk/jfr/internal/tool/PrettyWriter.java @@ -315,11 +315,7 @@ public final class PrettyWriter extends EventPrintWriter { printArray((Object[]) value); return; } - if (field.getContentType() != null) { - if (printFormatted(field, value)) { - return; - } - } + if (value instanceof Double) { Double d = (Double) value; if (Double.isNaN(d) || d == Double.NEGATIVE_INFINITY) { @@ -349,6 +345,12 @@ public final class PrettyWriter extends EventPrintWriter { } } + if (field.getContentType() != null) { + if (printFormatted(field, value)) { + return; + } + } + String text = String.valueOf(value); if (value instanceof String) { text = "\"" + text + "\""; @@ -472,7 +474,7 @@ public final class PrettyWriter extends EventPrintWriter { private boolean printFormatted(ValueDescriptor field, Object value) { if (value instanceof Duration) { Duration d = (Duration) value; - if (d.getSeconds() == Long.MIN_VALUE) { + if (d.getSeconds() == Long.MIN_VALUE && d.getNano() == 0) { println("N/A"); return true; } @@ -513,12 +515,26 @@ public final class PrettyWriter extends EventPrintWriter { if (dataAmount != null) { if (value instanceof Number) { Number n = (Number) value; - String bytes = Utils.formatBytes(n.longValue(), " "); + long amount = n.longValue(); if (field.getAnnotation(Frequency.class) != null) { - bytes += "/s"; + if (dataAmount.value().equals(DataAmount.BYTES)) { + println(Utils.formatBytesPerSecond(amount)); + return true; + } + if (dataAmount.value().equals(DataAmount.BITS)) { + println(Utils.formatBitsPerSecond(amount)); + return true; + } + } else { + if (dataAmount.value().equals(DataAmount.BYTES)) { + println(Utils.formatBytes(amount)); + return true; + } + if (dataAmount.value().equals(DataAmount.BITS)) { + println(Utils.formatBits(amount)); + return true; + } } - println(bytes); - return true; } } MemoryAddress memoryAddress = field.getAnnotation(MemoryAddress.class); @@ -529,6 +545,14 @@ public final class PrettyWriter extends EventPrintWriter { return true; } } + Frequency frequency = field.getAnnotation(Frequency.class); + if (frequency != null) { + if (value instanceof Number) { + println(value + " Hz"); + return true; + } + } + return false; } diff --git a/test/jdk/jfr/api/metadata/annotations/TestFormatMissingValue.java b/test/jdk/jfr/api/metadata/annotations/TestFormatMissingValue.java new file mode 100644 index 000000000..a9335cac6 --- /dev/null +++ b/test/jdk/jfr/api/metadata/annotations/TestFormatMissingValue.java @@ -0,0 +1,124 @@ +/* + * Copyright (c) 2015, 2018, Oracle and/or its affiliates. All rights reserved. + * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. + * + * This code is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License version 2 only, as + * published by the Free Software Foundation. Oracle designates this + * particular file as subject to the "Classpath" exception as provided + * by Oracle in the LICENSE file that accompanied this code. + * + * This code is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License + * version 2 for more details (a copy is included in the LICENSE file that + * accompanied this code). + * + * You should have received a copy of the GNU General Public License version + * 2 along with this work; if not, write to the Free Software Foundation, + * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA. + * + * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA + * or visit www.oracle.com if you need additional information or have any + * questions. + */ + +package jdk.jfr.api.metadata.annotations; + +import jdk.jfr.DataAmount; +import jdk.jfr.Event; +import jdk.jfr.Frequency; +import jdk.jfr.Recording; +import jdk.jfr.StackTrace; +import jdk.jfr.consumer.RecordedEvent; +import jdk.test.lib.Asserts; +import jdk.test.lib.jfr.Events; + +/** + * @test + * @key jfr + * @summary Check that event values are properly formatted and sanity check + * that extreme values don't throws exceptions + * @library /lib / + * @run main/othervm jdk.jfr.api.metadata.annotations.TestFormatMissingValue + */ +public class TestFormatMissingValue { + + @StackTrace(false) + static class MultiContentTypeEvent extends Event { + @DataAmount(DataAmount.BYTES) + @Frequency + long a = Long.MIN_VALUE; + + @DataAmount(DataAmount.BYTES) + @Frequency + long b = Long.MAX_VALUE; + + @DataAmount(DataAmount.BYTES) + @Frequency + int c = Integer.MIN_VALUE; + + @DataAmount(DataAmount.BYTES) + @Frequency + int d = Integer.MAX_VALUE; + + @DataAmount(DataAmount.BYTES) + @Frequency + double e = Double.NEGATIVE_INFINITY; + + @DataAmount(DataAmount.BYTES) + @Frequency + double f = Double.POSITIVE_INFINITY; + + @DataAmount(DataAmount.BYTES) + @Frequency + double g = Double.NaN; + + @DataAmount(DataAmount.BYTES) + @Frequency + float h = Float.NEGATIVE_INFINITY; + + @DataAmount(DataAmount.BYTES) + @Frequency + float i = Float.POSITIVE_INFINITY; + + @DataAmount(DataAmount.BYTES) + @Frequency + float j = Float.NaN; + } + + public static void main(String[] args) throws Exception { + try (Recording r = new Recording()) { + r.start(); + MultiContentTypeEvent m = new MultiContentTypeEvent(); + m.commit(); + r.stop(); + for (RecordedEvent e : Events.fromRecording(r)) { + String t = e.toString(); + assertContains(t, "a = N/A"); + assertContains(t, "c = N/A"); + assertContains(t, "e = N/A"); + assertContains(t, "g = N/A"); + assertContains(t, "h = N/A"); + assertContains(t, "j = N/A"); + + assertNotContains(t, "b = N/A"); + assertNotContains(t, "d = N/A"); + assertNotContains(t, "f = N/A"); + assertNotContains(t, "i = N/A"); + } + } + } + + private static void assertContains(String t, String text) { + if (!t.contains(text)) { + Asserts.fail("Expected '" + t + "' to contain text '" + text + "'"); + } + } + + private static void assertNotContains(String t, String text) { + if (t.contains(text)) { + Asserts.fail("Found unexpected value '" + text + "' in text '" + t + "'"); + } + } +} -- GitLab