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

8254980: ZGC: ZHeapIterator visits armed nmethods with -XX:-ClassUnloading #801

Closed

Conversation

stefank
Copy link
Member

@stefank stefank commented Oct 22, 2020

When class unloading is turned off (-XX:-ClassUnloading) we consider all nmethods in the code cache to be "concurrent roots":

void ZConcurrentRootsIterator::oops_do(ZRootsIteratorClosure* cl) {
...
  if (!ClassUnloading) {
    _code_cache.oops_do(cl);
  }
}

This means that they are concurrently fixed up. See ZMarkConcurrentRootsTask and ZMarkConcurrentRootsIteratorClosure. Java threads that want to engage with an nmethod, needs to go through an nmethod entry barrier in case the concurrent roots have not been processed yet. When the ZHeapIterator is used, we rely on the fact that the vm operation the heap iteration is running in, has done a pre-step to fix all thread stacks (including "entering" the relevant nmethods"). See code using:

// You may override skip_thread_oop_barriers to return true if the operation
// does not access thread-private oops (including frames).
virtual bool skip_thread_oop_barriers() const { return false; }

So, this works when we don't use the entire code cache as root. However, when running with -XX:-ClassUnloading, there's no guarantee that the _code_cache.oops_do(cl) call above has run when the heap iteration is executing. So, the heap iteration code could end up reading oops from an nmethod that has not been entered. Currently, this is benign, since we do apply appropriate load barriers to handle this. However, it would be better if we had a clearer model that only entered/disarmed nmethods are visited.

The proposed patch extends the previous should_disarm_nmethods() bool, into an enum stating what "enter" operation ZNMethodToOopsDoClosure should perform.

I think the names of the values quite self-explanatory:

  PreBarrier,
  VerifyDisarmed,
  None

The tricky one is Disarm, which is only used by the marking code. It's used to provide an optimized entry barrier for the marking code. However, since the marking code only provides an OopClosure, and not an NMethodClosuer, we have to put the disarming code here. I have some ideas on how to make all this simpler, but I first want some simplifications that it requires some other patches around our weak root handling to trickle in first.


Progress

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

Testing

Linux x32 Linux x64 Windows x64 macOS x64
Build ✔️ (1/1 passed) ✔️ (5/5 passed) ✔️ (2/2 passed) ✔️ (2/2 passed)
Test (tier1) ✔️ (9/9 passed) ✔️ (9/9 passed) ✔️ (9/9 passed)

Issue

  • JDK-8254980: ZGC: ZHeapIterator visits armed nmethods with -XX:-ClassUnloading

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/801/head:pull/801
$ git checkout pull/801

@bridgekeeper
Copy link

bridgekeeper bot commented Oct 22, 2020

👋 Welcome back stefank! 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 openjdk bot added the rfr Pull request is ready for review label Oct 22, 2020
@openjdk
Copy link

openjdk bot commented Oct 22, 2020

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

  • hotspot-gc

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.

@openjdk openjdk bot added the hotspot-gc hotspot-gc-dev@openjdk.org label Oct 22, 2020
@mlbridge
Copy link

mlbridge bot commented Oct 22, 2020

Webrevs

Copy link
Contributor

@fisk fisk left a comment

Choose a reason for hiding this comment

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

Looks good in general. Found 2 VerifyDisarmed that should probably be None instead as I don't think they ever touch an nmethod. If you agree, then I am good with the rest.

virtual void do_oop(narrowOop* p) override;

virtual ZNMethodEntry nmethod_entry() const override {
return ZNMethodEntry::VerifyDisarmed;
Copy link
Contributor

Choose a reason for hiding this comment

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

I can't see that this will trigger any verification. The ZPhantomKeepAliveOopClosure is used in ZProcessWeakRootsTask, and it does not visit nmethods. Even if it did, it could still not assert that the visited nmethods are not disarmed, because it could have a single oop that is live, despite the nmethod not being entered.

virtual void do_oop(narrowOop* p) override;

virtual ZNMethodEntry nmethod_entry() const override {
return ZNMethodEntry::VerifyDisarmed;
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here, except the ZPhantomCleanOopClosure is used from ZProcessConcurrentWeakRootsTask which also does not visit any nmethods. Similar argument that if it did, it still would not make sense to verify that nmethods have been disarmed.

@stefank
Copy link
Member Author

stefank commented Oct 22, 2020

Yes, I agree. I revisited my prototype to completely get rid of should_disarm_nmethods/nmethod_entry(), and I came to the same conclusion when updating the code.

@@ -582,8 +582,9 @@ class ZMarkConcurrentRootsIteratorClosure : public ZRootsIteratorClosure {
ZThreadLocalAllocBuffer::publish_statistics();
}

virtual bool should_disarm_nmethods() const {
return true;
virtual ZNMethodEntry nmethod_entry() const override {
Copy link
Contributor

Choose a reason for hiding this comment

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

According to the stye-guide we shouldn't use override yet. It's used in a few more places in this patch.

Copy link
Member Author

Choose a reason for hiding this comment

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

Will remove.

Copy link
Member

Choose a reason for hiding this comment

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

I wonder if there's a ticket for revising the style guide to include override, which does make the overriding more explicit.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

I see; thank you.

if (_entry == ZNMethodEntry::VerifyDisarmed) {
// Only verify
assert(!ZNMethod::is_armed(nm), "Must be disarmed");

Copy link
Contributor

Choose a reason for hiding this comment

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

Extra white space can be removed.

Copy link
Member Author

Choose a reason for hiding this comment

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

If you insist. I think this style makes the code easier to read.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes please, to me it looks unintended, especially since we don't use that style elsewhere. Maybe flipping the if (VerifyDisarmed) and else if(Disarm) would make it read better?

if (_entry == ZNMethodEntry::VerifyDisarmed) {
// Only verify
assert(!ZNMethod::is_armed(nm), "Must be disarmed");

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes please, to me it looks unintended, especially since we don't use that style elsewhere. Maybe flipping the if (VerifyDisarmed) and else if(Disarm) would make it read better?

virtual ZNMethodEntry nmethod_entry() const override {
return ZNMethodEntry::VerifyDisarmed;
virtual ZNMethodEntry nmethod_entry() const {
return ZNMethodEntry::None;
Copy link
Contributor

Choose a reason for hiding this comment

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

How about letting ZRootsIteratorClosure::nmethod_entry() be

ShouldNotReachHere();
return ZNMethodEntry::None;

instead of pure virtual, so that we can remove these unused overrides and catch errors in case a closure doesn't override this but we then apply it on nmethods? Alternatively add a ShouldNotReachHere() in these unused overrides?

@stefank
Copy link
Member Author

stefank commented Oct 27, 2020

Found that during the pre-review of this the code was rearranged and called ZNMethod::nmethod_oops_do twice, when run from the marking code. I've restructured to code to fix that, which at the same time removes stylistic 'else if (' newline.

pliden
pliden approved these changes Oct 27, 2020
Copy link
Contributor

@pliden pliden left a comment

Choose a reason for hiding this comment

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

Looks good!

@openjdk
Copy link

openjdk bot commented Oct 27, 2020

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

8254980: ZGC: ZHeapIterator visits armed nmethods with -XX:-ClassUnloading

Reviewed-by: eosterlund, pliden

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

  • 7679650: 8231231: The printing result is different from the case instruction
  • f7c59c6: 8255231: Avoid upcalls when initializing the statSampler

Please see this link for an up-to-date comparison between the source branch of this pull request and 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 Pull request is ready to be integrated label Oct 27, 2020
fisk
fisk approved these changes Oct 27, 2020
Copy link
Contributor

@fisk fisk left a comment

Choose a reason for hiding this comment

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

Looks good.

@stefank
Copy link
Member Author

stefank commented Oct 27, 2020

Thanks! I'll now ...

/integrate

@openjdk openjdk bot closed this Oct 27, 2020
@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 Oct 27, 2020
@openjdk
Copy link

openjdk bot commented Oct 27, 2020

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

  • 18d9905: 8255342: Remove non-specified JVM checks on Classes with Record attributes
  • 7679650: 8231231: The printing result is different from the case instruction
  • f7c59c6: 8255231: Avoid upcalls when initializing the statSampler

Your commit was automatically rebased without conflicts.

Pushed as commit cf56c7e.

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

@stefank stefank deleted the 8254980_zheapiterator_classunloading branch Oct 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot-gc hotspot-gc-dev@openjdk.org integrated Pull request has been integrated
4 participants