Skip to content

8327057: Parallel: Refactor ParMarkBitMap::iterate #18065

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.

Sign up for GitHub

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

Closed
albertnetymk wants to merge 3 commits into from
Closed

8327057: Parallel: Refactor ParMarkBitMap::iterate #18065

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fd271ce
pgc-iterate-incomplete
albertnetymk Feb 26, 2024
84d9af5
review
albertnetymk Mar 1, 2024
9968335
Merge branch 'master' into pgc-iterate-incomplete
albertnetymk Mar 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 21 additions & 26 deletions src/hotspot/share/gc/parallel/parMarkBitMap.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,27 +173,26 @@ ParMarkBitMap::iterate(ParMarkBitMapClosure* live_closure,
// The bitmap routines require the right boundary to be word-aligned.
const idx_t search_end = align_range_end(range_end);

idx_t cur_beg = find_obj_beg(range_beg, search_end);
while (cur_beg < range_end) {
const idx_t cur_end = find_obj_end(cur_beg, search_end);
if (cur_end >= range_end) {
// The obj ends outside the range.
live_closure->set_source(bit_to_addr(cur_beg));
Copy link
Contributor

@tschatzl tschatzl Mar 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The caller of iterate expects live_closure->source() to be set at exit but the new code does not update it. It is in use in case of abnormal status.

This is not the case for the iterate taking two closures afaict, but nevertheless it seems part of the contract so it is surprising if some methods do and some do not.

The other changes seem to be okay.

Copy link
Contributor

@tschatzl tschatzl Mar 1, 2024

Choose a reason for hiding this comment

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

@albertnetymk mentioned that do_addr of MarkAndUpdateClosure properly updates the source of the closure.

return incomplete;
idx_t cur_beg = range_beg;
while (true) {
cur_beg = find_obj_beg(cur_beg, search_end);
if (cur_beg >= range_end) {
break;
}

const size_t size = obj_size(cur_beg, cur_end);
const size_t size = obj_size(cur_beg);
IterationStatus status = live_closure->do_addr(bit_to_addr(cur_beg), size);
if (status != incomplete) {
assert(status == would_overflow || status == full, "sanity");
return status;
}

// Successfully processed the object; look for the next object.
cur_beg = find_obj_beg(cur_end + 1, search_end);
cur_beg += words_to_bits(size);
if (cur_beg >= range_end) {
break;
}
}

live_closure->set_source(bit_to_addr(range_end));
return complete;
}

Expand All @@ -210,45 +209,41 @@ ParMarkBitMap::iterate(ParMarkBitMapClosure* live_closure,
assert(range_end <= dead_range_end, "dead range invalid");

// The bitmap routines require the right boundary to be word-aligned.
const idx_t live_search_end = align_range_end(range_end);
const idx_t dead_search_end = align_range_end(dead_range_end);

idx_t cur_beg = range_beg;
if (range_beg < range_end && is_unmarked(range_beg)) {
// The range starts with dead space. Look for the next object, then fill.
// This must be the beginning of old/eden/from/to-space, so it's must be
// large enough for a filler.
cur_beg = find_obj_beg(range_beg + 1, dead_search_end);
const idx_t dead_space_end = MIN2(cur_beg - 1, dead_range_end - 1);
const idx_t dead_space_end = cur_beg - 1;
const size_t size = obj_size(range_beg, dead_space_end);
dead_closure->do_addr(bit_to_addr(range_beg), size);
}

while (cur_beg < range_end) {
const idx_t cur_end = find_obj_end(cur_beg, live_search_end);
if (cur_end >= range_end) {
// The obj ends outside the range.
live_closure->set_source(bit_to_addr(cur_beg));
return incomplete;
}

const size_t size = obj_size(cur_beg, cur_end);
const size_t size = obj_size(cur_beg);
IterationStatus status = live_closure->do_addr(bit_to_addr(cur_beg), size);
if (status != incomplete) {
assert(status == would_overflow || status == full, "sanity");
return status;
}

const idx_t dead_space_beg = cur_beg + words_to_bits(size);
if (dead_space_beg >= dead_search_end) {
break;
}
// Look for the start of the next object.
const idx_t dead_space_beg = cur_end + 1;
cur_beg = find_obj_beg(dead_space_beg, dead_search_end);
if (cur_beg > dead_space_beg) {
// Found dead space; compute the size and invoke the dead closure.
const idx_t dead_space_end = MIN2(cur_beg - 1, dead_range_end - 1);
const size_t size = obj_size(dead_space_beg, dead_space_end);
dead_closure->do_addr(bit_to_addr(dead_space_beg), size);
const idx_t dead_space_end = cur_beg - 1;
dead_closure->do_addr(bit_to_addr(dead_space_beg),
obj_size(dead_space_beg, dead_space_end));
}
}

live_closure->set_source(bit_to_addr(range_end));
return complete;
}

Expand Down
4 changes: 0 additions & 4 deletions src/hotspot/share/gc/parallel/parMarkBitMap.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,10 +81,6 @@ class ParMarkBitMap: public CHeapObj<mtGC>
// range [live_range_beg, live_range_end). This is used to iterate over the
// compacted region of the heap. Return values:
//
// incomplete The iteration is not complete. The last object that
// begins in the range does not end in the range;
// closure->source() is set to the start of that object.
//
// complete The iteration is complete. All objects in the range
// were processed and the closure is not full;
// closure->source() is set one past the end of the range.
Expand Down
26 changes: 1 addition & 25 deletions src/hotspot/share/gc/parallel/psParallelCompact.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2597,12 +2597,7 @@ PSParallelCompact::update_and_deadwood_in_dense_prefix(ParCompactionManager* cm,
// Create closures and iterate.
UpdateOnlyClosure update_closure(mbm, cm, space_id);
FillClosure fill_closure(cm, space_id);
ParMarkBitMap::IterationStatus status;
status = mbm->iterate(&update_closure, &fill_closure, beg_addr, end_addr,
dense_prefix_end);
if (status == ParMarkBitMap::incomplete) {
update_closure.do_addr(update_closure.source());
}
mbm->iterate(&update_closure, &fill_closure, beg_addr, end_addr, dense_prefix_end);
}

// Mark the regions as filled.
Expand Down Expand Up @@ -2913,25 +2908,6 @@ void PSParallelCompact::fill_region(ParCompactionManager* cm, MoveAndUpdateClosu
src_space_top);
IterationStatus status = bitmap->iterate(&closure, cur_addr, end_addr);

if (status == ParMarkBitMap::incomplete) {
// The last obj that starts in the source region does not end in the
// region.
assert(closure.source() < end_addr, "sanity");
HeapWord* const obj_beg = closure.source();
HeapWord* const range_end = MIN2(obj_beg + closure.words_remaining(),
src_space_top);
HeapWord* const obj_end = bitmap->find_obj_end(obj_beg, range_end);
if (obj_end < range_end) {
// The end was found; the entire object will fit.
status = closure.do_addr(obj_beg, bitmap->obj_size(obj_beg, obj_end));
assert(status != ParMarkBitMap::would_overflow, "sanity");
} else {
// The end was not found; the object will not fit.
assert(range_end < src_space_top, "obj cannot cross space boundary");
status = ParMarkBitMap::would_overflow;
}
}

if (status == ParMarkBitMap::would_overflow) {
// The last object did not fit. Note that interior oop updates were
// deferred, then copy enough of the object to fill the region.
Expand Down