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
Closed
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
@@ -1070,20 +1070,25 @@ void ReferenceProcessor::preclean_discovered_references(BoolObjectClosure* is_al
GCTimer* gc_timer) {
Ticks preclean_start = Ticks::now();

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");
constexpr int ref_kinds = ARRAY_SIZE(ref_type_arr);
size_t ref_count_arr[ref_kinds] = {};

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().

for (int i = 0; i < ref_kinds; ++i) {
if (yield->should_return()) {
return; // aborted
}
ReferenceType ref_type = ref_type_arr[i];
DiscoveredList* list = get_discovered_list(ref_type);
ref_count_arr[i] = list->length();
preclean_discovered_reflist(*list, is_alive, enqueue, yield);
}
} else {
for (int i = 0; i < ref_kinds; ++i) {
if (yield->should_return()) {
return; // aborted
}
ReferenceType ref_type = ref_type_arr[i];
// When discovery is *not* multi-threaded, discovered refs are stored in
// list[0.._num_queues-1]. Loop _num_queues times to cover all lists.