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

8267947: CI: Preserve consistency between has_subklass() and is_subclass_of() #4269

Closed
wants to merge 3 commits into from

Conversation

@iwanowww
Copy link

@iwanowww iwanowww commented May 31, 2021

CI caches Klass::subklass() != NULL query, but concurrent class loading can
invalidate the cached value. Though recorded dependency won't let the nmethod
to be installed, the inconcistency can manifest as type paradoxes until
compilation is finished.

The fix caches only true value (since it can't change unless class unloading
takes place) and queries the VM otherwise.

Testing:

  • hs-tier1 - hs-tier6

Progress

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

Issue

  • JDK-8267947: CI: Preserve consistency between has_subklass() and is_subclass_of()

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 4269

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

Using diff file

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

@iwanowww
Copy link
Author

@iwanowww iwanowww commented May 31, 2021

/cc hotspot-compiler

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented May 31, 2021

👋 Welcome back vlivanov! 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
Copy link

@openjdk openjdk bot commented May 31, 2021

@iwanowww
The hotspot-compiler label was successfully added.

@iwanowww iwanowww marked this pull request as ready for review May 31, 2021
@openjdk openjdk bot added the rfr label May 31, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented May 31, 2021

Webrevs


return result;
#ifdef ASSERT
if (that->is_instance_klass() && !that->is_interface()) {
Copy link
Contributor

@rwestrel rwestrel Jun 1, 2021

Why the !that->is_interface() test?

Copy link
Author

@iwanowww iwanowww Jun 1, 2021

ciInstanceKlass::has_subklass() is always false for interfaces because they always have Klass::_subklass == NULL.

} else {
return compute_shared_has_subklass();
}
if (_has_subklass == subklass_true) {
Copy link
Member

@TobiHartmann TobiHartmann Jun 1, 2021

A comment explaining why we only cache subklass_true would be good. Also, do we still need a tri-state including subklass_unknown? It's now only used for initialization but not queried anywhere.

Copy link
Author

@iwanowww iwanowww Jun 1, 2021

Comment added.

Regarding subclass_false and subklass_unknown, keeping tri-state enum looks benign. Also, it makes sense to keep subklass_unknown to be able to distinguish between not yet initialized and already initialized cases (at least, during debugging).

Copy link
Member

@TobiHartmann TobiHartmann Jun 1, 2021

Okay, looks good.

@openjdk
Copy link

@openjdk openjdk bot commented Jun 1, 2021

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

8267947: CI: Preserve consistency between has_subklass() and is_subclass_of()

Reviewed-by: thartmann, roland

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

  • 229a6e2: 8267095: Miscellaneous cleanups in vm.runtime.defmeth tests
  • 6149b9a: 8267914: Remove DeferredObjectToKlass workaround
  • 4eb2168: 8267670: Update java.io, java.math, and java.text to use switch expressions
  • f5634fe: 8267979: C2: Fix verification code in SubTypeCheckNode::Ideal()
  • ae2f37f: 8267616: AArch64: Fix AES assertion messages in stubGenerator_aarch64.cpp
  • c06db45: 8267921: Remove redundant loop from sun.reflect.misc.ReflectUtil.privateCheckPackageAccess()
  • 382e7ec: 8246351: elements in headings are of incorrect size
  • 5df25dc088cfc3069e451b48c4f013d1d0491aa2: 8266807: Windows os_windows-gtest broken for UseLargePages
  • ce44cd6881bcbef81a840d7961a951ba586c0eae: 8267845: Add @requires to avoid running G1 large pages test with wrong page size
  • 4ade125c8a53e0bdc105e5f65e8c1d7aa13db950: 8267934: remove dead code in CLD
  • ... and 6 more: https://git.openjdk.java.net/jdk/compare/1e29005a22c7951242cf3b0d8cf2e6adc0b7b315...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 label Jun 1, 2021
Copy link
Contributor

@rwestrel rwestrel left a comment

Looks good to me.

@iwanowww
Copy link
Author

@iwanowww iwanowww commented Jun 1, 2021

Thanks for the reviews, Tobias and Roland.

/integrate

@openjdk openjdk bot closed this Jun 1, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Jun 1, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jun 1, 2021

@iwanowww Since your change was applied there have been 16 commits pushed to the master branch:

  • 229a6e2: 8267095: Miscellaneous cleanups in vm.runtime.defmeth tests
  • 6149b9a: 8267914: Remove DeferredObjectToKlass workaround
  • 4eb2168: 8267670: Update java.io, java.math, and java.text to use switch expressions
  • f5634fe: 8267979: C2: Fix verification code in SubTypeCheckNode::Ideal()
  • ae2f37f: 8267616: AArch64: Fix AES assertion messages in stubGenerator_aarch64.cpp
  • c06db45: 8267921: Remove redundant loop from sun.reflect.misc.ReflectUtil.privateCheckPackageAccess()
  • 382e7ec: 8246351: elements in headings are of incorrect size
  • 5df25dc088cfc3069e451b48c4f013d1d0491aa2: 8266807: Windows os_windows-gtest broken for UseLargePages
  • ce44cd6881bcbef81a840d7961a951ba586c0eae: 8267845: Add @requires to avoid running G1 large pages test with wrong page size
  • 4ade125c8a53e0bdc105e5f65e8c1d7aa13db950: 8267934: remove dead code in CLD
  • ... and 6 more: https://git.openjdk.java.net/jdk/compare/1e29005a22c7951242cf3b0d8cf2e6adc0b7b315...master

Your commit was automatically rebased without conflicts.

Pushed as commit ffd28c4a86aa8e8e59afb13abd4aeeea66557f66.

:bulb: 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
3 participants