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

8276887: G1: Move precleaning to Concurrent Mark From Roots subphase #6327

Closed
wants to merge 6 commits into from

Conversation

albertnetymk
Copy link
Member

@albertnetymk albertnetymk commented Nov 10, 2021

Simple change of moving the "Preclean" subphase into "Mark from roots" subphase so that precleaning becomes parallel automatically. Evaluation using a contrived java program shows that multiple GC threads do precleaning. More detailed testing results are available in the JBS ticket.

Test: hotspot_gc


Progress

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

Issue

  • JDK-8276887: G1: Move precleaning to Concurrent Mark From Roots subphase

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 6327

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

Using diff file

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Nov 10, 2021

👋 Welcome back ayang! 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 Nov 10, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Nov 10, 2021

@albertnetymk 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 Nov 10, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Nov 10, 2021

Webrevs

Copy link
Contributor

@tschatzl tschatzl left a comment

Lgtm.

@openjdk
Copy link

@openjdk openjdk bot commented Nov 10, 2021

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

8276887: G1: Move precleaning to Concurrent Mark From Roots subphase

Reviewed-by: tschatzl, sjohanss, kbarrett

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

  • 8c5f030: 8276453: Undefined behavior in C1 LIR_OprDesc causes SEGV in fastdebug build
  • 176d21d: 8276824: refactor Thread::is_JavaThread_protected
  • 74f3e69: 8277071: [BACKOUT] JDK-8276743 Make openjdk build Zip Archive generation "reproducible"
  • b85500e: 8276123: ZipFile::getEntry will not return a file entry when there is a directory entry of the same name within a Zip File
  • 0d2980c: 8258192: Obsolete the CriticalJNINatives flag
  • 5a2452c: 8274835: Remove unnecessary castings in java.base
  • 3b2585c: 8276658: Clean up JNI local handles code
  • aeba653: 8276743: Make openjdk build Zip Archive generation "reproducible"
  • 51a5731: 8277016: Use blessed modifier order in jdk.httpserver
  • c4b4432: 8277012: Use blessed modifier order in src/utils
  • ... and 50 more: https://git.openjdk.java.net/jdk/compare/e35abe3235ab38985a19545e76c58260ec97c718...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 ready and removed ready labels Nov 10, 2021
@albertnetymk
Copy link
Member Author

@albertnetymk albertnetymk commented Nov 10, 2021

In the initial patch, the case ConcGCThreads=1 && ParallelGCThreads>1 is not properly handle. The latest commit fixes that by calling get_discovered_list multiple times when the discovery is single-threaded.

Copy link
Contributor

@kstefanj kstefanj left a comment

Nice improvement. I have one suggestion, but feel free to ignore it if you don't agree.

The new logging is on par with the old one, but I think it would be nice to make it more consistent with other reference processing logging. We currently have other logging that look like this for example:

[228,965s][debug][gc,phases,ref  ] GC(38)   SoftReference:
[228,965s][debug][gc,phases,ref  ] GC(38)     Discovered: 0
[228,965s][debug][gc,phases,ref  ] GC(38)     Cleared: 0
[228,965s][debug][gc,phases,ref  ] GC(38)   WeakReference:
[228,965s][debug][gc,phases,ref  ] GC(38)     Discovered: 0
[228,965s][debug][gc,phases,ref  ] GC(38)     Cleared: 0
[228,965s][debug][gc,phases,ref  ] GC(38)   FinalReference:
[228,965s][debug][gc,phases,ref  ] GC(38)     Discovered: 0
[228,965s][debug][gc,phases,ref  ] GC(38)     Cleared: 0
[228,965s][debug][gc,phases,ref  ] GC(38)   PhantomReference:
[228,965s][debug][gc,phases,ref  ] GC(38)     Discovered: 0
[228,965s][debug][gc,phases,ref  ] GC(38)     Cleared: 0

Maybe saying "Precleaned instead of "Cleared". Took a quick look at the ref-printing code and if you feel that this is a good idea but should be as a separate thing I'm good with that as well.

Copy link
Contributor

@kstefanj kstefanj left a comment

Approved, suggested improvements to logging will be addressed separately.

class G1CMConcurrentMarkingTask : public WorkerTask {
G1ConcurrentMark* _cm;
ReferenceProcessor* _cm_rp;

void do_mark_step(G1CMTask* task) {

Choose a reason for hiding this comment

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

I found the name of this confusingly close to do_marking_step by the task. I can see how this could be called a "step" with preclean being another (new) step, but I think the confusion with the other function outweighs that and needs a different name here. Maybe just do_marking or do_all_marking or something like that.

Copy link
Member Author

@albertnetymk albertnetymk Nov 13, 2021

Choose a reason for hiding this comment

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

Renamed to do_marking.

}
log_reflist("SoftRef after: ", _discoveredSoftRefs, _max_num_queues);
}
ReferenceType ref_type_arr[4] = { REF_SOFT, REF_WEAK, REF_FINAL, REF_PHANTOM };

Choose a reason for hiding this comment

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

Instead of specifying the array size, just let it be computed from the initializer length.

Copy link
Member Author

@albertnetymk albertnetymk Nov 13, 2021

Choose a reason for hiding this comment

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

Fixed.

