Uploaded image for project: 'Minecraft: Java Edition'
  1. Minecraft: Java Edition
  2. MC-236873

Problems with JFR event metadata

    XMLWordPrintable

    Details

    • Type: Bug
    • Status: Resolved
    • Resolution: Fixed
    • Affects Version/s: 21w37a
    • Fix Version/s: 21w38a
    • Labels:
      None
    • Confirmation Status:
      Plausible
    • Category:
      (Unassigned)

      Description

      The snapshot 21w37a of Minecraft adds user defined JFR events. This is really cool!

      I work on OpenJDK and JDK Flight Recorder, so I'm excited to see it being used in Minecraft. I will try to use recordings from Minecraft in demos.

      I thought I bring some feedback when it comes to the event metadata.

      The event "minecraft.ServerTickTime" has a field called averageTickMs of type float, with the annotation @Timespan(Timespan.MILLISECONDS).

      Unfortunately, this doesn't work well with the API to consume JFR recordings, in particular the jdk.jfr.RecordedObject::getDuration(String) method, which can only operate on fields of integral data types. Float values may become too large to fit into a java.time.Duration object, which uses a long for seconds internally, so we can't allow this.

      This can be seen when using the JAVA_HOME/bin/jfr tool:

      $ jfr print minecraft.jfr

      Which will result in an IllegalArgumentException.  

      (This doesn't happen in JDK Mission Control, because it doesn't use the parser that comes with the JDK as it needs to be backward compatible with old versions of the Oracle JDK. JMC converts timespans to nanos since 1970 and uses long values internally, which has its own limitations, but works most of the time.)

      The easiest way to fix this is to take the value and multiply it by 1_000_000 and cast it to a long and use @Timespan(Timespan.NANOSECONDS).

      If you are poking around in JFR the code, you may consider the following changes to make event metadata consistent with other events and more in line with recommendations [1][2]

      ------------------------------------------------------------
      Event: minecraft.ChunkGeneration:

      Label("Chunk generation duration") -> Label("Chunk Generation")

      Label("First block x world position") -> Label("First Block X World Position")
      long worldPosX -> long worldPositionX;
      ...

      Label("Chunk x position") -> Label("Chunk X Position") 
      int chunkPosX -> int chunkPositionX;
      ...

      We have tried to avoid abbreviation, such as Info, Alloc, Pos in field names. Labels should use headline capitalization. 

      Add Label("Status")
      Add Label("Level")
      Add Label("Success")

      JMC creates a label from the field name, if it is missing, but that is not the case with the API in the JDK as it doesn't work well in some locales.

      ------------------------------------------------------------

      Event: minecraft.PacketRead / minecraft.PacketSend

       Label("Packet name") -> Label("Packet Name)
       Label("Remote address") -> Label("Remote Address")

      ------------------------------------------------------------
      Event: minecraft.ServerTickTime:

      Label("Server tick time") ->Label("Server Tick Time")
      Label("Average server tick time (ms)") -> Label("Average Server Tick Time")

      @Timespan("MILLISECONDS") -> @Timespan("NANOSECONDS")
      float averageTickMs -> long averageTick

      There is no need to include the unit in the name, if it has been annotated with a timespan.

      ------------------------------------------------------------

      Event: minecraft.WorldLoadFinishedEvent

      Name("minecraft.WorldLoadFinishedEvent") -> Name("minecraft.LoadWorld")

      Label("World create/load duration") -> Label("Create/Load World")

      Avoid using the word "Event" in the name.

      [1] [https://docs.oracle.com/en/java/javase/16/docs/api/jdk.jfr/jdk/jfr/Label.html

      [2] https://docs.oracle.com/en/java/javase/16/docs/api/jdk.jfr/jdk/jfr/Name.html

        Attachments

          Activity

            People

            Assignee:
            billy.sjoberg [Mojang] Billy Sjöberg
            Reporter:
            egahlin Erik Gahlin
            Votes:
            2 Vote for this issue
            Watchers:
            1 Start watching this issue

              Dates

              Created:
              Updated:
              Resolved:
              CHK: