Re: RFR: 8266936: Add a finalization JFR event [v2]
> Greetings, > > Object.finalize() was deprecated in JDK9. There is an ongoing effort to > replace and mitigate Object.finalize() uses in the JDK libraries; please see > https://bugs.openjdk.java.net/browse/JDK-8253568 for more information. > > We also like to assist users in replacing and mitigating uses in non-JDK code. > > Hence, this changeset adds a periodic JFR event to help identify which > classes are overriding Object.finalize(). > > Thanks > Markus Markus Grönlund has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 16 additional commits since the last revision: - merge - whitespace - update - update - whitespace - 8266936-alt: Add a finalization JFR event - shortcut calling Jfr::is_recording() - 8266936: Add a finalization JFR event - Code Source attribute for Finalizer event - whitespace - ... and 6 more: https://git.openjdk.java.net/jdk/compare/021c6513...d4d154e8 - Changes: - all: https://git.openjdk.java.net/jdk/pull/4731/files - new: https://git.openjdk.java.net/jdk/pull/4731/files/9a966f9f..d4d154e8 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=4731&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=4731&range=00-01 Stats: 10639 lines in 412 files changed: 6366 ins; 2700 del; 1573 mod Patch: https://git.openjdk.java.net/jdk/pull/4731.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4731/head:pull/4731 PR: https://git.openjdk.java.net/jdk/pull/4731
Re: RFR: 8266936: Add a finalization JFR event [v2]
On Wed, 19 May 2021 17:57:11 GMT, Erik Gahlin wrote: > > I was also thinking if this event should be implemented in the VM in order > > to avoid some performance overhead such as object allocation. Erik, what is > > the benefit of implementing in in the VM? > > No startup cost, no allocation and there are callbacks when a class gets > unloaded, so it's probably easier to clear any table where the statistics is > held. Thanks for the confirmation. This is performance sensitive area and so it's worth considering doing it in the VM. In either case, performance measurement of the overhead will tell. - PR: https://git.openjdk.java.net/jdk/pull/4101
Re: RFR: 8266936: Add a finalization JFR event [v2]
On Tue, 18 May 2021 22:41:06 GMT, Brent Christian wrote: >> Please review this enhancement to add a new JFR event, generated whenever a >> finalizer is run. >> (The makeup is similar to the Deserialization event, >> [JDK-8261160](https://bugs.openjdk.java.net/browse/JDK-8261160).) >> >> The event's only datum (beyond those common to all jfr events) is the class >> of the object that was finalized. >> >> The Category for the event: >> `"Java Virtual Machine" / "GC" / "Finalization"` >> is what made sense to me, even though the event is generated from library >> code. >> >> Along with the new regtest, I added a run mode to the basic finalizer test >> to enable jfr. >> Automated testing looks good so far. >> >> Thanks, >> -Brent > > Brent Christian has updated the pull request incrementally with one > additional commit since the last revision: > > Test flag should be volatile > I was also thinking if this event should be implemented in the VM in order to > avoid some performance overhead such as object allocation. Erik, what is the > benefit of implementing in in the VM? No startup cost, no allocation and there are callbacks when a class gets unloaded, so it's probably easier to clear any table where the statistics is held. - PR: https://git.openjdk.java.net/jdk/pull/4101
Re: RFR: 8266936: Add a finalization JFR event [v2]
On Wed, 19 May 2021 09:00:27 GMT, Alan Bateman wrote: > I wonder if there needs to be one event per finalized object? I also concern with the performance overhead of one event per finalized object. The primary goal of this JFR event is to help identifying the use of finalizers in existing libraries/applications and prepare them to migrate away from using finalization. As well-known, the finalization mechanism is inherently problematic. > Perhaps a counter per class would be as useful, i.e. > jdk.FinalizationStatistics, and if it could be implemented in the VM, without > other implications, that would be great. Therefore, a counter per class would be useful as it identifies the usage of finalizers while providing the number of objects per class pending for finalization (see `ReferenceQueue::enqueue` and `ReferenceQueue::reallyPoll` where it keeps track of the pending for finalization counter). Another option is to go with a simple approach - just report the aggregated number of `Finalizer` objects per class which still meets the primary goal to identify what existing code uses finalizers and the counter gives the users an additional information how many finalizers are created. BTW the number of finalizer invocation is not more useful than the number of `Finalizer` instances unless we provide both counters so that the users can determine the number of objects pending for finalization. > Such an event could be enabled by default and provide much greater value than > an event that users would need to know about and configure themselves, which > 99,99% of all user will not do. I agree an event enabled by default is more useful provided that the performance overhead is insignificant. I was also thinking if this event should be implemented in the VM in order to avoid some performance overhead such as object allocation. Erin, what is the benefit of implementing in in the VM? - PR: https://git.openjdk.java.net/jdk/pull/4101
Re: RFR: 8266936: Add a finalization JFR event [v2]
On Tue, 18 May 2021 22:41:06 GMT, Brent Christian wrote: >> Please review this enhancement to add a new JFR event, generated whenever a >> finalizer is run. >> (The makeup is similar to the Deserialization event, >> [JDK-8261160](https://bugs.openjdk.java.net/browse/JDK-8261160).) >> >> The event's only datum (beyond those common to all jfr events) is the class >> of the object that was finalized. >> >> The Category for the event: >> `"Java Virtual Machine" / "GC" / "Finalization"` >> is what made sense to me, even though the event is generated from library >> code. >> >> Along with the new regtest, I added a run mode to the basic finalizer test >> to enable jfr. >> Automated testing looks good so far. >> >> Thanks, >> -Brent > > Brent Christian has updated the pull request incrementally with one > additional commit since the last revision: > > Test flag should be volatile My 0.02$ ;-) I really like the idea of a jdk.FinalizationStatistics event. The devil is (as always) in the details. Multiple concerns have been raised above, which benefit from being separated out: 1) Raising an event per invocation of each individual finalizer may be costly when JFR is enabled (since there could be an extremely large number of these), as well as a little unwieldy for a human observer. Aggregating invocations of finalizers and reporting periodically seems like a nice solution to this. 2) A jdk.FinalizationStatistics event that provides an aggregate count of *all* finalizer invocations seems most straightforward, but less useful. A jdk.FinalizationStatistics event that provides per-class invocation metrics seems more useful, but at the expense of a more complex event structure. Maybe model jdk.FinalizationStatistics as a tuple of Class and long (count) - periodically committing multiple jdk.FinalizationStatistics events, one event per class? ( or was the thought to somehow aggregate all these per-class metrics into a single jdk.FinalizationStatistics event? ) 3) If we keep the currently proposed semantic - capturing actual invocation/queueing counts (rather than registrations), then I see no reason why the implementation of a jdk.FinalizationStatistics needs to be in the JVM. The metrics can be captured in Java code and reported in a similar way to the container metrics (being proposed in [PR 3126][PR3126]). Surely, this would be more straightforward to implement and maintain, no? 4) The startup cost of JFR. I dunno enough about this, but what I do know is that a handler needs to be spun per Java-event, so maybe this has some bearing on the decision of no.3 above? [PR3126]: https://github.com/openjdk/jdk/pull/3126 - PR: https://git.openjdk.java.net/jdk/pull/4101
Re: RFR: 8266936: Add a finalization JFR event [v2]
On Wed, 19 May 2021 07:50:55 GMT, Erik Gahlin wrote: > I wonder if there needs to be one event per finalized object? > > Perhaps a counter per class would be as useful, i.e. > jdk.FinalizationStatistics, and if it could be implemented in the VM, without > other implications, that would be great. > > Such an event could be enabled by default and provide much greater value than > an event that users would need to know about and configure themselves, which > 99,99% of all user will not do. So some statistics or periodic event that N instances of class C were registered or queued? I think you are right that this would be a lot more valuable. - PR: https://git.openjdk.java.net/jdk/pull/4101
Re: RFR: 8266936: Add a finalization JFR event [v2]
On Tue, 18 May 2021 22:41:06 GMT, Brent Christian wrote: >> Please review this enhancement to add a new JFR event, generated whenever a >> finalizer is run. >> (The makeup is similar to the Deserialization event, >> [JDK-8261160](https://bugs.openjdk.java.net/browse/JDK-8261160).) >> >> The event's only datum (beyond those common to all jfr events) is the class >> of the object that was finalized. >> >> The Category for the event: >> `"Java Virtual Machine" / "GC" / "Finalization"` >> is what made sense to me, even though the event is generated from library >> code. >> >> Along with the new regtest, I added a run mode to the basic finalizer test >> to enable jfr. >> Automated testing looks good so far. >> >> Thanks, >> -Brent > > Brent Christian has updated the pull request incrementally with one > additional commit since the last revision: > > Test flag should be volatile I wonder if there needs to be one event per finalized object? Perhaps a counter per class would be as useful, i.e. jdk.FinalizationStatistics, and if it could be implemented in the VM, without other implications, that would be great. Such an event could be enabled by default and provide much greater value than an event that users would need to know about and configure themselves, which 99,99% of all user will not do. - PR: https://git.openjdk.java.net/jdk/pull/4101
Re: RFR: 8266936: Add a finalization JFR event [v2]
On Wed, 19 May 2021 06:20:59 GMT, Erik Gahlin wrote: > This looks useful, but I am worried about the performance impact: > > * The added allocation for every object that is finalized. > * The event being enabled in the default configuration. > > The default configuration must be safe to use even in pathological cases, > i.e. an application with lots of objects being finalized. It's probably too > much overhead (or number of events) for the profile configuration as well. > > There is also the added startup cost of JFR. One event may not matter that > much, but they all add up. We need to put in place a mechanism that doesn't > rely on bytecode instrumentation at runtime. There is an enhancement for > that, but it requires some engineering first. I'm a bit worried by this too. Does it need to be enabled by default? Can we put a static final instance of FinalizerEvent in that class that can be used to test if the event is enabled so that it doesn't create a FinalizerEvent when disabled? Is it worth exploring doing have an event in the VM, in register_finalizer, instead? - PR: https://git.openjdk.java.net/jdk/pull/4101
Re: RFR: 8266936: Add a finalization JFR event [v2]
On Tue, 18 May 2021 22:41:06 GMT, Brent Christian wrote: >> Please review this enhancement to add a new JFR event, generated whenever a >> finalizer is run. >> (The makeup is similar to the Deserialization event, >> [JDK-8261160](https://bugs.openjdk.java.net/browse/JDK-8261160).) >> >> The event's only datum (beyond those common to all jfr events) is the class >> of the object that was finalized. >> >> The Category for the event: >> `"Java Virtual Machine" / "GC" / "Finalization"` >> is what made sense to me, even though the event is generated from library >> code. >> >> Along with the new regtest, I added a run mode to the basic finalizer test >> to enable jfr. >> Automated testing looks good so far. >> >> Thanks, >> -Brent > > Brent Christian has updated the pull request incrementally with one > additional commit since the last revision: > > Test flag should be volatile This looks useful, but I am worried about the performance impact: - The added allocation for every object that is finalized. - The event being enabled in the default configuration. The default configuration must be safe to use even in pathological cases, i.e. an application with lots of objects being finalized. It's probably too much overhead (or number of events) for the profile configuration as well. There is also the added startup cost of JFR. One event may not matter that much, but they all add up. We need to put in place a mechanism that doesn't rely on bytecode instrumentation at runtime. There is an enhancement for that, but it requires some engineering first. - PR: https://git.openjdk.java.net/jdk/pull/4101
Re: RFR: 8266936: Add a finalization JFR event [v2]
> Please review this enhancement to add a new JFR event, generated whenever a > finalizer is run. > (The makeup is similar to the Deserialization event, > [JDK-8261160](https://bugs.openjdk.java.net/browse/JDK-8261160).) > > The event's only datum (beyond those common to all jfr events) is the class > of the object that was finalized. > > The Category for the event: > `"Java Virtual Machine" / "GC" / "Finalization"` > is what made sense to me, even though the event is generated from library > code. > > Along with the new regtest, I added a run mode to the basic finalizer test to > enable jfr. > Automated testing looks good so far. > > Thanks, > -Brent Brent Christian has updated the pull request incrementally with one additional commit since the last revision: Test flag should be volatile - Changes: - all: https://git.openjdk.java.net/jdk/pull/4101/files - new: https://git.openjdk.java.net/jdk/pull/4101/files/200268ab..e0ef383b Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=4101&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=4101&range=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/4101.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4101/head:pull/4101 PR: https://git.openjdk.java.net/jdk/pull/4101
