8299074: nmethod marked for deoptimization is not deoptimized #12012

TobiHartmann · 2023-01-16T15:51:28Z

In the failing UnexpectedDeoptimizationAllTest there is a race condition between one thread repeatedly calling WB_DeoptimizeAll() and the main thread checking nmethod dependencies on class loading and also attempting marking/deoptimization of nmethods due to dependency violations. The problem is that already (concurrently) marked nmethods are not counted and if no new nmethods are marked, we then wrongly assume that there is no need to call deoptimize_all_marked:

jdk/src/hotspot/share/code/codeCache.cpp

Lines 1438 to 1440 in 0800813

    
           if (marked > 0) { 
        
             // At least one nmethod has been marked for deoptimization 
        
             Deoptimization::deoptimize_all_marked();

Details:

Thread 1: Method useNativeAccessor(Executable member) is compiled under the assumption that its argument type java.lang.reflect.Executable has only one implementer java.lang.reflect.Method. A corresponding dependency is registered in the nmethod and a virtual call to member.getParameterCount() is inlined to java.lang.reflect.Method::getParameterCount().

Thread 2: Calls Whitebox API method WB_DeoptimizeAll -> CodeCache::mark_all_nmethods_for_deoptimization() that marks useNativeAccessor for deoptimization.

Thread 1: Triggers class loading of java.lang.reflect.Constructor and CodeCache::flush_dependents_on -> CodeCache::mark_for_deoptimization -> ... -> DependencyContext::mark_dependent_nmethods detects that useNativeAccessor needs to be deoptimized now that java.lang.reflect.Executable has more than one implementer. However, the nmethod is already marked for deoptimization (most nmethods are) and therefore ignored. The marked counter is 0 and therefore Deoptimization::deoptimize_all_marked() is not executed either. The thread continues execution and ends up crashing because a java.lang.reflect.Constructor object is passed to compiled useNativeAccessor which can not handle it (we call the wrong getParameterCount on it).

Thread 2: Is still in WB_DeoptimizeAll, marking nmethods for deoptimization but didn't get a chance to call Deoptimization::deoptimize_all_marked() yet.

Before JDK-8221734 in JDK 13, WB_DeoptimizeAll acquired the Compile_lock but it got removed. Instead of re-adding the lock, I modified the code to also count nmethods that were already marked for deoptimization, as suggested by @robehn in the bug comments.

Thanks,
Tobias

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-8299074: nmethod marked for deoptimization is not deoptimized

Reviewers

Erik Österlund (@fisk - Reviewer) ⚠️ Review applies to 5ac3fdde
Robbin Ehn (@robehn - Reviewer)
Vladimir Kozlov (@vnkozlov - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 12012

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

Using diff file

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

bridgekeeper · 2023-01-16T15:52:40Z

👋 Welcome back thartmann! 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 · 2023-01-16T15:54:38Z

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

hotspot-compiler

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 · 2023-01-16T15:57:51Z

Webrevs

01: Full - Incremental (98bec705)
00: Full (5ac3fdde)

fisk

Looks good.

openjdk · 2023-01-16T16:28:46Z

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

8299074: nmethod marked for deoptimization is not deoptimized

Reviewed-by: eosterlund, rehn, kvn

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

8c12ae8: 8283203: Fix typo in SystemTray.getTrayIconSize javadoc
e7e3712: 8300010: UnsatisfiedLinkError on calling System.console().readPassword() on Windows
0b9ff06: 8300184: Optimize ResourceHashtableBase::iterate_all using _number_of_entries
75b122f: 8300120: Configure should support different defaults for CI/dev build environments
e139ec3: 8300069: Left shift of negative value in share/adlc/dict2.cpp
fb147aa: 8300228: ModuleReader.find on exploded module throws if resource name maps to invalid file path
4cd166f: 8300125: Serial: Remove unused Generation::reset_saved_marks
85d70ac: Merge
e15bdc5: 8299789: Compilation of gtest causes build to fail if runtime libraries are in different dirs
b7fb8ef: 8299323: Allow extended registers for cmpw
... and 24 more: https://git.openjdk.org/jdk/compare/83f2c9a2b290f11fbfb118a22c9667f26ac7c516...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.

vnkozlov · 2023-01-16T23:27:00Z

compiler/jsr292/ContinuousCallSiteTargetChange.java in GHA testing failed with:

#  Internal Error (/jdk/src/hotspot/share/code/dependencies.cpp:2054), pid=51800, tid=51802
#  assert(method_handle != changes->method_handle()) failed: must be

I don't see such failure in other recent PRs.

TobiHartmann · 2023-01-17T09:00:38Z

Thanks for the reviews!

I hit this assert in my testing now as well, will investigate.

robehn

Looks good! (- assertion :) )

TobiHartmann · 2023-01-17T15:23:10Z

Thanks for the review Robbin.

Regarding the failure with ContinuousCallSiteTargetChange.java: The problem is that two threads can update a method handle call site target via MHN_setCallSiteTargetNormal and trigger flushing of dependent nmethods. One thread might then succeed, mark/deopt the nmethod and update the call site target. If the other thread now processes marked nmethods again, it will observe that the call site target already changed and hit an assert. The solution is to not check dependencies on already marked nmethods but still count them. I updated the fix accordingly and added comments.

robehn · 2023-01-17T15:24:47Z

src/hotspot/share/code/dependencyContext.cpp

+          nm->print_dependencies();
+        }
+        changes.mark_for_deoptimization(nm);
+        found++;
      }