log_reflist("SoftRef after: ", _discoveredSoftRefs, _max_num_queues);
}
ReferenceType ref_type_arr[4] = { REF_SOFT, REF_WEAK, REF_FINAL, REF_PHANTOM };
size_t ref_count_arr[4] = {};

Choose a reason for hiding this comment

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

Instead of literal 4 here and below, use ARRAY_SIZE(ref_type_arr), or maybe give that value a name and replace all the literal 4s with that name.

Copy link
Member Author

@albertnetymk albertnetymk Nov 13, 2021

Choose a reason for hiding this comment

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

Fixed.

log_reflist("WeakRef abort: ", _discoveredWeakRefs, _max_num_queues);
return;
}
if (discovery_is_mt()) {

Choose a reason for hiding this comment

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

The old form of these loops called yield->should_return() for each queue. That's been lost in this rewrite, and I'm not sure that's correct.

Copy link
Member Author

@albertnetymk albertnetymk Nov 13, 2021

Choose a reason for hiding this comment

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

It's still correct because preclean_discovered_reflist checkes for if-aborted while iterating the list. I could have written sth like the following if you think prompt abort is important.

  bool is_aborted = preclean_discovered_reflist(...);
  if (is_aborted) {
    return;
  }

Choose a reason for hiding this comment

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

It's hard to know for sure, but the description of YieldClosure makes me think that's not really the intended usage. OTOH, this seems to be the only use of YieldClosure; maybe there were others in CMS? I would prefer the yield->should_return() checks be retained. Also, preclean_discovered_reflist might no longer need to return bool.

Copy link
Member Author

@albertnetymk albertnetymk Nov 14, 2021

Choose a reason for hiding this comment

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

Restored yield->should_return().

}
log_reflist("PhantomRef after: ", _discoveredPhantomRefs, _max_num_queues);
}
uint worker_id = WorkerThread::current()->id();

Choose a reason for hiding this comment

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

This logging is potentially far more verbose than previously, since there will now be a line for each concurrent marking thread, rather than one line for the previously single-threaded precleaning. We have mechanisms for collecting and reporting timing and work units for parallel activities that should be used here.

Copy link
Member Author

@albertnetymk albertnetymk Nov 13, 2021

Choose a reason for hiding this comment

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

The new logs are on trace level, just FYI. How about I address all logging related issues (from Thomas and Stefan as well) in a follow-up PR?

Choose a reason for hiding this comment

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

I know this is trace logging, but that doesn't make the verbosity or content good. Consider a machine with a large amount of concurrency; one might be interested in any of the totals/min/max/avg, and having to extract that manually is going to be annoying. I'm okay with it being a followup though.

Copy link
Member Author

@albertnetymk albertnetymk Nov 14, 2021

Choose a reason for hiding this comment

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

one might be interested in any of the totals/min/max/avg

Thomas suggested this as well. I will address this in a followup PR.


constexpr int ref_kinds = 4;
ReferenceType ref_type_arr[] = { REF_SOFT, REF_WEAK, REF_FINAL, REF_PHANTOM };
static_assert(ARRAY_SIZE(ref_type_arr) == ref_kinds, "invariant");

Choose a reason for hiding this comment

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

I don't think there's a need for the literal 4 initializer for ref_kinds and the static assert to verify it's correct. Why not just use this after the declaration of ref_type_arr?
constexpr int ref_kinds = ARRAY_SIZE(ref_type_arr);

Copy link
Member Author

@albertnetymk albertnetymk Nov 14, 2021

Choose a reason for hiding this comment

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

Changed as suggested.

log_reflist("WeakRef abort: ", _discoveredWeakRefs, _max_num_queues);
return;
}
if (discovery_is_mt()) {

Choose a reason for hiding this comment

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

It's hard to know for sure, but the description of YieldClosure makes me think that's not really the intended usage. OTOH, this seems to be the only use of YieldClosure; maybe there were others in CMS? I would prefer the yield->should_return() checks be retained. Also, preclean_discovered_reflist might no longer need to return bool.

}
log_reflist("PhantomRef after: ", _discoveredPhantomRefs, _max_num_queues);
}
uint worker_id = WorkerThread::current()->id();

Choose a reason for hiding this comment

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

I know this is trace logging, but that doesn't make the verbosity or content good. Consider a machine with a large amount of concurrency; one might be interested in any of the totals/min/max/avg, and having to extract that manually is going to be annoying. I'm okay with it being a followup though.

Copy link

@kimbarrett kimbarrett left a comment

Looks good.

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Dec 13, 2021

@albertnetymk This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@albertnetymk
Copy link
Member Author

@albertnetymk albertnetymk commented Jan 7, 2022

In the scenario where discovery is sequential && parallel ref-processing is disabled (e.g. ParallelRefProcEnabled == false) && ParallelGCThreads > 1, all iterations except the first one in for (uint j = 0; j < _num_queues; ++j) { are no-op. Will be fixed by
https://bugs.openjdk.java.net/browse/JDK-8279456

Therefore, I will keep this PR open until then.

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Feb 4, 2022

@albertnetymk This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Mar 5, 2022

@albertnetymk This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.

@bridgekeeper bridgekeeper bot closed this Mar 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot-gc ready rfr
4 participants