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

8269897: Shenandoah: Resolve UNKNOWN access strength, where possible #4697

Closed
wants to merge 6 commits into from

Conversation

rkennke
Copy link
Contributor

@rkennke rkennke commented Jul 6, 2021

We've observed test failures in jcstress, see:
https://bugs.openjdk.java.net/browse/JDK-8269897

We used to treat UNKNOWN reference accesses like weak accesses. UNKNOWN is used for Unsafe, reflection and JNI accesses, where it cannot be determined at compilation-time if we are accessing a regular field or a Reflection.referent field. The rationale for treating UNKNOWN as weak was that if the reference is a regular reference, then the value would be strongly reachable anyway, and only if it is a referent field would reachability matter. However, it turns out that this assumption is wrong: the test shows that a reference that is only weakly reachable can be legitimately written into a field, thus resurrecting the reference, and when that weakly reachable reference is loaded, it would be (wrongly) filtered as NULL.

A fix is to treat UNKNOWN accesses as strong. Accessing Reference.referent via reflection, JNI or Unsafe is Bad Idea anyway.
This test shows the problem with CAS, but I believe it affects all accesses via reflection, JNI, etc.

Testing:

  • the provided jcstress test
  • hotspot_gc_shenandoah
  • tier1

Progress

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

Issue

  • JDK-8269897: Shenandoah: Resolve UNKNOWN access strength, where possible

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 4697

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

Using diff file

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Jul 6, 2021

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

@rkennke
Copy link
Contributor Author

@rkennke rkennke commented Jul 6, 2021

Closing and retargeting to jdk17.

@rkennke rkennke closed this Jul 6, 2021
@openjdk openjdk bot added the rfr label Jul 6, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jul 6, 2021

@rkennke The following labels will be automatically applied to this pull request:

  • hotspot-gc
  • shenandoah

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-gc shenandoah labels Jul 6, 2021
@rkennke rkennke reopened this Jul 8, 2021
@rkennke rkennke changed the title 8269897: Shenandoah: Treat UNKNOWN refs access as strong 8269897: Shenandoah: Resolve UNKNOWN access strength, where possible Jul 8, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Jul 8, 2021

Webrevs

Copy link
Contributor

@shipilev shipilev left a comment

Looks okay to me, I am testing.

One nit: lets assert no UNKNOWN decorators are passed to AccessBarrier methods that do not do the UNKNOWN resolve?

@shipilev
Copy link
Contributor

@shipilev shipilev commented Jul 9, 2021

Current patch passes my testing, thanks. Still think we need to do asserts for extra safety.

@@ -125,7 +132,6 @@ class ShenandoahBarrierSet: public BarrierSet {
template <DecoratorSet decorators, typename BarrierSetT = ShenandoahBarrierSet>
class AccessBarrier: public BarrierSet::AccessBarrier<decorators, BarrierSetT> {
typedef BarrierSet::AccessBarrier<decorators, BarrierSetT> Raw;

Copy link
Contributor

@shipilev shipilev Jul 9, 2021

Choose a reason for hiding this comment

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

This change looks unnecessary.

}

template <DecoratorSet decorators, typename BarrierSetT>
inline oop ShenandoahBarrierSet::AccessBarrier<decorators, BarrierSetT>::oop_atomic_xchg_in_heap_at(oop base, ptrdiff_t offset, oop new_value) {
return oop_atomic_xchg_in_heap(AccessInternal::oop_field_addr<decorators>(base, offset), new_value);
assert((decorators & ON_STRONG_OOP_REF) != 0, "must be present");
Copy link
Contributor

@shipilev shipilev Jul 9, 2021

Choose a reason for hiding this comment

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

Shouldn't this be | ON_UNKNOWN_OOP_REF? We are doing resolve_possibly_unknown_oop_ref_strength later.

In fact, I am not so sure about asserting ON_STRONG_OOP_REF here and elsewhere as well. The future ON_WEAK_OOP_REF code should work fine here too? I think only asserting that ON_Uresolve_possibly_unknown_oop_ref_strengthNKNOWN_OOP_REF is not passed on bad paths is enough?

Copy link
Contributor

@shipilev shipilev left a comment

This still passes testing and looks fine to me.

@openjdk
Copy link

@openjdk openjdk bot commented Jul 12, 2021

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

8269897: Shenandoah: Resolve UNKNOWN access strength, where possible

Reviewed-by: shade

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

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 Jul 12, 2021
@rkennke
Copy link
Contributor Author

@rkennke rkennke commented Jul 13, 2021

/integrate

@openjdk
Copy link

@openjdk openjdk bot commented Jul 13, 2021

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

  • 460c4bb: 8270179: Rename Amalloc_4
  • 375fc2a: 8270009: Factor out and shuffle methods in G1CollectedHeap::do_collection_pause_at_safepoint_helper
  • 6b123b0: Merge
  • 4fc3180: 8266345: (fs) Custom DefaultFileSystemProvider security related loops
  • 999ced0: 8269873: serviceability/sa/Clhsdb tests are using a C2 specific VMStruct field
  • e1d3e73: 8268965: TCP Connection Reset when connecting simple socket to SSL server
  • 3d82b0e: 8269558: fix of JDK-8252657 missed to update history at the end of JVM TI spec
  • 2546006: 8270216: [macOS] Update named used for Java run loop mode
  • 565ec85: 8270282: Semantically rename reference processing subphases
  • 07e9052: 8270056: Generated lambda class can not access protected static method of target class
  • ... and 85 more: https://git.openjdk.java.net/jdk/compare/4dfcf53a8bf2ca8717e418b1cbd66ba263b77980...master

Your commit was automatically rebased without conflicts.

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

@openjdk openjdk bot commented Jul 13, 2021

@rkennke Pushed as commit 7ac0816.

💡 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
hotspot-gc integrated shenandoah
2 participants