Re: RFR: 8261160: Add a deserialization JFR event [v5]
On Fri, 12 Feb 2021 16:26:09 GMT, Chris Hegarty wrote: >> This issue adds a new event to improve diagnostic information of Java >> deserialization. The event captures the details of deserialization activity >> from ObjectInputStream. The event details are similar to that of the serial >> filter, but is agnostic of whether a filter is installed or not. The event >> also captures the filter status, if there is one. > > Chris Hegarty has updated the pull request incrementally with one additional > commit since the last revision: > > Split exception into type and message Marked as reviewed by egahlin (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/2479
Re: RFR: 8261160: Add a deserialization JFR event [v5]
On Fri, 12 Feb 2021 16:26:09 GMT, Chris Hegarty wrote: >> This issue adds a new event to improve diagnostic information of Java >> deserialization. The event captures the details of deserialization activity >> from ObjectInputStream. The event details are similar to that of the serial >> filter, but is agnostic of whether a filter is installed or not. The event >> also captures the filter status, if there is one. > > Chris Hegarty has updated the pull request incrementally with one additional > commit since the last revision: > > Split exception into type and message Marked as reviewed by dfuchs (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/2479
Re: RFR: 8261160: Add a deserialization JFR event [v5]
On Fri, 12 Feb 2021 16:26:09 GMT, Chris Hegarty wrote: >> This issue adds a new event to improve diagnostic information of Java >> deserialization. The event captures the details of deserialization activity >> from ObjectInputStream. The event details are similar to that of the serial >> filter, but is agnostic of whether a filter is installed or not. The event >> also captures the filter status, if there is one. > > Chris Hegarty has updated the pull request incrementally with one additional > commit since the last revision: > > Split exception into type and message Marked as reviewed by coffeys (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/2479
Re: RFR: 8261160: Add a deserialization JFR event [v5]
On Fri, 12 Feb 2021 16:26:09 GMT, Chris Hegarty wrote: >> This issue adds a new event to improve diagnostic information of Java >> deserialization. The event captures the details of deserialization activity >> from ObjectInputStream. The event details are similar to that of the serial >> filter, but is agnostic of whether a filter is installed or not. The event >> also captures the filter status, if there is one. > > Chris Hegarty has updated the pull request incrementally with one additional > commit since the last revision: > > Split exception into type and message Marked as reviewed by rriggs (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/2479
Re: RFR: 8261160: Add a deserialization JFR event [v4]
On Thu, 11 Feb 2021 16:02:09 GMT, Roger Riggs wrote: >> Chris Hegarty has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Filter **C**onfigured > > Marked as reviewed by rriggs (Reviewer). Speaking to Erik offline, he suggested to split the `exceptionMessage` into `exceptionType` and `exceptionMessage` ( since the code was somewhat overloading the existing Sting field by using toString to force the exception class name and message into a single sting ). While the exceptionXXX field should rarely be non-null, they add very little overhead. - PR: https://git.openjdk.java.net/jdk/pull/2479
Re: RFR: 8261160: Add a deserialization JFR event [v5]
> This issue adds a new event to improve diagnostic information of Java > deserialization. The event captures the details of deserialization activity > from ObjectInputStream. The event details are similar to that of the serial > filter, but is agnostic of whether a filter is installed or not. The event > also captures the filter status, if there is one. Chris Hegarty has updated the pull request incrementally with one additional commit since the last revision: Split exception into type and message - Changes: - all: https://git.openjdk.java.net/jdk/pull/2479/files - new: https://git.openjdk.java.net/jdk/pull/2479/files/d764f75f..8cb8f75e Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=2479=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2479=03-04 Stats: 48 lines in 4 files changed: 34 ins; 6 del; 8 mod Patch: https://git.openjdk.java.net/jdk/pull/2479.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2479/head:pull/2479 PR: https://git.openjdk.java.net/jdk/pull/2479
Re: RFR: 8261160: Add a deserialization JFR event [v4]
On Thu, 11 Feb 2021 15:53:06 GMT, Chris Hegarty wrote: >> This issue adds a new event to improve diagnostic information of Java >> deserialization. The event captures the details of deserialization activity >> from ObjectInputStream. The event details are similar to that of the serial >> filter, but is agnostic of whether a filter is installed or not. The event >> also captures the filter status, if there is one. > > Chris Hegarty has updated the pull request incrementally with one additional > commit since the last revision: > > Filter **C**onfigured Marked as reviewed by rriggs (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/2479
Re: RFR: 8261160: Add a deserialization JFR event [v4]
On Thu, 11 Feb 2021 15:53:06 GMT, Chris Hegarty wrote: >> This issue adds a new event to improve diagnostic information of Java >> deserialization. The event captures the details of deserialization activity >> from ObjectInputStream. The event details are similar to that of the serial >> filter, but is agnostic of whether a filter is installed or not. The event >> also captures the filter status, if there is one. > > Chris Hegarty has updated the pull request incrementally with one additional > commit since the last revision: > > Filter **C**onfigured LGTM - Marked as reviewed by dfuchs (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/2479
Re: RFR: 8261160: Add a deserialization JFR event [v3]
On Thu, 11 Feb 2021 15:45:33 GMT, Daniel Fuchs wrote:
>> Chris Hegarty has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Fix failing test
>
> src/jdk.jfr/share/classes/jdk/jfr/events/DeserializationEvent.java line 42:
>
>> 40:
>> 41: @Label("Filter configured")
>> 42: public boolean filterConfigured;
>
> Should that be "Filter Configured" for consistency?
Good catch. Fixed.
-
PR: https://git.openjdk.java.net/jdk/pull/2479
Re: RFR: 8261160: Add a deserialization JFR event [v3]
On Thu, 11 Feb 2021 15:28:07 GMT, Chris Hegarty wrote:
>> This issue adds a new event to improve diagnostic information of Java
>> deserialization. The event captures the details of deserialization activity
>> from ObjectInputStream. The event details are similar to that of the serial
>> filter, but is agnostic of whether a filter is installed or not. The event
>> also captures the filter status, if there is one.
>
> Chris Hegarty has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Fix failing test
src/jdk.jfr/share/classes/jdk/jfr/events/DeserializationEvent.java line 42:
> 40:
> 41: @Label("Filter configured")
> 42: public boolean filterConfigured;
Should that be "Filter Configured" for consistency?
-
PR: https://git.openjdk.java.net/jdk/pull/2479
Re: RFR: 8261160: Add a deserialization JFR event [v4]
> This issue adds a new event to improve diagnostic information of Java > deserialization. The event captures the details of deserialization activity > from ObjectInputStream. The event details are similar to that of the serial > filter, but is agnostic of whether a filter is installed or not. The event > also captures the filter status, if there is one. Chris Hegarty has updated the pull request incrementally with one additional commit since the last revision: Filter **C**onfigured - Changes: - all: https://git.openjdk.java.net/jdk/pull/2479/files - new: https://git.openjdk.java.net/jdk/pull/2479/files/5737ee7c..d764f75f Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=2479=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2479=02-03 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/2479.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2479/head:pull/2479 PR: https://git.openjdk.java.net/jdk/pull/2479
Re: RFR: 8261160: Add a deserialization JFR event [v3]
On Thu, 11 Feb 2021 15:28:07 GMT, Chris Hegarty wrote: >> This issue adds a new event to improve diagnostic information of Java >> deserialization. The event captures the details of deserialization activity >> from ObjectInputStream. The event details are similar to that of the serial >> filter, but is agnostic of whether a filter is installed or not. The event >> also captures the filter status, if there is one. > > Chris Hegarty has updated the pull request incrementally with one additional > commit since the last revision: > > Fix failing test Marked as reviewed by coffeys (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/2479
Re: RFR: 8261160: Add a deserialization JFR event [v3]
> This issue adds a new event to improve diagnostic information of Java > deserialization. The event captures the details of deserialization activity > from ObjectInputStream. The event details are similar to that of the serial > filter, but is agnostic of whether a filter is installed or not. The event > also captures the filter status, if there is one. Chris Hegarty has updated the pull request incrementally with one additional commit since the last revision: Fix failing test - Changes: - all: https://git.openjdk.java.net/jdk/pull/2479/files - new: https://git.openjdk.java.net/jdk/pull/2479/files/8ecf63ce..5737ee7c Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=2479=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2479=01-02 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/2479.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2479/head:pull/2479 PR: https://git.openjdk.java.net/jdk/pull/2479
Re: RFR: 8261160: Add a deserialization JFR event [v2]
On Thu, 11 Feb 2021 14:27:19 GMT, Roger Riggs wrote: >> Marked as reviewed by rriggs (Reviewer). > > As proposed, events are only created if there is a serialFilter in effect > (and enabled by JFR configuration). > Being able to create the events without a serialFilter in effect would be > useful for monitoring, especially if it could be controlled by a separate JFR > configuration option. (always, never, serial-filter , etc.) I updated the PR and addressed all comments so far. Specifically: @RogerRiggs The generation of the event is independent of whether the filter is set or not. I also added a piece of state to determine if a filter is set or not. I think it could be useful to analyse all Deserialisation events to, say, ensure that there are none operating without a filter ( and the `filterStatus` state is ambitious in the `null` case ). @coffeys I would like GlobalFilterTest to run regardless of whether or not the jfr module is present, but of course running the test with jfr enabled is desirable too, so I added a separate at test tag for that case. @egahlin Excellent suggestions on the naming, etc. I adopted all. And added a test to ensure that the creation and generation of the event does not inadvertently trigger class initialization if the filter rejects the attempt ( thanks @dfuch ) @dfuch Thanks for the improved label suggestion, it is now merged in. - PR: https://git.openjdk.java.net/jdk/pull/2479
Re: RFR: 8261160: Add a deserialization JFR event [v2]
> This issue adds a new event to improve diagnostic information of Java > deserialization. The event captures the details of deserialization activity > from ObjectInputStream. The event details are similar to that of the serial > filter, but is agnostic of whether a filter is installed or not. The event > also captures the filter status, if there is one. Chris Hegarty has updated the pull request incrementally with one additional commit since the last revision: Review comments - Changes: - all: https://git.openjdk.java.net/jdk/pull/2479/files - new: https://git.openjdk.java.net/jdk/pull/2479/files/ca0bcf44..8ecf63ce Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=2479=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2479=00-01 Stats: 124 lines in 5 files changed: 76 ins; 2 del; 46 mod Patch: https://git.openjdk.java.net/jdk/pull/2479.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2479/head:pull/2479 PR: https://git.openjdk.java.net/jdk/pull/2479
Re: RFR: 8261160: Add a deserialization JFR event
On Wed, 10 Feb 2021 20:30:02 GMT, Roger Riggs wrote: >> This issue adds a new event to improve diagnostic information of Java >> deserialization. The event captures the details of deserialization activity >> from ObjectInputStream. The event details are similar to that of the serial >> filter, but is agnostic of whether a filter is installed or not. The event >> also captures the filter status, if there is one. > > Marked as reviewed by rriggs (Reviewer). As proposed, events are only created if there is a serialFilter in effect (and enabled by JFR configuration). Being able to create the events without a serialFilter in effect would be useful for monitoring, especially if it could be controlled by a separate JFR configuration option. (always, never, serial-filter , etc.) - PR: https://git.openjdk.java.net/jdk/pull/2479
Re: RFR: 8261160: Add a deserialization JFR event
On Tue, 9 Feb 2021 12:35:27 GMT, Chris Hegarty wrote:
> This issue adds a new event to improve diagnostic information of Java
> deserialization. The event captures the details of deserialization activity
> from ObjectInputStream. The event details are similar to that of the serial
> filter, but is agnostic of whether a filter is installed or not. The event
> also captures the filter status, if there is one.
src/java.base/share/classes/java/io/ObjectInputStream.java line 1366:
> 1364: DeserializationEvent event = new DeserializationEvent();
> 1365: if (event.shouldCommit()) {
> 1366: event.filterStatus = status == null ? "n/a" : status.name();
We use null for missing value, so no need to have "n/a"
src/java.base/share/classes/java/io/ObjectInputStream.java line 1372:
> 1370: event.depth = depth;
> 1371: event.bytesRead = bytesRead;
> 1372: event.exception = Objects.toString(ex, "n/a");
You may want to change the name of the field to "exceptionMessage" to make it
clear it's the message, not the class.
src/jdk.jfr/share/classes/jdk/jfr/events/DeserializationEvent.java line 45:
> 43:
> 44: @Label ("Class")
> 45: public String clazz;
We typically use "type" when referring to a class.
src/jdk.jfr/share/classes/jdk/jfr/events/DeserializationEvent.java line 45:
> 43:
> 44: @Label ("Class")
> 45: public String clazz;
Is it possible to make the field of type Class?
src/jdk.jfr/share/classes/jdk/jfr/events/DeserializationEvent.java line 51:
> 49:
> 50: @Label ("Reference count")
> 51: public long totalObjectRefs;
"Reference count" sounds a bit like resource counter? Is that the case? If not,
perhaps "Object References" is better. We tried to avoid abbreviations. How
about naming the field "totalObjectReferences" or just "objectReferences"?
-
PR: https://git.openjdk.java.net/jdk/pull/2479
Re: RFR: 8261160: Add a deserialization JFR event
On Tue, 9 Feb 2021 12:35:27 GMT, Chris Hegarty wrote:
> This issue adds a new event to improve diagnostic information of Java
> deserialization. The event captures the details of deserialization activity
> from ObjectInputStream. The event details are similar to that of the serial
> filter, but is agnostic of whether a filter is installed or not. The event
> also captures the filter status, if there is one.
src/jdk.jfr/share/classes/jdk/jfr/events/DeserializationEvent.java line 35:
> 33:
> 34: @Category({"Java Development Kit", "Serialization"})
> 35: @Label("Deserialization events")
This seems to differ from the format of other event labels defined in this
package, e.g:
@Label("Process Start")
@Label("File Read")
...
What would you think of changing it to one of:
@Label("Deserialization")
@Label("Deserialized Object")
-
PR: https://git.openjdk.java.net/jdk/pull/2479
Re: RFR: 8261160: Add a deserialization JFR event
On Tue, 9 Feb 2021 12:35:27 GMT, Chris Hegarty wrote: > This issue adds a new event to improve diagnostic information of Java > deserialization. The event captures the details of deserialization activity > from ObjectInputStream. The event details are similar to that of the serial > filter, but is agnostic of whether a filter is installed or not. The event > also captures the filter status, if there is one. Marked as reviewed by rriggs (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/2479
Re: RFR: 8261160: Add a deserialization JFR event
On Tue, 9 Feb 2021 12:35:27 GMT, Chris Hegarty wrote: > This issue adds a new event to improve diagnostic information of Java > deserialization. The event captures the details of deserialization activity > from ObjectInputStream. The event details are similar to that of the serial > filter, but is agnostic of whether a filter is installed or not. The event > also captures the filter status, if there is one. Marked as reviewed by coffeys (Reviewer). test/jdk/java/io/Serializable/serialFilter/GlobalFilterTest.java line 50: > 48: * -Dexpected-jdk.serialFilter=java.** GlobalFilterTest > 49: * @run testng/othervm/policy=security.policy GlobalFilterTest > 50: * @run testng/othervm/policy=security.policy You may want to add a "@requires vm.hasJFR" condition to this test - PR: https://git.openjdk.java.net/jdk/pull/2479
Re: RFR: 8261160: Add a deserialization JFR event
On Tue, 9 Feb 2021 12:35:27 GMT, Chris Hegarty wrote: > This issue adds a new event to improve diagnostic information of Java > deserialization. The event captures the details of deserialization activity > from ObjectInputStream. The event details are similar to that of the serial > filter, but is agnostic of whether a filter is installed or not. The event > also captures the filter status, if there is one. The logging in ObjectInputStream remains conditional on whether a filter is installed, which that seems reasonable since the logging is filter specific, while the JRF event is not (but both carry effectively the same information). The new jdk.Deserialization event uses a String to carry the filterStatus value. The value could be represented by its enum ordinal, but then the tool consuming the event would need to map that back to its string value to be meaningful. - PR: https://git.openjdk.java.net/jdk/pull/2479
RFR: 8261160: Add a deserialization JFR event
This issue adds a new event to improve diagnostic information of Java deserialization. The event captures the details of deserialization activity from ObjectInputStream. The event details are similar to that of the serial filter, but is agnostic of whether a filter is installed or not. The event also captures the filter status, if there is one. - Commit messages: - minor cleanup; all tests passing - Unconditionally fire a deser event regardless of filter, and add test - commit 2 - commit 1 Changes: https://git.openjdk.java.net/jdk/pull/2479/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=2479=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8261160 Stats: 516 lines in 12 files changed: 496 ins; 5 del; 15 mod Patch: https://git.openjdk.java.net/jdk/pull/2479.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2479/head:pull/2479 PR: https://git.openjdk.java.net/jdk/pull/2479
