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

8254562: ZGC: Remove ZMarkRootsTask #601

Closed
wants to merge 3 commits into from

Conversation

stefank
Copy link
Member

@stefank stefank commented Oct 12, 2020

After introducing concurrent stack scanning, we don't need to mark through any roots during the mark start pause. Remove the code.

Note that the old code passed in false to _roots(false /* visit_jvmti_weak_export */). This has the effect that no roots are visited by the iterator:

void ZRootsIterator::oops_do(ZRootsIteratorClosure* cl) {
  ZStatTimer timer(ZSubPhasePauseRoots);
  if (_visit_jvmti_weak_export) {
    _jvmti_weak_export.oops_do(cl);
  }
}

Progress

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

Testing

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

Issue

Reviewers

Download

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Oct 12, 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 label Oct 12, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Oct 12, 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 label Oct 12, 2020
@mlbridge
Copy link

@mlbridge mlbridge bot commented Oct 12, 2020

Webrevs

@stefank
Copy link
Member Author

@stefank stefank commented Oct 12, 2020

Looking at this some more I figured that we could clean up the rest of the ZRootIterator usages. The JVMTI cleaning isn't really a root, but rather a way for us to update the JVMTI datastructure after the objects have moved (update their identities).

So, the last patch:

  1. Removes the unnecessary spawning of parallel threads in ZRelocate::start. The JVMTI roots are only visited in a single-threaded operation.
  2. Removed/replaced ZRootsIterator. Neither ZVerify, nor ZHeapIterator, really used it, because they implicitly sent in false to the constructor (see above comment). ZRelocate now performs a direct calls, which makes it easier to understand, IMHO.

@pliden
Copy link
Contributor

@pliden pliden commented Oct 12, 2020

@stefank I see we're poking around in the same code :) I agree about the ZMarkTask/ZVerify/ZHeapIterator changes, but I don't think your change to ZRelocate will actually work, because without a ZTask the VM thread will not have a worker id, which will make the allocation path sad. To avoid stalling this patch, would you mind not doing the ZRelocate changes here, and we can continue discussning how we want to do this.

Copy link
Contributor

@fisk fisk left a comment

I agree with Per that we can start by removing the marking stuff first. I think Coleen is just about to move the JVMTI tag map oops into OopStorage, and then we can nuke the relocation code too.

@stefank
Copy link
Member Author

@stefank stefank commented Oct 12, 2020

Talked to Per. The plan is to revert the removal of the ZRelocateRootsTask, but at least run that code with only one worker thread. Running tests now.

// During relocation we need to visit the JVMTI
// export weak roots to rehash the JVMTI tag map
_roots.oops_do(&_cl);
ZRelocateRoots::oops_do(&_cl);
Copy link
Contributor

@pliden pliden Oct 12, 2020

Choose a reason for hiding this comment

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

I would have been more explicit here, something like this:

class ZRelocateJVMTIWeakExportTask : public ZTask {
public:
  ZRelocateJVMTIWeakExportTask() :
      ZTask("ZRelocateJVMTIWeakExportTask") {}

  virtual void work() {
    AlwaysTrueClosure always_alive;
    ZRelocateRootsIteratorClosure _cl;
    JvmtiExport::weak_oops_do(&always_alive, &cl);
  }
};

void ZRelocate::start() {
  // During relocation we need to visit the JVMTI
  // export weak roots to rehash the JVMTI tag map
  ZStatTimer timer(ZSubPhasePauseRelocateJVMTIWeakExport);
  ZRelocateJVMTIWeakExportTask task;
  _workers->run_serial(&task);
}

But this code will go away soon anyway, so you decide.

Copy link
Member Author

@stefank stefank Oct 13, 2020

Choose a reason for hiding this comment

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

I agree, and I actually started that way, but hit some minor snags that ZSubPhasePauseRelocateJVMTIWeakExport wasn't available to zRelocate.cpp, and I wasn't sure about moving a PhasePause instance out from the zRootsIterator.cpp.

pliden
pliden approved these changes Oct 12, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Oct 12, 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:

8254562: ZGC: Remove ZMarkRootsTask

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

  • c7f0064: 8253899: Make IsClassUnloadingEnabled signature match specification
  • aad3cf4: 8254234: Add test library stream object builder
  • 4184959: 8252374: Add a new factory method to concatenate a sequence of BodyPublisher instances into a single publisher.
  • 05459df: 8253765: C2: Control randomization in StressLCM and StressGCM
  • 6620b61: 8254573: Shenandoah: Streamline/inline native-LRB entry point
  • a6c23b7: 8253923: C2 doesn't always run loop opts for compilations that include loops
  • dfe8ba6: 8254320: Shenandoah: C2 native LRB should activate for non-cset objects
  • 295a44a: 8254558: Remove unimplemented Arguments::do_pd_flag_adjustments
  • 0fab73e: 8254560: Shenandoah: Concurrent Strong Roots logging is incorrect
  • 638f910: 8254559: Remove unimplemented JVMFlag::get_locked_message_ext
  • ... and 4 more: https://git.openjdk.java.net/jdk/compare/bf46acf9331ac3e5bc50631aa910d763ffb636f4...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 Oct 12, 2020
@stefank
Copy link
Member Author

@stefank stefank commented Oct 13, 2020

I'm going to push the patch as is. The relocate part will change soon anyway.

/integrate

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

@openjdk openjdk bot commented Oct 13, 2020

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

Your commit was automatically rebased without conflicts.

Pushed as commit 9d230ea.

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

@stefank stefank deleted the 8254562_remove_zmarkrootstask branch Oct 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot-gc integrated
3 participants