Re: RFR: 8275338: Add JFR events for notable serialization situations [v3]
> Adds serialization misdeclaration events to JFR. Raffaello Giulietti has updated the pull request incrementally with one additional commit since the last revision: Event enabled on profile.jfc but disabled on default.jfc. - Changes: - all: https://git.openjdk.org/jdk/pull/17129/files - new: https://git.openjdk.org/jdk/pull/17129/files/629e8f23..8534d7af Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17129&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17129&range=01-02 Stats: 37 lines in 5 files changed: 27 ins; 6 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/17129.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17129/head:pull/17129 PR: https://git.openjdk.org/jdk/pull/17129
Re: RFR: 8275338: Add JFR events for notable serialization situations [v3]
On Mon, 18 Dec 2023 17:49:04 GMT, Raffaello Giulietti
wrote:
>> Adds serialization misdeclaration events to JFR.
>
> Raffaello Giulietti has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Event enabled on profile.jfc but disabled on default.jfc.
src/jdk.jfr/share/classes/jdk/jfr/events/SerializationMisdeclarationEvent.java
line 41:
> 39: @Description("Methods and fields misdeclarations")
> 40: @MirrorEvent(className =
> "jdk.internal.event.SerializationMisdeclarationEvent")
> 41: @RemoveFields({"duration", "stackTrace", "thread"})
The field should be "eventThread" instead of "thread"
src/jdk.jfr/share/classes/jdk/jfr/events/SerializationMisdeclarationEvent.java
line 45:
> 43:
> 44: @Label("Class")
> 45: public Class cls;
We have often used a prefix, i.e. misdeclaredClass, to avoid using a reserved
word. We try to stay out of abbreviations.
src/jdk.jfr/share/classes/jdk/jfr/events/SerializationMisdeclarationEvent.java
line 48:
> 46:
> 47: @Label("Kind")
> 48: public int kind;
What is the use case for error codes? Are they public or an implementation
detail?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17129#discussion_r1431226200
PR Review Comment: https://git.openjdk.org/jdk/pull/17129#discussion_r1431227843
PR Review Comment: https://git.openjdk.org/jdk/pull/17129#discussion_r1431229900
Re: RFR: 8275338: Add JFR events for notable serialization situations [v3]
On Tue, 19 Dec 2023 10:43:57 GMT, Erik Gahlin wrote:
>> Raffaello Giulietti has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Event enabled on profile.jfc but disabled on default.jfc.
>
> src/jdk.jfr/share/classes/jdk/jfr/events/SerializationMisdeclarationEvent.java
> line 48:
>
>> 46:
>> 47: @Label("Kind")
>> 48: public int kind;
>
> What is the use case for error codes? Are they public or an implementation
> detail?
The intent is that they are stable and for programmatic usage, whereas the
message is more for human consumption. The codes are used in the test, for
example, and are declared as public static in the event classes.
Alternatively, one could parse the message, but that's less robust in face of
changes, I think.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17129#discussion_r1431329983
Re: RFR: 8275338: Add JFR events for notable serialization situations [v3]
On Tue, 19 Dec 2023 12:17:38 GMT, Raffaello Giulietti
wrote:
>> src/jdk.jfr/share/classes/jdk/jfr/events/SerializationMisdeclarationEvent.java
>> line 48:
>>
>>> 46:
>>> 47: @Label("Kind")
>>> 48: public int kind;
>>
>> What is the use case for error codes? Are they public or an implementation
>> detail?
>
> The intent is that they are stable and for programmatic usage, whereas the
> message is more for human consumption. The codes are used in the test, for
> example, and are declared as public static in the event classes.
>
> Alternatively, one could parse the message, but that's less robust in face of
> changes, I think.
Users (not OpenJDK developers) don't know what the error code means. I think
it's better to not have them. This is how other events work. If you want to
guard against changes, I would export the package to the test.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17129#discussion_r1431716132
Re: RFR: 8275338: Add JFR events for notable serialization situations [v3]
On Tue, 19 Dec 2023 17:13:58 GMT, Erik Gahlin wrote: >> The intent is that they are stable and for programmatic usage, whereas the >> message is more for human consumption. The codes are used in the test, for >> example, and are declared as public static in the event classes. >> >> Alternatively, one could parse the message, but that's less robust in face >> of changes, I think. > > Users (not OpenJDK developers) don't know what the error code means. I think > it's better to not have them. This is how other events work. If you want to > guard against changes, I would export the package to the test. What about fixed `String`s rather than `int`s for the kind of error? Something like `"SUID_INEFFECTIVE_ON_ENUM"`, and so on? It would be nice to be able to use enums, but AFAIK that's not supported in JFR. - PR Review Comment: https://git.openjdk.org/jdk/pull/17129#discussion_r1431744479
Re: RFR: 8275338: Add JFR events for notable serialization situations [v3]
On Tue, 19 Dec 2023 17:41:57 GMT, Raffaello Giulietti wrote: >> Users (not OpenJDK developers) don't know what the error code means. I think >> it's better to not have them. This is how other events work. If you want to >> guard against changes, I would export the package to the test. > > What about fixed `String`s rather than `int`s for the kind of error? > Something like `"SUID_INEFFECTIVE_ON_ENUM"`, and so on? > It would be nice to be able to use enums, but AFAIK that's not supported in > JFR. You could define them with an Enum but use the ordinal as the value for JFR. - PR Review Comment: https://git.openjdk.org/jdk/pull/17129#discussion_r1431864329
Re: RFR: 8275338: Add JFR events for notable serialization situations [v3]
On Tue, 19 Dec 2023 19:39:22 GMT, Roger Riggs wrote: >> What about fixed `String`s rather than `int`s for the kind of error? >> Something like `"SUID_INEFFECTIVE_ON_ENUM"`, and so on? >> It would be nice to be able to use enums, but AFAIK that's not supported in >> JFR. > > You could define them with an Enum but use the ordinal as the value for JFR. Same remark here about finality as https://github.com/openjdk/jdk/pull/17129#discussion_r1432400888. public statics should be final. - PR Review Comment: https://git.openjdk.org/jdk/pull/17129#discussion_r1432402527
Re: RFR: 8275338: Add JFR events for notable serialization situations [v3]
On Wed, 20 Dec 2023 08:29:19 GMT, Daniel Fuchs wrote: >> You could define them with an Enum but use the ordinal as the value for JFR. > > Same remark here about finality as > https://github.com/openjdk/jdk/pull/17129#discussion_r1432400888. public > statics should be final. I'd also remove the error codes, the string messages will be pretty stable and the extra surface area of the constants is just maintenance overhead. - PR Review Comment: https://git.openjdk.org/jdk/pull/17129#discussion_r1432772028
Re: RFR: 8275338: Add JFR events for notable serialization situations [v3]
On Wed, 20 Dec 2023 14:17:19 GMT, Roger Riggs wrote: >> Same remark here about finality as >> https://github.com/openjdk/jdk/pull/17129#discussion_r1432400888. public >> statics should be final. > > I'd also remove the error codes, the string messages will be pretty stable > and the extra surface area of the constants is just maintenance overhead. Working on that. - PR Review Comment: https://git.openjdk.org/jdk/pull/17129#discussion_r1432828516
