8291718: Remove mark_for_deoptimization from klass unloading #9713

xmas92 · 2022-08-02T14:30:07Z

@stefank suggested creating a separate issue for this part of JDK-8291237

Description from the original issue:

The klass unloading code currently sets the mark for deoptimzation flag on nmethods that depend on the klass being unloaded. That however is all it does, there is no guarantee that those nmethods will become not entrant and make_deoptimized. The current implementation may if some future deoptimization happens to trigger see these marks as it scans the whole code cache, but it is never triggered from klass unloading.

Me and [~eosterlund] discussed this and this behaviour seems like some leftover from earlier versions of klass unloading. There should be no reason to eliminate the nmethods that depend on unloaded klasses except to eliminate dead code. It is probably better to let other mechanisms decide if an nmethod with dead code should be deoptimized than the klass unloading.

It might be a future improvement to have some stats which record that an nmethod had some dependency to an unloaded klass which might effect the heuristic for selecting what methods are deoptimized.

Testing: Tier 1-7

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8291718: Remove mark_for_deoptimization from klass unloading

Reviewers

Erik Österlund (@fisk - Reviewer)
Dean Long (@dean-long - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/9713/head:pull/9713
$ git checkout pull/9713

Update a local copy of the PR:
$ git checkout pull/9713
$ git pull https://git.openjdk.org/jdk pull/9713/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 9713

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/9713.diff

bridgekeeper · 2022-08-02T14:31:12Z

👋 Welcome back xmas92! 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.

openjdk · 2022-08-02T14:35:39Z

@xmas92 The following label will be automatically applied to this pull request:

hotspot

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

mlbridge · 2022-08-02T14:39:06Z

Webrevs

fisk

Looks good.

openjdk · 2022-08-02T17:31:27Z

@xmas92 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:

8291718: Remove mark_for_deoptimization from klass unloading

Reviewed-by: eosterlund, dlong

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 107 new commits pushed to the master branch:

e89abb7: 8292190: [IR Framework] Remove redundant regex matching for failing counts constraints
77cd917: Merge
967a28c: 8292260: [BACKOUT] JDK-8279219: [REDO] C2 crash when allocating array of size too large
1c1c441: 8292072: NMT: repurpose Tracking overhead counter as global malloc counter
d546d13: 8292319: ProblemList 2 runtime/cds/appcds tests due to JDK-8292313
8353a33: 8286313: [macos] Voice over reads the boolean value as null in the JTable
e70747b: 8292305: [BACKOUT] JDK-8289208 Test DrawRotatedStringUsingRotatedFont.java occasionally crashes on MacOS
22d6d31: 8284313: Improve warning messages when CDS archive fails to load
00decca: 8289208: Test DrawRotatedStringUsingRotatedFont.java occasionally crashes on MacOS
9f8cc42: 8292218: Convert PackageEntryTable to ResourceHashtable
... and 97 more: https://git.openjdk.org/jdk/compare/aa557b9b01571d7614f54d341c5cd8bf36cdacee...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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@fisk, @dean-long) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

dean-long · 2022-08-02T21:30:14Z

src/hotspot/share/code/dependencyContext.cpp

+   set_dependencies(NULL);
+   int removed = 0;
+   while (b != NULL) {
+     b = release_and_get_next_not_unloading(b);


If we are being called by class unloading, finding an nmethod with nm->is_alive() returning true would be fatal, wouldn't it? Could we make it clear this function is used for unloading, and add an assert that !nm->is_alive() && nm->has_flushed_dependencies()?

You are correct that nm should all be at least unloading, as if a klass is unloading all dependent nmethods should be unloading. So this code should boil down to:

void DependencyContext::remove_all_dependents() { nmethodBucket* b = dependencies_not_unloading(); set_dependencies(NULL); assert(b == nullptr, "All dependents should be unloading"); }

As dependencies_not_unloading() will unlink and release every dependent that is unloading.
Have run stress tests on that change and no violations were found.
Will push and test this change.

You are correct that nm should all be at least unloading, as if a klass is unloading all dependent nmethods should be unloading. So this code should boil down to:

void DependencyContext::remove_all_dependents() { nmethodBucket* b = dependencies_not_unloading(); set_dependencies(NULL); assert(b == nullptr, "All dependents should be unloading"); }

As dependencies_not_unloading() will unlink and release every dependent that is unloading. Have run stress tests on that change and no violations were found. Will push and test this change.

From testing this seems not to be the case. Will investigate some more. So there are dependents that do no become unloading when the klass unloads.

I would like to find out more in detail what you want to ensure, or situation you are afraid that might occur.

I've tried to investigate what types of dependent nmethods are not unloading when the the klass with a dependency is unloaded. I suspect it has to do with class loading and compilation order. From what I understand the nmethod will only be unloading if it has a reference to a dead oop inside it. So the compiler must generate code which is dependent without having an oop baked in.

I do find it difficult to reason about the lifetime of these dependencies (at least the logical part, the release, purge cycle of the memory seems pretty straight forward) and nmethods (the sweeper removal does make the nmethod lifecycle a lot easier to reason about tough).

I am really interested in what you think. As this change is just always running an edge case which can happen in mainline. This just makes it default. Hotspot seems to have relied on this interaction being this way for some time.

The windows machines finally woke up and the patch passed tier 1-7

My concern was that we could be removing the dependencies from nmethods that are still using them. But if that was the case, the old code would have the same problem.

The compiler dependencies are protecting against loading of a class that is a subtype of X. If X is unloaded, then we can be pretty sure nobody is going to load a subclass of X.

Isn't there something we can assert about the state of these nmethods? Maybe unlinked and !in_use?

Seems like the culprit it CodeCache::UnloadingScope. Currently the CodeCache::UnloadingScope does not encompass the klass unloading (different for each GC, some do some don't). Only the CodeCache unloading. This means that the is_unloading data is not up to date.

After modifying serial to have the UnloadingScope covering both klass unloading and nmethod unloading this code seems to work.

void DependencyContext::remove_all_dependents() { nmethodBucket* b = dependencies_not_unloading(); set_dependencies(NULL); assert(b == nullptr, "All dependents should be unloading"); }

The options here are to either fix the scopes for all GCs so it covers both, or introduce a slow is_unloading for the assert which does not use the cached unloading value. The second option still need to be GC aware as it requires the GCs notion of is_alive. The first option seems the best as it will be more correct with respect to the internal vm state. It will probably be better at catching future bugs as well.

This reverts commit 0141b28.

xmas92 · 2022-08-04T15:08:08Z

The perf counters were double counting some deallocations. The release and purge code was already handling it. This code would just double count any not_unloading dependencies.

xmas92 · 2022-08-08T15:38:12Z

Extended the UnloadingScope.
Seems to work for tier 1-3. Will test tier 1-7

xmas92 · 2022-08-09T05:07:17Z

Tier 1-7 testing done.

xmas92 · 2022-08-15T06:33:39Z

/integrate

openjdk · 2022-08-15T06:35:59Z

@xmas92
Your change (at version f6dcef0) is now ready to be sponsored by a Committer.

fisk · 2022-08-15T09:51:57Z

/sponsor

openjdk · 2022-08-15T09:53:50Z

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

9d7c13e: 8155246: Throw error if default java.security file is missing
e89abb7: 8292190: [IR Framework] Remove redundant regex matching for failing counts constraints
77cd917: Merge
967a28c: 8292260: [BACKOUT] JDK-8279219: [REDO] C2 crash when allocating array of size too large
1c1c441: 8292072: NMT: repurpose Tracking overhead counter as global malloc counter
d546d13: 8292319: ProblemList 2 runtime/cds/appcds tests due to JDK-8292313
8353a33: 8286313: [macos] Voice over reads the boolean value as null in the JTable
e70747b: 8292305: [BACKOUT] JDK-8289208 Test DrawRotatedStringUsingRotatedFont.java occasionally crashes on MacOS
22d6d31: 8284313: Improve warning messages when CDS archive fails to load
00decca: 8289208: Test DrawRotatedStringUsingRotatedFont.java occasionally crashes on MacOS
... and 98 more: https://git.openjdk.org/jdk/compare/aa557b9b01571d7614f54d341c5cd8bf36cdacee...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-08-15T09:54:07Z

@fisk @xmas92 Pushed as commit fd4b2f2.

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

8291718: Remove mark_for_deoptimization from klass unloading

6f86211

openjdk bot added the rfr Pull request is ready for review label Aug 2, 2022

openjdk bot added the hotspot hotspot-dev@openjdk.org label Aug 2, 2022

fisk approved these changes Aug 2, 2022

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Aug 2, 2022

dean-long reviewed Aug 2, 2022

View reviewed changes

xmas92 added 5 commits August 3, 2022 14:27

Simplify and assert in remove_all_dependents

0141b28

Fix indentation

d94258d

Revert "Simplify and assert in remove_all_dependents"

7349d24

This reverts commit 0141b28.

Fix indentation

048f2f7

Remove double counting _perf_total_buckets_deallocated_count

f8eaa68

Extend CodeCache::UnloadingScope to include klass unloading

019d4d1

xmas92 added 3 commits August 8, 2022 17:39

Merge remote-tracking branch 'upstream/master' into JDK-8291718

1dba0c9

Fix for shenandoah

25f3390

Remove indentation

f6dcef0

fisk approved these changes Aug 9, 2022

View reviewed changes

dean-long approved these changes Aug 12, 2022

View reviewed changes

openjdk bot added the sponsor Pull request is ready to be sponsored label Aug 15, 2022

openjdk bot added the integrated Pull request has been integrated label Aug 15, 2022

openjdk bot closed this Aug 15, 2022

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Aug 15, 2022

xmas92 deleted the JDK-8291718 branch January 5, 2026 06:12

8291718: Remove mark_for_deoptimization from klass unloading #9713

8291718: Remove mark_for_deoptimization from klass unloading #9713

Uh oh!

Conversation

xmas92 commented Aug 2, 2022 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Aug 2, 2022

Uh oh!

openjdk bot commented Aug 2, 2022

Uh oh!

mlbridge bot commented Aug 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

fisk left a comment

Choose a reason for hiding this comment

Uh oh!

openjdk bot commented Aug 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xmas92 commented Aug 4, 2022

Uh oh!

xmas92 commented Aug 8, 2022

Uh oh!

xmas92 commented Aug 9, 2022

Uh oh!

xmas92 commented Aug 15, 2022

Uh oh!

openjdk bot commented Aug 15, 2022

Uh oh!

fisk commented Aug 15, 2022

Uh oh!

openjdk bot commented Aug 15, 2022

Uh oh!

openjdk bot commented Aug 15, 2022

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

xmas92 commented Aug 2, 2022 •

edited by openjdk bot

Loading

mlbridge bot commented Aug 2, 2022 •

edited

Loading

openjdk bot commented Aug 2, 2022 •

edited

Loading