A minor suggestion is to move the found++ from the individual branches to here, so there is only one.
Looks good!

But that would increment found also in the cases where no method was marked for deoptimization, right?

Yes, sorry, please ignore :)

vnkozlov

Good.

TobiHartmann · 2023-01-17T18:17:47Z

Thanks again for the reviews, Vladimir and Robbin.

TobiHartmann · 2023-01-18T08:13:46Z

/integrate

openjdk · 2023-01-18T08:15:28Z

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

1f438a8: 8282651: ZGC: vmTestbase/gc/ArrayJuggle/ tests fails intermittently with exit code 97
f1194dc: 8300109: RISC-V: Improve code generation for MinI/MaxI nodes
89a032d: 8300002: Performance regression caused by non-inlined hot methods due to post call noop instructions
7071397: 8299224: TestReporterStreams.java has bad indentation for legal header
1d8b87d: 8300321: Use link tags in javax.sql.rowset package-info
f9883fc: 8300279: Use generalized see and link tags in core libs in client libs
00b6c55: 8300254: ASan build does not correctly propagate ASAN_OPTIONS
e37078f: 8282664: Unroll by hand StringUTF16 and StringLatin1 polynomial hash loops
ade08e1: 8300093: Refactor code examples to use @snippet in java.text.MessageFormat
d7c05d1: 8300011: Refactor code examples to use @snippet in java.util.TimeZone
... and 34 more: https://git.openjdk.org/jdk/compare/83f2c9a2b290f11fbfb118a22c9667f26ac7c516...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-01-18T08:15:46Z

@TobiHartmann Pushed as commit 66f7387.

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

8299074: nmethod marked for deoptimization is not deoptimized

5ac3fdd

openjdk bot added the rfr Pull request is ready for review label Jan 16, 2023

openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Jan 16, 2023

fisk approved these changes Jan 16, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jan 16, 2023

robehn approved these changes Jan 17, 2023

View reviewed changes

Do not check the dependency on already marked nmethods

98bec70

robehn approved these changes Jan 17, 2023

View reviewed changes

vnkozlov approved these changes Jan 17, 2023

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Jan 18, 2023

openjdk bot closed this Jan 18, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jan 18, 2023

	if (marked > 0) {
	// At least one nmethod has been marked for deoptimization
	Deoptimization::deoptimize_all_marked();

8299074: nmethod marked for deoptimization is not deoptimized #12012

8299074: nmethod marked for deoptimization is not deoptimized #12012

Uh oh!

Conversation

TobiHartmann commented Jan 16, 2023 • 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 Jan 16, 2023

Uh oh!

openjdk bot commented Jan 16, 2023

Uh oh!

mlbridge bot commented Jan 16, 2023 • 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 Jan 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vnkozlov commented Jan 16, 2023

Uh oh!

TobiHartmann commented Jan 17, 2023

Uh oh!

robehn left a comment

Choose a reason for hiding this comment

Uh oh!

TobiHartmann commented Jan 17, 2023

Uh oh!

robehn Jan 17, 2023

Choose a reason for hiding this comment

Uh oh!

TobiHartmann Jan 17, 2023

Choose a reason for hiding this comment

Uh oh!

robehn Jan 17, 2023

Choose a reason for hiding this comment

Uh oh!

vnkozlov left a comment

Choose a reason for hiding this comment

Uh oh!

TobiHartmann commented Jan 17, 2023

Uh oh!

TobiHartmann commented Jan 18, 2023

Uh oh!

openjdk bot commented Jan 18, 2023

Uh oh!

openjdk bot commented Jan 18, 2023

Uh oh!

Uh oh!

TobiHartmann commented Jan 16, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Jan 16, 2023 •

edited

Loading

openjdk bot commented Jan 16, 2023 •

edited

Loading