Re: RFR: 8275338: Add JFR events for notable serialization situations [v4]
On Wed, 10 Jan 2024 17:41:41 GMT, Raffaello Giulietti wrote: >> The spec is silent about methods being 'native'; it would generally be >> impractical to implement native methods for these purposes, but a native >> method can implement the behavior. > > @RogerRiggs The checks are agnostic about a method being `native` or not, so > I'm not sure I get your point about `native` methods. Right, there's nothing to change; a method declared as native is not mis-declared. - PR Review Comment: https://git.openjdk.org/jdk/pull/17129#discussion_r1447785756
Re: RFR: 8275338: Add JFR events for notable serialization situations [v4]
On Wed, 10 Jan 2024 16:58:43 GMT, Roger Riggs wrote: >> I'm not sure that a `native` method is not considered effective by >> serialization. I have to check. > > The spec is silent about methods being 'native'; it would generally be > impractical to implement native methods for these purposes, but a native > method can implement the behavior. @RogerRiggs The checks are agnostic about a method being `native` or not, so I'm not sure I get your point about `native` methods. - PR Review Comment: https://git.openjdk.org/jdk/pull/17129#discussion_r1447721944
Re: RFR: 8275338: Add JFR events for notable serialization situations [v4]
On Tue, 19 Dec 2023 15:58:10 GMT, Raffaello Giulietti wrote: >> src/java.base/share/classes/java/io/SerializationMisdeclarationChecker.java >> line 185: >> >>> 183: commitEvent(PRIV_METH_NON_STATIC, >>> 184: m + " must be non-static to be effective"); >>> 185: } >> >> Should there also be a check to see if this `private` method is >> (misdeclared) as a `native` method? > > I'm not sure that a `native` method is not considered effective by > serialization. I have to check. The spec is silent about methods being 'native'; it would generally be impractical to implement native methods for these purposes, but a native method can implement the behavior. - PR Review Comment: https://git.openjdk.org/jdk/pull/17129#discussion_r1447665831
Re: RFR: 8275338: Add JFR events for notable serialization situations [v4]
On Tue, 19 Dec 2023 19:19:35 GMT, Roger Riggs wrote:
>> Raffaello Giulietti has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Better name for a label, corrected name of removed field.
>
> src/java.base/share/classes/java/io/SerializationMisdeclarationChecker.java
> line 108:
>
>> 106: SUID_NAME + " should be declared of type long");
>> 107: }
>> 108: if (!isStatic(f)) {
>
> The two calls to isStatic could be reordered closer together to be a single
> if (isstatic()) { ... } else {... }.
I guess you are looking to an older commit?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17129#discussion_r1432581987
Re: RFR: 8275338: Add JFR events for notable serialization situations [v4]
On Tue, 19 Dec 2023 16:00:09 GMT, Raffaello Giulietti wrote: >> src/java.base/share/classes/jdk/internal/event/SerializationMisdeclarationEvent.java >> line 94: >> >>> 92: >>> 93: /* >>> 94: * These constants are not final on purpose. >> >> Just curious - what's the purpose? I don't see them being changed/updated >> anywhere (not even in tests). > > Declaring a `public static int` field `final` means that the value is then > stored in the client class. If a value is changed, then the client needs to > be recompiled as well, but this is not enforced by the language, so it might > lead to inconsistencies. > > The clean solution would be to use an enum class, but that's not supported by > JFR. Not having them final is a quite smelly. I would suggest to make them final with a comment that new values can be added but that existing values must not be changed. - PR Review Comment: https://git.openjdk.org/jdk/pull/17129#discussion_r1432400888
Re: RFR: 8275338: Add JFR events for notable serialization situations [v4]
On Tue, 19 Dec 2023 12:21:05 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:
>
> Better name for a label, corrected name of removed field.
src/java.base/share/classes/java/io/SerializationMisdeclarationChecker.java
line 108:
> 106: SUID_NAME + " should be declared of type long");
> 107: }
> 108: if (!isStatic(f)) {
The two calls to isStatic could be reordered closer together to be a single if
(isstatic()) { ... } else {... }.
src/java.base/share/classes/jdk/internal/event/SerializationMisdeclarationEvent.java
line 126:
> 124: public static int ACC_METH_PARAM_TYPES= 404;
> 125: public static int ACC_METH_NON_ACCESSIBLE = 405;
> 126:
I'd rather see few (fewer, just one) kinds of events, with so many different
kinds of events there needs to be a convenient method to look for any kind of
serialization mis-declaration.
Perhaps a single event with flags for the different kinds of errors.
The event classes could be responsible for turning the flags back into useful
messages on display.
test/jdk/jdk/jfr/event/io/TestSerializationMisdeclarationEvent.java line 157:
> 155: }
> 156:
> 157: private static class A implements Serializable {
The test classes should document the good or badness of the class either in the
class/field/method names or in comments. What's wrong with this XXX.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17129#discussion_r1431846034
PR Review Comment: https://git.openjdk.org/jdk/pull/17129#discussion_r1431850091
PR Review Comment: https://git.openjdk.org/jdk/pull/17129#discussion_r1431856800
Re: RFR: 8275338: Add JFR events for notable serialization situations [v4]
On Tue, 19 Dec 2023 16:28:03 GMT, Raffaello Giulietti wrote: > However, the cache can be emptied under high memory pressure, so the > `ObjectStreamClass` instance might be recreated later, thus re-invoking the > serialization checker once again. I think it would be good to state in the description when checks occur, something like: Only the first time a class is serialized are the misdeclaration checks carried out, but may happen happen more frequently under high memory pressure. (Try to avoid using the word "event") - PR Comment: https://git.openjdk.org/jdk/pull/17129#issuecomment-1863216232
Re: RFR: 8275338: Add JFR events for notable serialization situations [v4]
On Tue, 19 Dec 2023 16:00:59 GMT, Raffaello Giulietti wrote: >> test/jdk/jdk/jfr/event/io/TestSerializationMisdeclarationEvent.java line 50: >> >>> 48: * @requires vm.hasJFR >>> 49: * @library /test/lib >>> 50: * @run junit/othervm >>> jdk.jfr.event.io.TestSerializationMisdeclarationEvent >> >> Is the use of "othervm" needed here because of the use of >> `jdk.jfr.consumer.RecordingStream`? Does `RecordingStream` do JVM wide state >> changes? I did a quick look at that class but couldn't come to a conclusion. > > Not sure, I have to check. All the other events run in othervm. While it may be possible in some case to not do that, it's much easier to analyse issues, if we are sure the JVM is fresh. For example, JFR may not generate bytecode if an event is disabled. The JFR tests have been hardened to be able to run in parallel, so they run pretty fast. - PR Review Comment: https://git.openjdk.org/jdk/pull/17129#discussion_r1431727571
Re: RFR: 8275338: Add JFR events for notable serialization situations [v4]
On Tue, 19 Dec 2023 15:56:39 GMT, Raffaello Giulietti wrote: > > Is it per class for each classloader that loads it? Or is it per class per > > JVM? It's more out of curiosity than anything else because I don't think it > > makes a big difference (I don't expect too many classloaders that would > > lead to the case of extremely large streams of events). > > The checks are done on the `Class` instance, that is, per each defined (as > per JVMS) and _used_ serializable class, on first usage in serialization. If > enabled at all, they are invoked by the private `ObjectStreamClass` > constructor. Well, in fact `ObjectStreamClass` maintains a cache of `Class` -> `ObjectStreamClass` to avoid creating a new instance of `ObjectStreamClass` each time a `Class` is looked up. It memoizes the association the first time. However, the cache can be emptied under high memory pressure, so the `ObjectStreamClass` instance might be recreated later, thus re-invoking the serialization checker once again. - PR Comment: https://git.openjdk.org/jdk/pull/17129#issuecomment-1863090984
Re: RFR: 8275338: Add JFR events for notable serialization situations [v4]
On Tue, 19 Dec 2023 14:39:47 GMT, Jaikiran Pai wrote:
> Is it per class for each classloader that loads it? Or is it per class per
> JVM? It's more out of curiosity than anything else because I don't think it
> makes a big difference (I don't expect too many classloaders that would lead
> to the case of extremely large streams of events).
The checks are done on the `Class` instance, that is, per each defined (as
per JVMS) and _used_ serializable class, on first usage in serialization. If
enabled at all, they are invoked by the private `ObjectStreamClass` constructor.
> src/java.base/share/classes/java/io/SerializationMisdeclarationChecker.java
> line 94:
>
>> 92: if (!isPrivate(f)) {
>> 93: commitEvent(SUID_PRIVATE,
>> 94: SUID_NAME + " should be declared private");
>
> Rest of the event messages use "... to be effective", should this one too use
> a similar text?
The field needs not be declared `private` to be effective, although it is
recommended to do so. In this sense, "should" is just a recommendation, while
"must" is really needed to make something effective.
> src/java.base/share/classes/java/io/SerializationMisdeclarationChecker.java
> line 108:
>
>> 106: SUID_NAME + " should be declared of type long");
>> 107: }
>> 108: if (!isStatic(f)) {
>
> Nit - perhaps save the return value from the previous call to `isStatic(f)` a
> few lines above and reuse it here?
Assuming that most serializable classes are declared correctly, I don't think
that caching the value of `isStatic()` makes a measurable difference.
If at all, then I'd prefer to cache the modifiers and "inline" the masking
logic of the individual is*() methods.
> src/java.base/share/classes/java/io/SerializationMisdeclarationChecker.java
> line 112:
>
>> 110: }
>> 111: f.setAccessible(true);
>> 112: if (getLong(f) == null) {
>
> Is this check and the `setAccessible()` needed if a few lines above, the call
> to `f.getType()` returns `Long.TYPE` (i.e. primitive type)? Perhaps we can
> conditionally do these additional checks and calls, only if `f.getType()`
> isn't a primitive?
Right, will be addressed in the next commit.
> src/java.base/share/classes/java/io/SerializationMisdeclarationChecker.java
> line 185:
>
>> 183: commitEvent(PRIV_METH_NON_STATIC,
>> 184: m + " must be non-static to be effective");
>> 185: }
>
> Should there also be a check to see if this `private` method is (misdeclared)
> as a `native` method?
I'm not sure that a `native` method is not considered effective by
serialization. I have to check.
> src/java.base/share/classes/java/io/SerializationMisdeclarationChecker.java
> line 276:
>
>> 274: }
>> 275:
>> 276: private static Object getObject(Field f) {
>
> Should we call this method `getStaticFieldValue(...)`, because that's what
> the implementation is.
Since the only parameter is `Field`, it has to be a static field almost for
sure.
Further, there's a `getLong()` method down below that operates on a static
field as well and that one would also need a renaming.
If I can come up with better names they will be in the next commit.
> src/java.base/share/classes/jdk/internal/event/SerializationMisdeclarationEvent.java
> line 94:
>
>> 92:
>> 93: /*
>> 94: * These constants are not final on purpose.
>
> Just curious - what's the purpose? I don't see them being changed/updated
> anywhere (not even in tests).
Declaring a `public static int` field `final` means that the value is then
stored in the client class. If a value is changed, then the client needs to be
recompiled as well, but this is not enforced by the language, so it might lead
to inconsistencies.
The clean solution would be to use an enum class, but that's not supported by
JFR.
> test/jdk/jdk/jfr/event/io/TestSerializationMisdeclarationEvent.java line 50:
>
>> 48: * @requires vm.hasJFR
>> 49: * @library /test/lib
>> 50: * @run junit/othervm
>> jdk.jfr.event.io.TestSerializationMisdeclarationEvent
>
> Is the use of "othervm" needed here because of the use of
> `jdk.jfr.consumer.RecordingStream`? Does `RecordingStream` do JVM wide state
> changes? I did a quick look at that class but couldn't come to a conclusion.
Not sure, I have to check.
> test/jdk/jdk/jfr/event/io/TestSerializationMisdeclarationEvent.java line 112:
>
>> 110: @MethodSource
>> 111: public void testGoodClass(Class cls) {
>> 112: assertEquals(0, getEventsFor(cls).size());
>
> Can this and other assertions in this test be updated to include the class
> name which failed? You can still use `assertEquals(...)`, it takes an
> optional message as a third parameter which you could use to include the
> failing class name. It becomes a bit more easier to debug (unexpected)
> failures when the assertion includes these additional details.
Thanks for the suggestion, will ta
Re: RFR: 8275338: Add JFR events for notable serialization situations [v4]
On Tue, 19 Dec 2023 12:21:05 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: > > Better name for a label, corrected name of removed field. test/jdk/jdk/jfr/event/io/TestSerializationMisdeclarationEvent.java line 50: > 48: * @requires vm.hasJFR > 49: * @library /test/lib > 50: * @run junit/othervm > jdk.jfr.event.io.TestSerializationMisdeclarationEvent Is the use of "othervm" needed here because of the use of `jdk.jfr.consumer.RecordingStream`? Does `RecordingStream` do JVM wide state changes? I did a quick look at that class but couldn't come to a conclusion. - PR Review Comment: https://git.openjdk.org/jdk/pull/17129#discussion_r1431578942
Re: RFR: 8275338: Add JFR events for notable serialization situations [v4]
On Tue, 19 Dec 2023 12:21:05 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:
>
> Better name for a label, corrected name of removed field.
test/jdk/jdk/jfr/event/io/TestSerializationMisdeclarationEvent.java line 144:
> 142: }
> 143:
> 144: static {
It's a bit odd for a test case to be doing this in a static block. Could this
instead be done in a `org.junit.jupiter.api.BeforeAll` method:
import org.junit.jupiter.api.BeforeAll;
...
@BeforeAll
static void recordEvents() {
// that static block's code here
}
Any (unexpected) failures in that method will then be reported in a better way
by the testing framework instead of an `ExceptionInInitializer` that would be
reported from a failure in static block and the test class itself failing to
load.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17129#discussion_r1431571145
Re: RFR: 8275338: Add JFR events for notable serialization situations [v4]
On Tue, 19 Dec 2023 12:21:05 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:
>
> Better name for a label, corrected name of removed field.
test/jdk/jdk/jfr/event/io/TestSerializationMisdeclarationEvent.java line 112:
> 110: @MethodSource
> 111: public void testGoodClass(Class cls) {
> 112: assertEquals(0, getEventsFor(cls).size());
Can this and other assertions in this test be updated to include the class name
which failed? You can still use `assertEquals(...)`, it takes an optional
message as a third parameter which you could use to include the failing class
name. It becomes a bit more easier to debug (unexpected) failures when the
assertion includes these additional details.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17129#discussion_r1431563775
Re: RFR: 8275338: Add JFR events for notable serialization situations [v4]
On Tue, 19 Dec 2023 12:21:05 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: > > Better name for a label, corrected name of removed field. src/java.base/share/classes/jdk/internal/event/SerializationMisdeclarationEvent.java line 94: > 92: > 93: /* > 94: * These constants are not final on purpose. Just curious - what's the purpose? I don't see them being changed/updated anywhere (not even in tests). - PR Review Comment: https://git.openjdk.org/jdk/pull/17129#discussion_r1431551183
Re: RFR: 8275338: Add JFR events for notable serialization situations [v4]
On Tue, 19 Dec 2023 12:21:05 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:
>
> Better name for a label, corrected name of removed field.
src/java.base/share/classes/java/io/SerializationMisdeclarationChecker.java
line 276:
> 274: }
> 275:
> 276: private static Object getObject(Field f) {
Should we call this method `getStaticFieldValue(...)`, because that's what the
implementation is.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17129#discussion_r1431535049
Re: RFR: 8275338: Add JFR events for notable serialization situations [v4]
On Tue, 19 Dec 2023 12:21:05 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: > > Better name for a label, corrected name of removed field. src/java.base/share/classes/java/io/SerializationMisdeclarationChecker.java line 185: > 183: commitEvent(PRIV_METH_NON_STATIC, > 184: m + " must be non-static to be effective"); > 185: } Should there also be a check to see if this `private` method is (misdeclared) as a `native` method? - PR Review Comment: https://git.openjdk.org/jdk/pull/17129#discussion_r1431524703
Re: RFR: 8275338: Add JFR events for notable serialization situations [v4]
On Tue, 19 Dec 2023 12:21:05 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:
>
> Better name for a label, corrected name of removed field.
src/java.base/share/classes/java/io/SerializationMisdeclarationChecker.java
line 94:
> 92: if (!isPrivate(f)) {
> 93: commitEvent(SUID_PRIVATE,
> 94: SUID_NAME + " should be declared private");
Rest of the event messages use "... to be effective", should this one too use a
similar text?
src/java.base/share/classes/java/io/SerializationMisdeclarationChecker.java
line 108:
> 106: SUID_NAME + " should be declared of type long");
> 107: }
> 108: if (!isStatic(f)) {
Nit - perhaps save the return value from the previous call to `isStatic(f)` a
few lines above and reuse it here?
src/java.base/share/classes/java/io/SerializationMisdeclarationChecker.java
line 112:
> 110: }
> 111: f.setAccessible(true);
> 112: if (getLong(f) == null) {
Is this check and the `setAccessible()` needed if a few lines above, the call
to `f.getType()` returns `Long.TYPE` (i.e. primitive type)? Perhaps we can
conditionally do these additional checks and calls, only if `f.getType()` isn't
a primitive?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17129#discussion_r1431512491
PR Review Comment: https://git.openjdk.org/jdk/pull/17129#discussion_r1431506705
PR Review Comment: https://git.openjdk.org/jdk/pull/17129#discussion_r1431511005
Re: RFR: 8275338: Add JFR events for notable serialization situations [v4]
On Tue, 19 Dec 2023 12:21:05 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: > > Better name for a label, corrected name of removed field. Hello Raffaello, > On the other hand, the checks and the events generation are done for > serializable classes that actually actively participate in serialization, > lazily, and just once per class Is it per class for each classloader that loads it? Or is it per class per JVM? It's more out of curiosity than anything else because I don't think it makes a big difference (I don't expect too many classloaders that would lead to the case of extremely large streams of events). - PR Comment: https://git.openjdk.org/jdk/pull/17129#issuecomment-1862882260
Re: RFR: 8275338: Add JFR events for notable serialization situations [v4]
> Adds serialization misdeclaration events to JFR. Raffaello Giulietti has updated the pull request incrementally with one additional commit since the last revision: Better name for a label, corrected name of removed field. - Changes: - all: https://git.openjdk.org/jdk/pull/17129/files - new: https://git.openjdk.org/jdk/pull/17129/files/8534d7af..51507e4e Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17129&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17129&range=02-03 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 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
