8215175: Inconsistencies in JFR event metadata

Reviewed-by: mgronlun

src/share/classes/jdk/jfr/internal/MetadataHandler.java

@@ -100,7 +100,7 @@ final class MetadataHandler extends DefaultHandler implements EntityResolver {
 
     final Map<String, TypeElement> types = new LinkedHashMap<>(200);
     final Map<String, XmlType> xmlTypes = new HashMap<>(20);
-    final Map<String, AnnotationElement> xmlContentTypes = new HashMap<>(20);
+    final Map<String, List<AnnotationElement>> xmlContentTypes = new HashMap<>(20);
     final List<String> 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<? extends Annotation> 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<AnnotationElement> createAnnotationElements(String annotation) throws InternalError {
+        String[] annotations = annotation.split(",");
+        List<AnnotationElement> 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<? extends Annotation> 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)));

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");

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";

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) {

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) {

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();
     }
 }

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,13 +515,27 @@ 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;
                     }
-                println(bytes);
+                    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;
+                    }
+                }
+            }
         }
         MemoryAddress memoryAddress = field.getAnnotation(MemoryAddress.class);
         if (memoryAddress != null) {
@@ -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;
     }
 

test/jdk/jfr/api/metadata/annotations/TestFormatMissingValue.java

+/*
+ * 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 + "'");
+        }
+    }
+}