Re: RFR: 8261160: Add a deserialization JFR event [v2]

2021-02-11 Thread Chris Hegarty
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]

2021-02-11 Thread Chris Hegarty
> 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&pr=2479&range=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2479&range=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