Skip to content

Conversation

@roberttoyonaga
Copy link
Collaborator

@roberttoyonaga roberttoyonaga commented Oct 12, 2022

Summary of bug:

Currently a null pointer exception is thrown at runtime if the thread sleep event is emitted (with JDK19). This is because jdk.ThreadSleep is only added as an event if the JDK version is <17. This is a recent change in SVM that was done because in JDK19, ThreadSleep is now a mirror event and is not longer in metadata.xml. This means it doesn't get added to JfrMetadataTypeLibrary, and so an NPE is eventually thrown when we try to emit the event in uninterruptible code. A solution to this could be to not attempt to emit thread sleep events if jdk version is over 17. This PR instead tries to add support for thread sleep as a mirror event.

This is the error:
Exception in thread "Thread-2" java.lang.NullPointerException: [no exception stack trace available because exception is thrown from code that must be allocation free]

To reproduce:

  1. Build graal with jdk19
  2. Compile this sample app with jdk 19. Any java app that uses Thread.sleep() could be used.
  3. Build with JFR included in the image mx native-image --no-fallback -H:+AllowVMInspection -cp /jfr-tests/target/classes com.redhat.jfr.tests.event.TestJavaMonitorWaitEvents
  4. Run with starting flight recorder ./com.redhat.jfr.tests.event.testjavamonitorwaitevents -XX:+FlightRecorder -XX:StartFlightRecording=filename=testjavamonitorwaitevents.jfr

Summary of changes

To solve this, special handling of jdk.ThreadSleep as a mirror event is added (only if jdk version is 19). In the future, support for new mirror events could possibly be done in a similar way.

@oracle-contributor-agreement oracle-contributor-agreement bot added the OCA Verified All contributors have signed the Oracle Contributor Agreement. label Oct 12, 2022
@roberttoyonaga roberttoyonaga marked this pull request as draft October 12, 2022 20:39
@roberttoyonaga roberttoyonaga marked this pull request as ready for review October 12, 2022 20:48
@fniephaus fniephaus changed the title Support thread sleep as a mirror event in jdk 19 [GR-41851] Support thread sleep as a mirror event in JDK 19 Oct 14, 2022
@christianhaeubl
Copy link
Member

@roberttoyonaga do you know why this event was removed in JDK19? Does the event cause issues for virtual threads?

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, I stumbled across the same issue today in a slightly different context.

public class ThreadSleepEvent {
@Category("Java Application")
@Label("Java Thread Sleep")
@Name("jdk.ThreadSleep")
Copy link
Member

Choose a reason for hiding this comment

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

Does that really work reliably? If I understand everything correctly, then the following is happening:

  • HotSpot uses jdk.jfr.events.ThreadSleepEvent as the mirror event.
  • HotSpot registers jdk.internal.event.ThreadSleepEvent as the thread sleep event. This creates a PlatformEventType object at build time that ends up in the image heap (instance A).
  • You create a new PlatformEventType object for the SVM-internal class com.oracle.svm.core.jfr.events.ThreadSleepEvent at build time (instance B). This new object reuses the event id of instance A but otherwise behaves like a VM-internal event (e.g., SubstrateJVM.eventSettings is used to determine if the event is enabled or disabled).
  • You unconditionally enable the thread sleep event at build time (SubstrateJVM.handleMirrorEvents()).

I see the following issues:

  • What if the event should not be enabled by default?
  • What if the event is enabled at runtime via jdk.internal.event.ThreadSleepEvent? I don't think that the updated value will be used because JDK events directly store in their PlatformEventType object if the event is enabled. However, we would access SubstrateJVM.eventSettings to determine if the event is enabled.

Copy link
Collaborator Author

@roberttoyonaga roberttoyonaga Oct 20, 2022

Choose a reason for hiding this comment

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

Thank you for the review Christian! Yes that is what I was doing. I was under the impression that built in events were enabled by default. I have removed that unconditional enablement and instead substituted PlatformEventType.setEnabled(boolean). This way changes can be propagated to the SubstrateJVM.eventSettings if the event happens to be one of the supported mirror event types. Please let me know if this is an acceptable solution.

I think the thread sleep event was moved from hotspot native code to java code to be able to emit the events for virtual threads as well.

@christianhaeubl
Copy link
Member

I spent some more time on this topic and I think that we need a different approach. The JfrNativeEventWriter infrastructure is primarily designed to emit VM-level JFR events. Mirror events are JDK-level events that should use the JDK infrastructure (and therefore also all the JDK-level settings that are stored in the PlatformEventType). We could probably use workarounds for most of the issues (similar to the one that you added for setEnabled) but it would be pretty hacky.

So, I implemented a prototype where the thread sleep event is emitted in the same way as it is done by the JDK code. This seems to work but I need to do a few more cleanups and some more testing before I open a PR.

@roberttoyonaga
Copy link
Collaborator Author

I spent some more time on this topic and I think that we need a different approach. The JfrNativeEventWriter infrastructure is primarily designed to emit VM-level JFR events. Mirror events are JDK-level events that should use the JDK infrastructure (and therefore also all the JDK-level settings that are stored in the PlatformEventType). We could probably use workarounds for most of the issues (similar to the one that you added for setEnabled) but it would be pretty hacky.

So, I implemented a prototype where the thread sleep event is emitted in the same way as it is done by the JDK code. This seems to work but I need to do a few more cleanups and some more testing before I open a PR.

Hi @christianhaeubl is there any update on this? Thanks!

@christianhaeubl
Copy link
Member

@roberttoyonaga I created #5528

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

OCA Verified All contributors have signed the Oracle Contributor Agreement.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[GR-41952] NPE in thread "Performance data" when using JFR on JDK19

2 participants