Skip to content

Conversation

jovanstevanovic
Copy link
Member

@jovanstevanovic jovanstevanovic commented Aug 25, 2021

This PR tries to solve multiple problems in current JFR support.

First, it introduces support for Thread and ThreadGroup repositories.

Before:

  <struct name="eventThread" xsi:nil="true"/>

After:

  <struct name="eventThread">
    <value name="osName">main</value>
    <value name="osThreadId">1</value>
    <value name="javaName">main</value>
    <value name="javaThreadId">1</value>
    <struct name="group">
      <struct name="parent">
        <struct name="parent" xsi:nil="true"/>
        <value name="name">system</value>
      </struct>
      <value name="name">main</value>
    </struct>
  </struct>

Second, it is trying to solve a problem with a bad event signature. For example:

@name("jdk.OSInformation")
@Label("OS Information")
@category("Operating System")
@name("jdk.OSInformation")
class OSInformation extends jdk.jfr.Event {
@Label("Start Time")
@timestamp("TICKS")
long startTime;

@Label("Duration")
@timespan("TICKS")
long duration;

@Label("Event Thread")
@description("Thread in which event was committed in")
Thread eventThread;

@Label("Stack Trace")
@description("Stack Trace starting from the method the event was committed in")
StackTrace stackTrace;

@Label("OS Version")
String osVersion;
}

This is a signature of OSInformation on SVM. In order to make this event work, it should have the same signature as the event on JVM (below).

@name("jdk.OSInformation")
@category("Operating System")
@Label("OS Information")
class OSInformation extends jdk.jfr.Event {
@Label("Start Time")
@timestamp("TICKS")
long startTime;

@Label("OS Version")
String osVersion;
}

@graalvmbot
Copy link
Collaborator

Hello jovanstevanovic, thanks for contributing a PR to our project!

We use the Oracle Contributor Agreement to make the copyright of contributions clear. We don't have a record of you having signed this yet, based on your email address jovan -(dot)- stevanovic1 -(at)- outlook -(dot)- com. You can sign it at that link.

If you think you've already signed it, please comment below and we'll check.

@jovanstevanovic
Copy link
Member Author

jovanstevanovic commented Aug 30, 2021

/signed

@vjovanov
Copy link
Member

LGTM, but I think @christianhaeubl would be a better choice to review here.

Copy link
Collaborator

@jiekang jiekang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks to run fine now. Thanks Jovan! I think a review + approval from @christianhaeubl is also appropriate.

@graalvmbot
Copy link
Collaborator

jovanstevanovic has signed the Oracle Contributor Agreement (based on email address jovan -(dot)- stevanovic1 -(at)- outlook -(dot)- com) so can contribute to this repository.

Copy link
Member

@christianhaeubl christianhaeubl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on that. However, the current approach won't work for the following reasons:

  • It is not always safe to access the heap allocated Thread object (see one of my comments in the source code).
  • At the moment, you are storing Java heap objects into native memory. This will cause crashes.

@christianhaeubl
Copy link
Member

@jovanstevanovic: just to let you know, I am currently doing a few final cleanups and then I will merge the changes of this PR to master.

@jovanstevanovic
Copy link
Member Author

@jovanstevanovic: just to let you know, I am currently doing a few final cleanups and then I will merge the changes of this PR to master.

Great! Thanks a lot! 💯

@christianhaeubl
Copy link
Member

@jovanstevanovic : could you add a few test cases for this feature? I also fixed a couple bugs as part of my cleanup, so you probably want to wait until the final version is merged.

@jovanstevanovic
Copy link
Member Author

@jovanstevanovic : could you add a few test cases for this feature? I also fixed a couple bugs as part of my cleanup, so you probably want to wait until the final version is merged.

No problem, I can add a few test cases.

@jovanstevanovic jovanstevanovic deleted the jovanstevanovic/improving-jfr-support branch November 17, 2021 10:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants