RFR: JDK-8276422 Add command-line option to disable finalization
Pretty much what it says. The new option controls a static member in InstanceKlass that's consulted to determine whether the finalization machinery is activated for instances when a class is loaded. A new native method is added so that this state can be queried from Java. This is used to control whether a finalizer thread is created and to disable the `System` and `Runtime::runFinalization` methods. Includes tests for the above. ------------- Commit messages: - extraneous newline - Merge branch 'master' into JDK-8276422-disable-finalization-option - Simplify InvalidFinalizationOption test. - Change InvalidFinalizationOption test to driver mode. - Revert extraneous whitespace change to globals.hpp. - Renaming within the test class itself. - Rename invalid finalization option test. - Add test for invalid finalization option syntax or value. - Add @bug line to JFR finalization event test. - Test that no jdk.FinalizationStatistics events are generated when finalization is disabled - ... and 7 more: https://git.openjdk.java.net/jdk/compare/29e552c0...3836cc94 Changes: https://git.openjdk.java.net/jdk/pull/6442/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=6442&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8276422 Stats: 266 lines in 13 files changed: 249 ins; 0 del; 17 mod Patch: https://git.openjdk.java.net/jdk/pull/6442.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6442/head:pull/6442 PR: https://git.openjdk.java.net/jdk/pull/6442
On Thu, 18 Nov 2021 01:34:36 GMT, Stuart Marks <smarks@openjdk.org> wrote:
Pretty much what it says. The new option controls a static member in InstanceKlass that's consulted to determine whether the finalization machinery is activated for instances when a class is loaded. A new native method is added so that this state can be queried from Java. This is used to control whether a finalizer thread is created and to disable the `System` and `Runtime::runFinalization` methods. Includes tests for the above.
Hi Stuart, This all looks fine to me. The hotspot part needs a second reviewer (especially as I contributed a chunk of that code :) ). Thanks, David ------------- Marked as reviewed by dholmes (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/6442
On Thu, 18 Nov 2021 01:34:36 GMT, Stuart Marks <smarks@openjdk.org> wrote:
Pretty much what it says. The new option controls a static member in InstanceKlass that's consulted to determine whether the finalization machinery is activated for instances when a class is loaded. A new native method is added so that this state can be queried from Java. This is used to control whether a finalizer thread is created and to disable the `System` and `Runtime::runFinalization` methods. Includes tests for the above.
src/java.base/share/classes/java/lang/ref/Finalizer.java line 195:
193: 194: static { 195: if (Holder.ENABLED) {
Hello Stuart, My understanding of the the lazy `Holder` is that it's there to delay the static initialization of the code that's part of the `Holder`. In this case here, the `Holder` is being used right within the `static` block of the `Finalizer` class, that too as the first thing. In this case, is that `Holder` class necessary? ------------- PR: https://git.openjdk.java.net/jdk/pull/6442
On Thu, 18 Nov 2021 04:13:21 GMT, Jaikiran Pai <jpai@openjdk.org> wrote:
Pretty much what it says. The new option controls a static member in InstanceKlass that's consulted to determine whether the finalization machinery is activated for instances when a class is loaded. A new native method is added so that this state can be queried from Java. This is used to control whether a finalizer thread is created and to disable the `System` and `Runtime::runFinalization` methods. Includes tests for the above.
src/java.base/share/classes/java/lang/ref/Finalizer.java line 195:
193: 194: static { 195: if (Holder.ENABLED) {
Hello Stuart, My understanding of the the lazy `Holder` is that it's there to delay the static initialization of the code that's part of the `Holder`. In this case here, the `Holder` is being used right within the `static` block of the `Finalizer` class, that too as the first thing. In this case, is that `Holder` class necessary?
Huh, good catch! This was mostly left over from an earlier version of the flag that used system properties, which aren't initialized until after the Finalizer class is initialized. It might be the case that the Holder can be removed at this point, since the finalization-enabled bit is no longer in a system property and is in a native class member that should be available before the VM is started. I say "might" though because this occurs early in system startup, and weird things potentially happen. For example, suppose the first object with a finalizer is created before the Finalizer class is initialized. The VM will perform an upcall to Finalizer::register. An ordinary call to a static method will ensure the class is initialized before proceeding with the call, but this VM upcall is a special case.... I'll have to investigate this some more. ------------- PR: https://git.openjdk.java.net/jdk/pull/6442
On Thu, 18 Nov 2021 05:20:02 GMT, Stuart Marks <smarks@openjdk.org> wrote:
src/java.base/share/classes/java/lang/ref/Finalizer.java line 195:
193: 194: static { 195: if (Holder.ENABLED) {
Hello Stuart, My understanding of the the lazy `Holder` is that it's there to delay the static initialization of the code that's part of the `Holder`. In this case here, the `Holder` is being used right within the `static` block of the `Finalizer` class, that too as the first thing. In this case, is that `Holder` class necessary?
Huh, good catch! This was mostly left over from an earlier version of the flag that used system properties, which aren't initialized until after the Finalizer class is initialized.
It might be the case that the Holder can be removed at this point, since the finalization-enabled bit is no longer in a system property and is in a native class member that should be available before the VM is started.
I say "might" though because this occurs early in system startup, and weird things potentially happen. For example, suppose the first object with a finalizer is created before the Finalizer class is initialized. The VM will perform an upcall to Finalizer::register. An ordinary call to a static method will ensure the class is initialized before proceeding with the call, but this VM upcall is a special case.... I'll have to investigate this some more.
@stuart-marks not sure I see how anything is different here compared to the existing logic. The `Finalizer` class is explicitly initialized quite early in the init process, but if a preceding class's initialization created an object with a finalizer then that same upcall would be involved. ------------- PR: https://git.openjdk.java.net/jdk/pull/6442
On Thu, 18 Nov 2021 07:13:55 GMT, David Holmes <dholmes@openjdk.org> wrote:
Huh, good catch! This was mostly left over from an earlier version of the flag that used system properties, which aren't initialized until after the Finalizer class is initialized.
It might be the case that the Holder can be removed at this point, since the finalization-enabled bit is no longer in a system property and is in a native class member that should be available before the VM is started.
I say "might" though because this occurs early in system startup, and weird things potentially happen. For example, suppose the first object with a finalizer is created before the Finalizer class is initialized. The VM will perform an upcall to Finalizer::register. An ordinary call to a static method will ensure the class is initialized before proceeding with the call, but this VM upcall is a special case.... I'll have to investigate this some more.
@stuart-marks not sure I see how anything is different here compared to the existing logic. The `Finalizer` class is explicitly initialized quite early in the init process, but if a preceding class's initialization created an object with a finalizer then that same upcall would be involved.
Do we even have to have a flag on Java side? It looks like these calls are only done as the upcalls from VM, so we might just keep the flag on VM side? ------------- PR: https://git.openjdk.java.net/jdk/pull/6442
On Thu, 18 Nov 2021 07:27:30 GMT, Aleksey Shipilev <shade@openjdk.org> wrote:
@stuart-marks not sure I see how anything is different here compared to the existing logic. The `Finalizer` class is explicitly initialized quite early in the init process, but if a preceding class's initialization created an object with a finalizer then that same upcall would be involved.
Do we even have to have a flag on Java side? It looks like these calls are only done as the upcalls from VM, so we might just keep the flag on VM side?
@shipilev not sure what you mean by "a flag on the Java side". The Java code just queries the VM for the finalization enabled/disabled state and uses that to control things. ------------- PR: https://git.openjdk.java.net/jdk/pull/6442
On Thu, 18 Nov 2021 07:40:34 GMT, David Holmes <dholmes@openjdk.org> wrote:
Do we even have to have a flag on Java side? It looks like these calls are only done as the upcalls from VM, so we might just keep the flag on VM side?
@shipilev not sure what you mean by "a flag on the Java side". The Java code just queries the VM for the finalization enabled/disabled state and uses that to control things.
Yeah, "flag" is `Holder.ENABLED` here. I mean, are Java methods `registerFinalizer` and `runFinalization` called only by VM? If so, can VM check the whole thing on VM side, without going to Java and asking back from there? ------------- PR: https://git.openjdk.java.net/jdk/pull/6442
On Thu, 18 Nov 2021 07:44:05 GMT, Aleksey Shipilev <shade@openjdk.org> wrote:
@shipilev not sure what you mean by "a flag on the Java side". The Java code just queries the VM for the finalization enabled/disabled state and uses that to control things.
Yeah, "flag" is `Holder.ENABLED` here. I mean, are Java methods `registerFinalizer` and `runFinalization` called only by VM? If so, can VM check the whole thing on VM side, without going to Java and asking back from there?
`registerFinalizer` does not expect to be called and only uses the "flag" as a form of assertion. `runFinalization` is called from Java code. ------------- PR: https://git.openjdk.java.net/jdk/pull/6442
On Thu, 18 Nov 2021 07:52:18 GMT, David Holmes <dholmes@openjdk.org> wrote:
Yeah, "flag" is `Holder.ENABLED` here. I mean, are Java methods `registerFinalizer` and `runFinalization` called only by VM? If so, can VM check the whole thing on VM side, without going to Java and asking back from there?
`registerFinalizer` does not expect to be called and only uses the "flag" as a form of assertion.
`runFinalization` is called from Java code.
@dholmes-ora If the Finalizer class is initialized explicitly and at the right time, then maybe we can do away with the Holder class entirely. Can you point me to where this is done? ------------- PR: https://git.openjdk.java.net/jdk/pull/6442
On Thu, 18 Nov 2021 07:44:05 GMT, Aleksey Shipilev <shade@openjdk.org> wrote:
@shipilev not sure what you mean by "a flag on the Java side". The Java code just queries the VM for the finalization enabled/disabled state and uses that to control things.
Yeah, "flag" is `Holder.ENABLED` here. I mean, are Java methods `registerFinalizer` and `runFinalization` called only by VM? If so, can VM check the whole thing on VM side, without going to Java and asking back from there?
I think @shipilev asks a good question. This could be done completely in the VM without the changes to j.l.ref.Finalizer. The CLI option is for experimenting, at least in the short term, and should be benign to have the Finalizer thread running, it just won't do anything. ------------- PR: https://git.openjdk.java.net/jdk/pull/6442
On Thu, 18 Nov 2021 08:39:52 GMT, Alan Bateman <alanb@openjdk.org> wrote:
Yeah, "flag" is `Holder.ENABLED` here. I mean, are Java methods `registerFinalizer` and `runFinalization` called only by VM? If so, can VM check the whole thing on VM side, without going to Java and asking back from there?
I think @shipilev asks a good question. This could be done completely in the VM without the changes to j.l.ref.Finalizer. The CLI option is for experimenting, at least in the short term, and should be benign to have the Finalizer thread running, it just won't do anything.
Or, you could move the static initialization block that statrts the finalizer thread into the Finalizer.FinalizerThread class itself and then arrange for that class to be initialized explicitly immediately after the Finalizer class, but conditionally, only if the option to disable finalization was not specified... This way the Finalizer class could still be initialized early, but the thread would not be started if it is not needed. ------------- PR: https://git.openjdk.java.net/jdk/pull/6442
On Thu, 18 Nov 2021 14:53:38 GMT, Peter Levart <plevart@openjdk.org> wrote:
I think @shipilev asks a good question. This could be done completely in the VM without the changes to j.l.ref.Finalizer. The CLI option is for experimenting, at least in the short term, and should be benign to have the Finalizer thread running, it just won't do anything.
Or, you could move the static initialization block that statrts the finalizer thread into the Finalizer.FinalizerThread class itself and then arrange for that class to be initialized explicitly immediately after the Finalizer class, but conditionally, only if the option to disable finalization was not specified... This way the Finalizer class could still be initialized early, but the thread would not be started if it is not needed.
If you then need this "flag" in the assert of registerFinalizer and runFinalization, you could use unsafe.shouldBeInitialized(Finalizer.FinalizerThread.class) as a means to find out whether the flag was set or not... ------------- PR: https://git.openjdk.java.net/jdk/pull/6442
On Thu, 18 Nov 2021 15:05:49 GMT, Peter Levart <plevart@openjdk.org> wrote:
Or, you could move the static initialization block that statrts the finalizer thread into the Finalizer.FinalizerThread class itself and then arrange for that class to be initialized explicitly immediately after the Finalizer class, but conditionally, only if the option to disable finalization was not specified... This way the Finalizer class could still be initialized early, but the thread would not be started if it is not needed.
If you then need this "flag" in the assert of registerFinalizer and runFinalization, you could use unsafe.shouldBeInitialized(Finalizer.FinalizerThread.class) as a means to find out whether the flag was set or not...
The disable-finalization feature is a bit more than experimental. The goal is to provide a faithful representation of what the system will look like when finalization is removed. Of course most of that is objects' `finalize` methods not being called, but it also includes having no finalizer thread running, as well as having `runFinalization` (a public API) do nothing at all. Thus I think it's useful to have the flag visible to Java. ------------- PR: https://git.openjdk.java.net/jdk/pull/6442
On Thu, 18 Nov 2021 04:13:21 GMT, Jaikiran Pai <jpai@openjdk.org> wrote:
Stuart Marks has updated the pull request incrementally with one additional commit since the last revision:
Remove Finalizer.Holder class.
src/java.base/share/classes/java/lang/ref/Finalizer.java line 195:
193: 194: static { 195: if (Holder.ENABLED) {
Hello Stuart, My understanding of the the lazy `Holder` is that it's there to delay the static initialization of the code that's part of the `Holder`. In this case here, the `Holder` is being used right within the `static` block of the `Finalizer` class, that too as the first thing. In this case, is that `Holder` class necessary?
I pushed an update to remove the Holder class. It seems to continue to work fine. Thanks for pointing this out @jaikiran ! ------------- PR: https://git.openjdk.java.net/jdk/pull/6442
On Fri, 19 Nov 2021 00:14:34 GMT, Stuart Marks <smarks@openjdk.org> wrote:
src/java.base/share/classes/java/lang/ref/Finalizer.java line 195:
193: 194: static { 195: if (Holder.ENABLED) {
Hello Stuart, My understanding of the the lazy `Holder` is that it's there to delay the static initialization of the code that's part of the `Holder`. In this case here, the `Holder` is being used right within the `static` block of the `Finalizer` class, that too as the first thing. In this case, is that `Holder` class necessary?
I pushed an update to remove the Holder class. It seems to continue to work fine. Thanks for pointing this out @jaikiran !
Thank you Stuart, this changed version looks fine to me. ------------- PR: https://git.openjdk.java.net/jdk/pull/6442
On Thu, 18 Nov 2021 01:34:36 GMT, Stuart Marks <smarks@openjdk.org> wrote:
Pretty much what it says. The new option controls a static member in InstanceKlass that's consulted to determine whether the finalization machinery is activated for instances when a class is loaded. A new native method is added so that this state can be queried from Java. This is used to control whether a finalizer thread is created and to disable the `System` and `Runtime::runFinalization` methods. Includes tests for the above.
I only really reviewed the hotspot changes. There is nothing here to make the various GCs take advantage of finalization being disabled. Is the plan to leave that to followup changes? src/hotspot/share/oops/instanceKlass.hpp line 338:
336: 337: // Queries finalization state 338: static bool finalization_enabled() { return _finalization_enabled; }
Predicate functions like this are often named "is_xxx"; that idiom is common in this class. src/hotspot/share/prims/jvm.cpp line 694:
692: 693: JVM_ENTRY(jboolean, JVM_IsFinalizationEnabled(JNIEnv * env)) 694: return InstanceKlass::finalization_enabled() ? JNI_TRUE : JNI_FALSE;
missing indentation ------------- Changes requested by kbarrett (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/6442
On Thu, 18 Nov 2021 06:43:01 GMT, Kim Barrett <kbarrett@openjdk.org> wrote:
Pretty much what it says. The new option controls a static member in InstanceKlass that's consulted to determine whether the finalization machinery is activated for instances when a class is loaded. A new native method is added so that this state can be queried from Java. This is used to control whether a finalizer thread is created and to disable the `System` and `Runtime::runFinalization` methods. Includes tests for the above.
src/hotspot/share/prims/jvm.cpp line 694:
692: 693: JVM_ENTRY(jboolean, JVM_IsFinalizationEnabled(JNIEnv * env)) 694: return InstanceKlass::finalization_enabled() ? JNI_TRUE : JNI_FALSE;
missing indentation
I think this could just be `return InstanceKlass::finalization_enabled();`. There is lots of code in this file and elsewhere that assumes C++ `bool` converts to `jboolean` appropriately. ------------- PR: https://git.openjdk.java.net/jdk/pull/6442
On Thu, 18 Nov 2021 06:49:03 GMT, Kim Barrett <kbarrett@openjdk.org> wrote:
src/hotspot/share/prims/jvm.cpp line 694:
692: 693: JVM_ENTRY(jboolean, JVM_IsFinalizationEnabled(JNIEnv * env)) 694: return InstanceKlass::finalization_enabled() ? JNI_TRUE : JNI_FALSE;
missing indentation
I think this could just be `return InstanceKlass::finalization_enabled();`. There is lots of code in this file and elsewhere that assumes C++ `bool` converts to `jboolean` appropriately.
One typical way for VM to pass the arguments to the library is via private system properties. System::initPhase1 will save the VM properties in `jdk.internal.misc.VM` and filters out the private properties from the system properties returned from System::getProperties (see System::createProperties). You can query the flag via `jdk.internal.misc.VM.getProperty("jdk.finalization.disabled")` for example. I don't see any issue moving the Finalizer class initialization after initPhase1 since there is no finalizer during VM startup. ------------- PR: https://git.openjdk.java.net/jdk/pull/6442
On Thu, 18 Nov 2021 20:13:23 GMT, Mandy Chung <mchung@openjdk.org> wrote:
I think this could just be `return InstanceKlass::finalization_enabled();`. There is lots of code in this file and elsewhere that assumes C++ `bool` converts to `jboolean` appropriately.
One typical way for VM to pass the arguments to the library is via private system properties. System::initPhase1 will save the VM properties in `jdk.internal.misc.VM` and filters out the private properties from the system properties returned from System::getProperties (see System::createProperties).
You can query the flag via `jdk.internal.misc.VM.getProperty("jdk.finalization.disabled")` for example.
I don't see any issue moving the Finalizer class initialization after initPhase1 since there is no finalizer during VM startup.
I renamed the function to `is_finalization_enabled` per previous comment, and I also made these cleanups. ------------- PR: https://git.openjdk.java.net/jdk/pull/6442
On Thu, 18 Nov 2021 21:19:44 GMT, Stuart Marks <smarks@openjdk.org> wrote:
One typical way for VM to pass the arguments to the library is via private system properties. System::initPhase1 will save the VM properties in `jdk.internal.misc.VM` and filters out the private properties from the system properties returned from System::getProperties (see System::createProperties).
You can query the flag via `jdk.internal.misc.VM.getProperty("jdk.finalization.disabled")` for example.
I don't see any issue moving the Finalizer class initialization after initPhase1 since there is no finalizer during VM startup.
I renamed the function to `is_finalization_enabled` per previous comment, and I also made these cleanups.
Regarding using system properties, my initial prototype did this in the launcher, and it did run into the problem that the Finalizer class is initialized before system properties are available. That's why I created the Holder class, so that reading the property could be delayed until the first upcall to Finalizer::register. I suppose the initialization of Finalizer could be moved later, but that seems more invasive. The flag needs to be available in the VM in order to avoid upcalls for instances-with-finalizers in the first place. Alan had [suggested](https://bugs.openjdk.java.net/browse/JDK-8276422?focusedCommentId=14456185&p...) moving the argument processing into the VM, and David suggested putting the flag into InstanceKlass, which seems a sensible place to me. It's also reasonably accessible there to GC implementations, should they want to inspect it. ------------- PR: https://git.openjdk.java.net/jdk/pull/6442
On Thu, 18 Nov 2021 22:04:52 GMT, Stuart Marks <smarks@openjdk.org> wrote:
I renamed the function to `is_finalization_enabled` per previous comment, and I also made these cleanups.
Regarding using system properties, my initial prototype did this in the launcher, and it did run into the problem that the Finalizer class is initialized before system properties are available. That's why I created the Holder class, so that reading the property could be delayed until the first upcall to Finalizer::register. I suppose the initialization of Finalizer could be moved later, but that seems more invasive.
The flag needs to be available in the VM in order to avoid upcalls for instances-with-finalizers in the first place. Alan had [suggested](https://bugs.openjdk.java.net/browse/JDK-8276422?focusedCommentId=14456185&p...) moving the argument processing into the VM, and David suggested putting the flag into InstanceKlass, which seems a sensible place to me. It's also reasonably accessible there to GC implementations, should they want to inspect it.
Alan had suggested moving the argument processing into the VM, and David suggested putting the flag into InstanceKlass, which seems a sensible place to me. It's also reasonably accessible there to GC implementations, should they want to inspect it.
That's still all good. What I meant is for the VM to add a private system property (not the launcher) as to pass the flag to the library code. The precedence is like `sun.nio.MaxDirectMemorySize` or `java.lang.Integer.IntegerCache.high`. ------------- PR: https://git.openjdk.java.net/jdk/pull/6442
Hi Mandy, On 19/11/2021 6:16 am, Mandy Chung wrote:
On Thu, 18 Nov 2021 06:49:03 GMT, Kim Barrett <kbarrett@openjdk.org> wrote:
src/hotspot/share/prims/jvm.cpp line 694:
692: 693: JVM_ENTRY(jboolean, JVM_IsFinalizationEnabled(JNIEnv * env)) 694: return InstanceKlass::finalization_enabled() ? JNI_TRUE : JNI_FALSE;
missing indentation
I think this could just be `return InstanceKlass::finalization_enabled();`. There is lots of code in this file and elsewhere that assumes C++ `bool` converts to `jboolean` appropriately.
One typical way for VM to pass the arguments to the library is via private system properties. System::initPhase1 will save the VM properties in `jdk.internal.misc.VM` and filters out the private properties from the system properties returned from System::getProperties (see System::createProperties).
The Finalizer class is initialized before initPhase1() happens. So to use a property the Holder class had to be introduced to be initialized after initPhase1(). There is always a choice of having the VM push up a system property to the Java code, or the Java code calling down to query the VM. The VM call seems simpler/cheaper/cleaner in this case. Cheers, David
You can query the flag via `jdk.internal.misc.VM.getProperty("jdk.finalization.disabled")` for example.
I don't see any issue moving the Finalizer class initialization after initPhase1 since there is no finalizer during VM startup.
-------------
On Thu, 18 Nov 2021 07:16:56 GMT, Kim Barrett <kbarrett@openjdk.org> wrote:
There is nothing here to make the various GCs take advantage of finalization being disabled. Is the plan to leave that to followup changes?
@kimbarrett I provided the basic VM parts here. I'm not aware of what specifically a GC might optimise if it knows there can be no finalizers, but that seems like something the GC folk should look to providing as a follow up. Thanks.
src/hotspot/share/oops/instanceKlass.hpp line 338:
336: 337: // Queries finalization state 338: static bool finalization_enabled() { return _finalization_enabled; }
Predicate functions like this are often named "is_xxx"; that idiom is common in this class.
This was intended as an accessor function, similar to `count()` or `offset()` not a query as-in `is_shared_boot_class()`. As it is a boolean field you could convert it to a query instead. ------------- PR: https://git.openjdk.java.net/jdk/pull/6442
On Thu, 18 Nov 2021 01:34:36 GMT, Stuart Marks <smarks@openjdk.org> wrote:
Pretty much what it says. The new option controls a static member in InstanceKlass that's consulted to determine whether the finalization machinery is activated for instances when a class is loaded. A new native method is added so that this state can be queried from Java. This is used to control whether a finalizer thread is created and to disable the `System` and `Runtime::runFinalization` methods. Includes tests for the above.
From the brief look, it is OK. Minor nits.
src/hotspot/share/prims/jvm.cpp line 694:
692: 693: JVM_ENTRY(jboolean, JVM_IsFinalizationEnabled(JNIEnv * env)) 694: return InstanceKlass::finalization_enabled() ? JNI_TRUE : JNI_FALSE;
Suggestion: return InstanceKlass::finalization_enabled() ? JNI_TRUE : JNI_FALSE; ------------- PR: https://git.openjdk.java.net/jdk/pull/6442
On Thu, 18 Nov 2021 06:47:05 GMT, Aleksey Shipilev <shade@openjdk.org> wrote:
Stuart Marks has updated the pull request incrementally with one additional commit since the last revision:
Rename InstanceKlass::finalization_enabled to is_finalization_enabled. Minor cleanups.
src/hotspot/share/prims/jvm.cpp line 694:
692: 693: JVM_ENTRY(jboolean, JVM_IsFinalizationEnabled(JNIEnv * env)) 694: return InstanceKlass::finalization_enabled() ? JNI_TRUE : JNI_FALSE;
Suggestion:
return InstanceKlass::finalization_enabled() ? JNI_TRUE : JNI_FALSE;
Fixed. ------------- PR: https://git.openjdk.java.net/jdk/pull/6442
Pretty much what it says. The new option controls a static member in InstanceKlass that's consulted to determine whether the finalization machinery is activated for instances when a class is loaded. A new native method is added so that this state can be queried from Java. This is used to control whether a finalizer thread is created and to disable the `System` and `Runtime::runFinalization` methods. Includes tests for the above.
Stuart Marks has updated the pull request incrementally with one additional commit since the last revision: Include instanceKlass.hpp in arguments.cpp ------------- Changes: - all: https://git.openjdk.java.net/jdk/pull/6442/files - new: https://git.openjdk.java.net/jdk/pull/6442/files/3836cc94..911af0b1 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=6442&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=6442&range=00-01 Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/6442.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6442/head:pull/6442 PR: https://git.openjdk.java.net/jdk/pull/6442
Pretty much what it says. The new option controls a static member in InstanceKlass that's consulted to determine whether the finalization machinery is activated for instances when a class is loaded. A new native method is added so that this state can be queried from Java. This is used to control whether a finalizer thread is created and to disable the `System` and `Runtime::runFinalization` methods. Includes tests for the above.
Adding an option to disable finalization is part of [JEP 421](https://openjdk.java.net/jeps/421).
Stuart Marks has updated the pull request incrementally with one additional commit since the last revision: Rename InstanceKlass::finalization_enabled to is_finalization_enabled. Minor cleanups. ------------- Changes: - all: https://git.openjdk.java.net/jdk/pull/6442/files - new: https://git.openjdk.java.net/jdk/pull/6442/files/911af0b1..5df8bf9f Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=6442&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=6442&range=01-02 Stats: 4 lines in 3 files changed: 0 ins; 0 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/6442.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6442/head:pull/6442 PR: https://git.openjdk.java.net/jdk/pull/6442
On Thu, 18 Nov 2021 20:05:15 GMT, Stuart Marks <smarks@openjdk.org> wrote:
Pretty much what it says. The new option controls a static member in InstanceKlass that's consulted to determine whether the finalization machinery is activated for instances when a class is loaded. A new native method is added so that this state can be queried from Java. This is used to control whether a finalizer thread is created and to disable the `System` and `Runtime::runFinalization` methods. Includes tests for the above.
Adding an option to disable finalization is part of [JEP 421](https://openjdk.java.net/jeps/421).
Stuart Marks has updated the pull request incrementally with one additional commit since the last revision:
Rename InstanceKlass::finalization_enabled to is_finalization_enabled. Minor cleanups.
When the finalization is disabled, perhaps jcmd GC.finalizer_info should just be made as a nop in the VM. ------------- PR: https://git.openjdk.java.net/jdk/pull/6442
On Thu, 18 Nov 2021 21:15:11 GMT, Mandy Chung <mchung@openjdk.org> wrote:
When the finalization is disabled, perhaps jcmd GC.finalizer_info should just be made as a nop in the VM.
Would it be interesting (perhaps in a follow-up) for GC.finalizer_info to report that the given VM had finalization disabled? ------------- PR: https://git.openjdk.java.net/jdk/pull/6442
On Thu, 18 Nov 2021 23:36:23 GMT, Brent Christian <bchristi@openjdk.org> wrote:
When the finalization is disabled, perhaps jcmd GC.finalizer_info should just be made as a nop in the VM.
Yes that is a trivial change to add. @stuart-marks I can provide the code. You can choose whether to include in this PR or else we can do a follow-up. ------------- PR: https://git.openjdk.java.net/jdk/pull/6442
On Fri, 19 Nov 2021 01:03:22 GMT, David Holmes <dholmes@openjdk.org> wrote:
When the finalization is disabled, perhaps jcmd GC.finalizer_info should just be made as a nop in the VM.
Yes that is a trivial change to add. @stuart-marks I can provide the code. You can choose whether to include in this PR or else we can do a follow-up.
Seems simple enough. Is there any testing that needs to be done for this? Does jcmd output require CSR review? I guess there would be a compatibility issue if there were something that was parsing the output of jcmd. Or is it solely intended to be read by humans? ------------- PR: https://git.openjdk.java.net/jdk/pull/6442
On Fri, 19 Nov 2021 02:29:33 GMT, Stuart Marks <smarks@openjdk.org> wrote:
When the finalization is disabled, perhaps jcmd GC.finalizer_info should just be made as a nop in the VM.
Yes that is a trivial change to add. @stuart-marks I can provide the code. You can choose whether to include in this PR or else we can do a follow-up.
When the finalization is disabled, perhaps jcmd GC.finalizer_info should just be made as a nop in the VM.
Yes that is a trivial change to add. @stuart-marks I can provide the code. You can choose whether to include in this PR or else we can do a follow-up.
Seems simple enough. Is there any testing that needs to be done for this? Does jcmd output require CSR review? I guess there would be a compatibility issue if there were something that was parsing the output of jcmd. Or is it solely intended to be read by humans?
@stuart-marks No CSR needed for this as no output format is specified. Plus this command already has a simple text response when there are no finalizers queued. E.g. ```
../build/linux-x64-debug-finalization/images/jdk/bin/jcmd 27939 GC.finalizer_info 27939: No instances waiting for finalization found
so when finalization is disabled this just becomes: ```
../build/linux-x64-debug-finalization/images/jdk/bin/jcmd 28018 GC.finalizer_info 28018: Finalization is disabled
There is a test for this Dcmd, but it doesn't test the "nothing here" case so I don't think it is necessary to augment it for this case: `hotspot/jtreg/serviceability/dcmd/gc/FinalizerInfoTest.java` ------------- PR: https://git.openjdk.java.net/jdk/pull/6442
On Thu, 18 Nov 2021 20:05:15 GMT, Stuart Marks <smarks@openjdk.org> wrote:
Pretty much what it says. The new option controls a static member in InstanceKlass that's consulted to determine whether the finalization machinery is activated for instances when a class is loaded. A new native method is added so that this state can be queried from Java. This is used to control whether a finalizer thread is created and to disable the `System` and `Runtime::runFinalization` methods. Includes tests for the above.
Adding an option to disable finalization is part of [JEP 421](https://openjdk.java.net/jeps/421).
Stuart Marks has updated the pull request incrementally with one additional commit since the last revision:
Rename InstanceKlass::finalization_enabled to is_finalization_enabled. Minor cleanups.
Marked as reviewed by dholmes (Reviewer). ------------- PR: https://git.openjdk.java.net/jdk/pull/6442
Pretty much what it says. The new option controls a static member in InstanceKlass that's consulted to determine whether the finalization machinery is activated for instances when a class is loaded. A new native method is added so that this state can be queried from Java. This is used to control whether a finalizer thread is created and to disable the `System` and `Runtime::runFinalization` methods. Includes tests for the above.
Adding an option to disable finalization is part of [JEP 421](https://openjdk.java.net/jeps/421).
Stuart Marks has updated the pull request incrementally with one additional commit since the last revision: Remove Finalizer.Holder class. ------------- Changes: - all: https://git.openjdk.java.net/jdk/pull/6442/files - new: https://git.openjdk.java.net/jdk/pull/6442/files/5df8bf9f..e357eeec Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=6442&range=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=6442&range=02-03 Stats: 6 lines in 1 file changed: 0 ins; 2 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/6442.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6442/head:pull/6442 PR: https://git.openjdk.java.net/jdk/pull/6442
On Fri, 19 Nov 2021 00:14:18 GMT, Stuart Marks <smarks@openjdk.org> wrote:
Pretty much what it says. The new option controls a static member in InstanceKlass that's consulted to determine whether the finalization machinery is activated for instances when a class is loaded. A new native method is added so that this state can be queried from Java. This is used to control whether a finalizer thread is created and to disable the `System` and `Runtime::runFinalization` methods. Includes tests for the above.
Adding an option to disable finalization is part of [JEP 421](https://openjdk.java.net/jeps/421).
Stuart Marks has updated the pull request incrementally with one additional commit since the last revision:
Remove Finalizer.Holder class.
Good simplification. src/java.base/share/classes/java/lang/ref/Finalizer.java line 64:
62: } 63: 64: static final boolean ENABLED = isFinalizationEnabled();
private? ------------- Marked as reviewed by dholmes (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/6442
On Fri, 19 Nov 2021 00:59:10 GMT, David Holmes <dholmes@openjdk.org> wrote:
Stuart Marks has updated the pull request incrementally with one additional commit since the last revision:
Remove Finalizer.Holder class.
src/java.base/share/classes/java/lang/ref/Finalizer.java line 64:
62: } 63: 64: static final boolean ENABLED = isFinalizationEnabled();
private?
Yeah, probably should be private. Other stuff in this class is private except things that are used from outside. ------------- PR: https://git.openjdk.java.net/jdk/pull/6442
On Fri, 19 Nov 2021 00:14:18 GMT, Stuart Marks <smarks@openjdk.org> wrote:
Pretty much what it says. The new option controls a static member in InstanceKlass that's consulted to determine whether the finalization machinery is activated for instances when a class is loaded. A new native method is added so that this state can be queried from Java. This is used to control whether a finalizer thread is created and to disable the `System` and `Runtime::runFinalization` methods. Includes tests for the above.
Adding an option to disable finalization is part of [JEP 421](https://openjdk.java.net/jeps/421).
Stuart Marks has updated the pull request incrementally with one additional commit since the last revision:
Remove Finalizer.Holder class.
@stuart-marks : https://github.com/openjdk/jdk/pull/6469 (didn't intend to actually make a PR but clicked the wrong part of the button :) ) ------------- PR: https://git.openjdk.java.net/jdk/pull/6442
On Fri, 19 Nov 2021 00:14:18 GMT, Stuart Marks <smarks@openjdk.org> wrote:
Pretty much what it says. The new option controls a static member in InstanceKlass that's consulted to determine whether the finalization machinery is activated for instances when a class is loaded. A new native method is added so that this state can be queried from Java. This is used to control whether a finalizer thread is created and to disable the `System` and `Runtime::runFinalization` methods. Includes tests for the above.
Adding an option to disable finalization is part of [JEP 421](https://openjdk.java.net/jeps/421).
Stuart Marks has updated the pull request incrementally with one additional commit since the last revision:
Remove Finalizer.Holder class.
Marked as reviewed by kbarrett (Reviewer). ------------- PR: https://git.openjdk.java.net/jdk/pull/6442
On Fri, 19 Nov 2021 00:14:18 GMT, Stuart Marks <smarks@openjdk.org> wrote:
Pretty much what it says. The new option controls a static member in InstanceKlass that's consulted to determine whether the finalization machinery is activated for instances when a class is loaded. A new native method is added so that this state can be queried from Java. This is used to control whether a finalizer thread is created and to disable the `System` and `Runtime::runFinalization` methods. Includes tests for the above.
Adding an option to disable finalization is part of [JEP 421](https://openjdk.java.net/jeps/421).
Stuart Marks has updated the pull request incrementally with one additional commit since the last revision:
Remove Finalizer.Holder class.
Regarding **jcmd** updates, I'm thinking maybe this would be better handled separately. There is the potential to update to `GC.finalizer_info` discussed previously. Looking at the **jcmd** tool docs, it seems like `GC.run_finalization` also ought to be updated. And maybe one or more of the other commands (maybe `VM.flags` or `VM.info`?) ought to list the finalization enabled or disabled status. And of course the tool's doc will need to be updated as well. ------------- PR: https://git.openjdk.java.net/jdk/pull/6442
On Fri, 19 Nov 2021 20:13:06 GMT, Stuart Marks <smarks@openjdk.org> wrote:
Stuart Marks has updated the pull request incrementally with one additional commit since the last revision:
Remove Finalizer.Holder class.
Regarding **jcmd** updates, I'm thinking maybe this would be better handled separately. There is the potential to update to `GC.finalizer_info` discussed previously. Looking at the **jcmd** tool docs, it seems like `GC.run_finalization` also ought to be updated. And maybe one or more of the other commands (maybe `VM.flags` or `VM.info`?) ought to list the finalization enabled or disabled status. And of course the tool's doc will need to be updated as well.
@stuart-marks no issue with doing dcmd/jcmd changes separately, but I don't think we need to go too far with this. I had considered `GC.run_finalization` but it just says it calls `System.run_finalization` - so no change needed there as it will be documented in System.runFinalization. And `VM.flags` only reports `-XX` flag information. And `VM.info` doesn't seem appropriate for mentioning this either. So no further changes needed to the other Dcmds IMO and no need to update anything on the jcmd tool page either. ------------- PR: https://git.openjdk.java.net/jdk/pull/6442
On Fri, 19 Nov 2021 22:50:49 GMT, David Holmes <dholmes@openjdk.org> wrote:
Regarding **jcmd** updates, I'm thinking maybe this would be better handled separately. There is the potential to update to `GC.finalizer_info` discussed previously. Looking at the **jcmd** tool docs, it seems like `GC.run_finalization` also ought to be updated. And maybe one or more of the other commands (maybe `VM.flags` or `VM.info`?) ought to list the finalization enabled or disabled status. And of course the tool's doc will need to be updated as well.
@stuart-marks no issue with doing dcmd/jcmd changes separately, but I don't think we need to go too far with this. I had considered `GC.run_finalization` but it just says it calls `System.run_finalization` - so no change needed there as it will be documented in System.runFinalization. And `VM.flags` only reports `-XX` flag information. And `VM.info` doesn't seem appropriate for mentioning this either. So no further changes needed to the other Dcmds IMO and no need to update anything on the jcmd tool page either.
@dholmes-ora OK if you're confident that it's sufficient just to add `GC.finalizer_info` and nothing else, and no docs or additional testing, then I'll just drop in the code from that branch you posted. Of course I'll do a full build & test. ------------- PR: https://git.openjdk.java.net/jdk/pull/6442
On Fri, 19 Nov 2021 00:14:18 GMT, Stuart Marks <smarks@openjdk.org> wrote:
Pretty much what it says. The new option controls a static member in InstanceKlass that's consulted to determine whether the finalization machinery is activated for instances when a class is loaded. A new native method is added so that this state can be queried from Java. This is used to control whether a finalizer thread is created and to disable the `System` and `Runtime::runFinalization` methods. Includes tests for the above.
Adding an option to disable finalization is part of [JEP 421](https://openjdk.java.net/jeps/421).
Stuart Marks has updated the pull request incrementally with one additional commit since the last revision:
Remove Finalizer.Holder class.
Lib changes and tests look good ------------- Marked as reviewed by bchristi (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/6442
participants (10)
-
Alan Bateman
-
Aleksey Shipilev
-
Brent Christian
-
David Holmes
-
David Holmes
-
Jaikiran Pai
-
Kim Barrett
-
Mandy Chung
-
Peter Levart
-
Stuart Marks