Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JDK-8276422 Add command-line option to disable finalization #6442

Conversation

stuart-marks
Copy link
Member

@stuart-marks stuart-marks commented Nov 18, 2021

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.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8276422: Add command-line option to disable finalization

Reviewers

Contributors

  • David Holmes <dholmes@openjdk.org>
  • Brent Christian <bchristi@openjdk.org>

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/6442/head:pull/6442
$ git checkout pull/6442

Update a local copy of the PR:
$ git checkout pull/6442
$ git pull https://git.openjdk.java.net/jdk pull/6442/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 6442

View PR using the GUI difftool:
$ git pr show -t 6442

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/6442.diff

@bridgekeeper
Copy link

bridgekeeper bot commented Nov 18, 2021

👋 Welcome back smarks! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@stuart-marks
Copy link
Member Author

stuart-marks commented Nov 18, 2021

/csr
/contributor add dholmes
/contributor add bchristi

@openjdk
Copy link

openjdk bot commented Nov 18, 2021

@stuart-marks The following labels will be automatically applied to this pull request:

  • build
  • core-libs
  • hotspot

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added hotspot hotspot-dev@openjdk.org build build-dev@openjdk.org core-libs core-libs-dev@openjdk.org csr Pull request needs approved CSR before integration labels Nov 18, 2021
@openjdk
Copy link

openjdk bot commented Nov 18, 2021

@stuart-marks this pull request will not be integrated until the CSR request JDK-8276773 for issue JDK-8276422 has been approved.

@openjdk
Copy link

openjdk bot commented Nov 18, 2021

@stuart-marks
Contributor David Holmes <dholmes@openjdk.org> successfully added.

@openjdk
Copy link

openjdk bot commented Nov 18, 2021

@stuart-marks
Contributor Brent Christian <bchristi@openjdk.org> successfully added.

@stuart-marks stuart-marks marked this pull request as ready for review Nov 18, 2021
@openjdk openjdk bot added the rfr Pull request is ready for review label Nov 18, 2021
@mlbridge
Copy link

mlbridge bot commented Nov 18, 2021

Webrevs

@dholmes-ora
Copy link
Member

dholmes-ora commented Nov 18, 2021

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.

@magicus
Copy link
Member

magicus commented Nov 18, 2021

/label remove build

@openjdk openjdk bot removed the build build-dev@openjdk.org label Nov 18, 2021
@openjdk
Copy link

openjdk bot commented Nov 18, 2021

@magicus
The build label was successfully removed.

@mlchung
Copy link
Member

mlchung commented Nov 18, 2021

When the finalization is disabled, perhaps jcmd GC.finalizer_info should just be made as a nop in the VM.

@mlbridge
Copy link

mlbridge bot commented Nov 18, 2021

Mailing list message from David Holmes on hotspot-dev:

Hi Mandy,

On 19/11/2021 6:16 am, Mandy Chung wrote:

On Thu, 18 Nov 2021 06:49:03 GMT, Kim Barrett <kbarrett at 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

@bchristi-git
Copy link
Member

bchristi-git commented Nov 18, 2021

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?

Copy link
Member

@dholmes-ora dholmes-ora left a comment

Good simplification.

static class Holder {
static final boolean ENABLED = isFinalizationEnabled();
}
static final boolean ENABLED = isFinalizationEnabled();
Copy link
Member

@dholmes-ora dholmes-ora Nov 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

private?

Copy link
Member Author

@stuart-marks stuart-marks Nov 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, probably should be private. Other stuff in this class is private except things that are used from outside.

@dholmes-ora
Copy link
Member

dholmes-ora commented Nov 19, 2021

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.

@dholmes-ora
Copy link
Member

dholmes-ora commented Nov 19, 2021

@stuart-marks : #6469 (didn't intend to actually make a PR but clicked the wrong part of the button :) )

@stuart-marks
Copy link
Member Author

stuart-marks commented Nov 19, 2021

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?

@dholmes-ora
Copy link
Member

dholmes-ora commented Nov 19, 2021

@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

@stuart-marks
Copy link
Member Author

stuart-marks commented Nov 19, 2021

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.

Copy link
Member

@bchristi-git bchristi-git left a comment

Lib changes and tests look good

@dholmes-ora
Copy link
Member

dholmes-ora commented Nov 19, 2021

@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.

@stuart-marks
Copy link
Member Author

stuart-marks commented Nov 20, 2021

@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.

@openjdk openjdk bot removed the csr Pull request needs approved CSR before integration label Nov 22, 2021
@openjdk
Copy link

openjdk bot commented Nov 22, 2021

@stuart-marks This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8276422: Add command-line option to disable finalization

Co-authored-by: David Holmes <dholmes@openjdk.org>
Co-authored-by: Brent Christian <bchristi@openjdk.org>
Reviewed-by: dholmes, kbarrett, bchristi

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 52 new commits pushed to the master branch:

  • ca31ed5: 8275448: [REDO] AArch64: Implement string_compare intrinsic in SVE
  • 4ff4301: 8224922: Access JavaFileObject from Element(s)
  • 0a9e76c: 8277485: Zero: Fix fast{i,f}access_0 bytecodes handling
  • 1c215f3: 8272773: Configurable card table card size
  • 1d7cef3: 8276662: Scalability bottleneck in SymbolTable::lookup_common()
  • c79a485: 8277494: [BACKOUT] JDK-8276150 Quarantined jpackage apps are labeled as "damaged"
  • 2ab43ec: 8273544: Increase test coverage for snippets
  • 2d4af22: 8277370: configure script cannot distinguish WSL version
  • a3406a1: 8277092: TestMetaspaceAllocationMT2.java#ndebug-default fails with "RuntimeException: Committed seems high: NNNN expected at most MMMM"
  • e47cc81: 8275386: Change nested classes in jdk.jlink to static nested classes
  • ... and 42 more: https://git.openjdk.java.net/jdk/compare/29e552c03a2825f9526330072668a1d63ac68fd4...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Nov 22, 2021
@stuart-marks
Copy link
Member Author

stuart-marks commented Dec 8, 2021

/integrate

@openjdk
Copy link

openjdk bot commented Dec 8, 2021

Going to push as commit d7ad546.
Since your change was applied there have been 292 commits pushed to the master branch:

  • ec7cb6d: 8276447: Deprecate finalization-related methods for removal
  • 3c2951f: 8275771: JDK source code contains redundant boolean operations in jdk.compiler and langtools
  • 3d61372: 8278363: Create extented container test groups
  • 716c2e1: 8278368: ProblemList tools/jpackage/share/MultiNameTwoPhaseTest.java on macosx-x64
  • a8a1fbc: 8278068: Fix next-line modifier (snippet markup)
  • 061017a: 8273175: Add @SInCE tags to the DocTree.Kind enum constants
  • d7c283a: 8275233: Incorrect line number reported in exception stack trace thrown from a lambda expression
  • 3955b03: 8277328: jdk/jshell/CommandCompletionTest.java failures on Windows
  • 5a036ac: 8277990: NMT: Remove NMT shutdown capability
  • 7217cb7: 8274883: (se) Selector.open throws IAE when the default file system provider is changed to a custom provider
  • ... and 282 more: https://git.openjdk.java.net/jdk/compare/29e552c03a2825f9526330072668a1d63ac68fd4...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot closed this Dec 8, 2021
@openjdk openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Dec 8, 2021
@openjdk
Copy link

openjdk bot commented Dec 8, 2021

@stuart-marks Pushed as commit d7ad546.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core-libs core-libs-dev@openjdk.org hotspot hotspot-dev@openjdk.org integrated Pull request has been integrated
10 participants