RFR: 8334031: Generated JfrNativeSettings seems off
This PR changes GenerateJfrFiles.java so that the generated `JfrNativeSettings` union does not include JFR structs.`JfrNativeSettings` is meant to hold the settings for JFR events, but previously also included JFR structs such as MetaspaceSizes, StackFrame, CopyFailed, G1EvacuationStatistics, ObjectSpace, VirtualSpace. These are not events, but instead are JFR `Type`s, and so do not have settings such as stacktraces or thresholds. The inclusion of JFR structs in `JfrNativeSettings` was problematic because it could cause a displacement between event ID and`JfrNativeSettings` array index (each index is meant to correspond with a specific event ID). Testing: - jdk/jdk/jfr - tier1 ------------- Commit messages: - Don't include JFR structs in the list of JFR events in generated files. Changes: https://git.openjdk.org/jdk/pull/19891/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=19891&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8334031 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/19891.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19891/head:pull/19891 PR: https://git.openjdk.org/jdk/pull/19891
On Tue, 25 Jun 2024 18:55:14 GMT, Robert Toyonaga <duke@openjdk.org> wrote:
This PR changes GenerateJfrFiles.java so that the generated `JfrNativeSettings` union does not include JFR structs.`JfrNativeSettings` is meant to hold the settings for JFR events, but previously also included JFR structs such as MetaspaceSizes, StackFrame, CopyFailed, G1EvacuationStatistics, ObjectSpace, VirtualSpace. These are not events, but instead are JFR `Type`s, and so do not have settings such as stacktraces or thresholds.
The inclusion of JFR structs in `JfrNativeSettings` was problematic because it could cause a displacement between event ID and`JfrNativeSettings` array index (each index is meant to correspond with a specific event ID).
Testing: - jdk/jdk/jfr - tier1
All types in JFR is assigned an ID, regardless if they are events or a structs. If we are going to index into the array, with the event type ID, must not the holes be filled with something? Or are all native event type IDs first? ------------- PR Comment: https://git.openjdk.org/jdk/pull/19891#issuecomment-2192215608
On Wed, 26 Jun 2024 17:00:23 GMT, Erik Gahlin <egahlin@openjdk.org> wrote:
All types in JFR is assigned an ID, regardless if they are events or a structs. If we are going to index into the array, with the event type ID, must not the holes be filled with something? Or are all native event type IDs first?
It looks like we [index using the `JfrEventId` enum](https://github.com/openjdk/jdk/blob/master/src/hotspot/share/jfr/recorder/jf...). This enum [only enumerates events](https://github.com/openjdk/jdk/blob/master/make/src/classes/build/tools/jfr/...), not all types. So I don't think there will actually be any holes. I'm a bit confused about why structs were included originally. It seems like it was intentional: ["_Then, to make it easy to debug, add named struct members also_"](https://github.com/openjdk/jdk/blob/master/make/src/classes/build/tools/jfr/...). But I can't see why it makes sense to group them with events in this way. ------------- PR Comment: https://git.openjdk.org/jdk/pull/19891#issuecomment-2192401387
On Tue, 25 Jun 2024 18:55:14 GMT, Robert Toyonaga <duke@openjdk.org> wrote:
This PR changes GenerateJfrFiles.java so that the generated `JfrNativeSettings` union does not include JFR structs.`JfrNativeSettings` is meant to hold the settings for JFR events, but previously also included JFR structs such as MetaspaceSizes, StackFrame, CopyFailed, G1EvacuationStatistics, ObjectSpace, VirtualSpace. These are not events, but instead are JFR `Type`s, and so do not have settings such as stacktraces or thresholds.
The inclusion of JFR structs in `JfrNativeSettings` was problematic because it could cause a displacement between event ID and`JfrNativeSettings` array index (each index is meant to correspond with a specific event ID).
Testing: - jdk/jdk/jfr - tier1
What drives the settings system is Java where the event name is linked to the ID, not the enum, e.g. jdk.jfr.internal.JVM.setEnabled(eventTypeId, enabled); If the type IDs are assigned in the order they appear in metadata.xml, then there will (or can be) holes. If GenerateJfrFiles.java makes sure native event types are processed first and then structs, we should be fine. ------------- PR Comment: https://git.openjdk.org/jdk/pull/19891#issuecomment-2192422923
On Wed, 26 Jun 2024 18:52:40 GMT, Erik Gahlin <egahlin@openjdk.org> wrote:
This PR changes GenerateJfrFiles.java so that the generated `JfrNativeSettings` union does not include JFR structs.`JfrNativeSettings` is meant to hold the settings for JFR events, but previously also included JFR structs such as MetaspaceSizes, StackFrame, CopyFailed, G1EvacuationStatistics, ObjectSpace, VirtualSpace. These are not events, but instead are JFR `Type`s, and so do not have settings such as stacktraces or thresholds.
The inclusion of JFR structs in `JfrNativeSettings` was problematic because it could cause a displacement between event ID and`JfrNativeSettings` array index (each index is meant to correspond with a specific event ID).
Testing: - jdk/jdk/jfr - tier1
What drives the settings system is Java where the event name is linked to the ID, not the enum, e.g.
jdk.jfr.internal.JVM.setEnabled(eventTypeId, enabled);
If the type IDs are assigned in the order they appear in metadata.xml, then there will (or can) be holes. If GenerateJfrFiles.java makes sure native event types are processed first and then structs, we should be fine.
Thank you for the review @egahlin ! ------------- PR Comment: https://git.openjdk.org/jdk/pull/19891#issuecomment-2194648286
On Tue, 25 Jun 2024 18:55:14 GMT, Robert Toyonaga <duke@openjdk.org> wrote:
This PR changes GenerateJfrFiles.java so that the generated `JfrNativeSettings` union does not include JFR structs.`JfrNativeSettings` is meant to hold the settings for JFR events, but previously also included JFR structs such as MetaspaceSizes, StackFrame, CopyFailed, G1EvacuationStatistics, ObjectSpace, VirtualSpace. These are not events, but instead are JFR `Type`s, and so do not have settings such as stacktraces or thresholds.
The inclusion of JFR structs in `JfrNativeSettings` was problematic because it could cause a displacement between event ID and`JfrNativeSettings` array index (each index is meant to correspond with a specific event ID).
Testing: - jdk/jdk/jfr - tier1
Ok I see. Yes, it looks like the events are processed first. https://github.com/openjdk/jdk/blob/master/make/src/classes/build/tools/jfr/... So the JFR structs should have IDs greater than any events. ------------- PR Comment: https://git.openjdk.org/jdk/pull/19891#issuecomment-2192498003
On Tue, 25 Jun 2024 18:55:14 GMT, Robert Toyonaga <duke@openjdk.org> wrote:
This PR changes GenerateJfrFiles.java so that the generated `JfrNativeSettings` union does not include JFR structs.`JfrNativeSettings` is meant to hold the settings for JFR events, but previously also included JFR structs such as MetaspaceSizes, StackFrame, CopyFailed, G1EvacuationStatistics, ObjectSpace, VirtualSpace. These are not events, but instead are JFR `Type`s, and so do not have settings such as stacktraces or thresholds.
The inclusion of JFR structs in `JfrNativeSettings` was problematic because it could cause a displacement between event ID and`JfrNativeSettings` array index (each index is meant to correspond with a specific event ID).
Testing: - jdk/jdk/jfr - tier1
Marked as reviewed by egahlin (Reviewer). ------------- PR Review: https://git.openjdk.org/jdk/pull/19891#pullrequestreview-2142981112
On Tue, 25 Jun 2024 18:55:14 GMT, Robert Toyonaga <duke@openjdk.org> wrote:
This PR changes GenerateJfrFiles.java so that the generated `JfrNativeSettings` union does not include JFR structs.`JfrNativeSettings` is meant to hold the settings for JFR events, but previously also included JFR structs such as MetaspaceSizes, StackFrame, CopyFailed, G1EvacuationStatistics, ObjectSpace, VirtualSpace. These are not events, but instead are JFR `Type`s, and so do not have settings such as stacktraces or thresholds.
The inclusion of JFR structs in `JfrNativeSettings` was problematic because it could cause a displacement between event ID and`JfrNativeSettings` array index (each index is meant to correspond with a specific event ID).
Testing: - jdk/jdk/jfr - tier1
@roberttoyonaga Your change (at version cfe16b77447f49acdb9ad92717855eced1cdb4e1) is now ready to be sponsored by a Committer. ------------- PR Comment: https://git.openjdk.org/jdk/pull/19891#issuecomment-2194650765
On Tue, 25 Jun 2024 18:55:14 GMT, Robert Toyonaga <duke@openjdk.org> wrote:
This PR changes GenerateJfrFiles.java so that the generated `JfrNativeSettings` union does not include JFR structs.`JfrNativeSettings` is meant to hold the settings for JFR events, but previously also included JFR structs such as MetaspaceSizes, StackFrame, CopyFailed, G1EvacuationStatistics, ObjectSpace, VirtualSpace. These are not events, but instead are JFR `Type`s, and so do not have settings such as stacktraces or thresholds.
The inclusion of JFR structs in `JfrNativeSettings` was problematic because it could cause a displacement between event ID and`JfrNativeSettings` array index (each index is meant to correspond with a specific event ID).
Testing: - jdk/jdk/jfr - tier1
This pull request has now been integrated. Changeset: da0ffa8b Author: Robert Toyonaga <rtoyonag@redhat.com> Committer: Thomas Stuefe <stuefe@openjdk.org> URL: https://git.openjdk.org/jdk/commit/da0ffa8b7ff04eb5cbc0fcbe4b858f20d7e46405 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod 8334031: Generated JfrNativeSettings seems off Reviewed-by: egahlin ------------- PR: https://git.openjdk.org/jdk/pull/19891
participants (3)
-
duke
-
Erik Gahlin
-
Robert Toyonaga