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

8270009: Factor out and shuffle methods in G1CollectedHeap::do_collection_pause_at_safepoint_helper #4744

Conversation

tschatzl
Copy link
Contributor

@tschatzl tschatzl commented Jul 9, 2021

Hi all,

can I get reviews for this change that factors out a few methods from do_collection_pause_at_safepoint_helper and reshuffles code from gc_prologue/gc_epilogue to be able to put those into parallel phase(s) later.

Testing: tier1-5

Thanks,
Thomas


Progress

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

Issue

  • JDK-8270009: Factor out and shuffle methods in G1CollectedHeap::do_collection_pause_at_safepoint_helper

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 4744

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

Using diff file

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Jul 9, 2021

👋 Welcome back tschatzl! 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 Jul 9, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jul 9, 2021

@tschatzl 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 Jul 9, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Jul 9, 2021

Webrevs

Copy link

@kimbarrett kimbarrett left a comment

A few minor nits, but generally looks good. Hopefully I didn't miss anything; this kind of code rearrangement can be hard to review. (Not that it shouldn't be done, but all the scrolling around and needing to dig through things made it hard to keep track.)

src/hotspot/share/gc/g1/g1CollectedHeap.cpp Outdated Show resolved Hide resolved
src/hotspot/share/gc/g1/g1CollectedHeap.cpp Show resolved Hide resolved
void G1CollectedHeap::do_collection_pause_at_safepoint_helper(double target_pause_time_ms) {
ResourceMark rm;

IsGCActiveMark active_gc_mark;

Choose a reason for hiding this comment

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

Moving the IsGCActiveMark here expands the scope of is_gc_active() being true to cover areas it didn't used to. I think this is okay, but I didn't check all callers carefully.

Copy link
Contributor Author

@tschatzl tschatzl Jul 12, 2021

Choose a reason for hiding this comment

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

None of the code before that (verification, timing setup, heap printing) should be dependent on that. Actually I was considering moving the mark right after the guarantee in G1CollectedHeap::do_collection_pause_at_safepoint, but then I reconsidered due to the GCLocker check there.

_bytes_used_during_gc = 0;

_expand_heap_after_alloc_failure = true;
Atomic::store(&_num_regions_failed_evacuation, 0u);

wait_for_root_region_scanning();

Choose a reason for hiding this comment

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

I kind of dislike having this wait moved here into this helper function, where I feel like it's less obvious in the flow of the collection pause. I'd rather it was still in the main body. And it's not like there's anything gained by waiting until after the other things that precede it in this function. It looks like it's okay to delay the wait later than its current location though; the scan is performed using the concurrent marking threads, so other parts of the setup along the way should be fine.

Copy link
Contributor Author

@tschatzl tschatzl Jul 12, 2021

Choose a reason for hiding this comment

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

I came to the same conclusion and considering that waiting as a candidate for putting it into the going-to-be parallel pre-evacuation phase. Then again, it might not be useful to have both the concurrent worker threads and parallel threads working at the same time (for performance reasons only; I do not consider the potential parallel verification also using the parallel worker threads as an issue). I checked that verification does not interfere with it (i.e. verification does not care about the mark bitmap in that sense).
The only requirement I can see is that all of these root-region objects need to have their referents marked, i.e. they must not be overwritten in some way, which this place guarantees as well, and to me it seems a more fitting place.

Choose a reason for hiding this comment

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

A problem with moving it later to the new placement is that it's now included in the pause time, where it previously wasn't. I'm not sure it should be included in the pause time, as it's not really part of this collection. I also wonder if the root scanning ought to be done using the concurrent workers rather than the stw workers, as there are typically fewer of the former (required to not be more?).

Copy link
Contributor Author

@tschatzl tschatzl Jul 12, 2021

Choose a reason for hiding this comment

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

It is true that it is now also included in the time used for MMU calculation (as part of the record_collection_start/end pair), which may a problem.

Previous gc timing also included it though, i.e. in the note_gc_start()/print_phase_times() pair.

I'll move it back then.

Choose a reason for hiding this comment

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

Things are unfortunately a bit messy and confusing/confused about pause time handling; see JDK-8240779 and JDK-7178365.

Copy link
Contributor Author

@tschatzl tschatzl Jul 12, 2021

Choose a reason for hiding this comment

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

I am aware of those, but this change does not really intend to fix these issues, but hopefully not make them worse (afaict this has been achieved).

Copy link
Member

@albertnetymk albertnetymk left a comment

Only minor comments regarding certain names.

src/hotspot/share/gc/g1/g1CollectedHeap.cpp Outdated Show resolved Hide resolved
src/hotspot/share/gc/g1/g1CollectedHeap.cpp Show resolved Hide resolved
Copy link

@kimbarrett kimbarrett left a comment

Looks good.

@openjdk
Copy link

@openjdk openjdk bot commented Jul 13, 2021

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

8270009: Factor out and shuffle methods in G1CollectedHeap::do_collection_pause_at_safepoint_helper

Reviewed-by: kbarrett, ayang

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

  • 07e9052: 8270056: Generated lambda class can not access protected static method of target class
  • afe957c: 8268698: Use Objects.check{Index,FromToIndex,FromIndexSize} for java.base
  • a4e5f08: 8267281: Call prepare_for_dynamic_dumping for jcmd dynamic_dump
  • 353e9c8: 8270320: JDK-8270110 committed invalid copyright headers
  • 7d2825e: 8270169: G1: Incorrect reference discovery MT degree in concurrent marking
  • 41a5eb4: 8270117: Broken jtreg link in "Building the JDK" page
  • 1aef372: 8266578: Disambiguate BigDecimal description of scale
  • 92ae6a5: 8244162: Additional opportunities to use NONCOPYABLE
  • 548bb31: 8270110: Shenandoah: Add test for JDK-8269661
  • c3a42ed: 8269878: Handle redundant reg-2-reg moves in X86 backend
  • ... and 20 more: https://git.openjdk.java.net/jdk/compare/c93204ce3d4653705e6aeeadb9b3c591e469de77...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 Jul 13, 2021
@tschatzl
Copy link
Contributor Author

@tschatzl tschatzl commented Jul 13, 2021

Thanks @albertnetymk @kimbarrett for your reviews.

/integrate

@openjdk
Copy link

@openjdk openjdk bot commented Jul 13, 2021

Going to push as commit 375fc2a.
Since your change was applied there have been 37 commits pushed to the master branch:

  • 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
  • afe957c: 8268698: Use Objects.check{Index,FromToIndex,FromIndexSize} for java.base
  • a4e5f08: 8267281: Call prepare_for_dynamic_dumping for jcmd dynamic_dump
  • ... and 27 more: https://git.openjdk.java.net/jdk/compare/c93204ce3d4653705e6aeeadb9b3c591e469de77...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

@tschatzl Pushed as commit 375fc2a.

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

@tschatzl tschatzl deleted the submit/8270009-shuffle-refactor-methods-in-do_collection_pause branch Aug 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot-gc integrated
3 participants