RFR: 8338440: Parallel: Improve fragmentation mitigation in Full GC [v2]
Guoxiong Li
gli at openjdk.org
Fri Aug 23 11:00:04 UTC 2024
On Fri, 23 Aug 2024 09:11:46 GMT, Albert Mingkun Yang <ayang at openjdk.org> wrote:
>> src/hotspot/share/gc/parallel/psParallelCompact.cpp line 393:
>>
>>> 391: if (cur_addr >= region_end) {
>>> 392: break;
>>> 393: }
>>
>> What about using a `for` statement here? For example:
>>
>>
>> --- a/src/hotspot/share/gc/parallel/psParallelCompact.cpp
>> +++ b/src/hotspot/share/gc/parallel/psParallelCompact.cpp
>> @@ -382,15 +382,11 @@ HeapWord* ParallelCompactData::summarize_split_space(size_t src_region,
>> // Obj-iteration to locate the overflowing obj
>> HeapWord* region_start = region_to_addr(src_region);
>> HeapWord* region_end = region_start + RegionSize;
>> - HeapWord* cur_addr = region_start + partial_obj_size;
>> size_t live_words = partial_obj_size;
>>
>> - while (true) {
>> - assert(cur_addr < region_end, "inv");
>> - cur_addr = PSParallelCompact::mark_bitmap()->find_obj_beg(cur_addr, region_end);
>> - if (cur_addr >= region_end) {
>> - break;
>> - }
>> + for (HeapWord* cur_addr = region_start + partial_obj_size;
>> + cur_addr < region_end;
>> + cur_addr = PSParallelCompact::mark_bitmap()->find_obj_beg(cur_addr, region_end)) {
>>
>> oop obj = cast_to_oop(cur_addr);
>> size_t obj_size = obj->size();
>
> The sub-parts of `for` are rather complex, IMO, so I'd prefer the original style.
Just a suggestion. I am also OK with the current code.
>> src/hotspot/share/gc/parallel/psParallelCompact.cpp line 482:
>>
>>> 480: assert(source_next != nullptr, "source_next is null when splitting");
>>> 481: *source_next = summarize_split_space(cur_region, split_info, dest_addr,
>>> 482: target_end, target_next);
>>
>> Actually, the last parameter `target_next` of the method `summarize_split_space` won't be used at any other places. The bottom of the new space will be used instead (see `PSParallelCompact::summary_phase`). So I think the last parameter `target_next` of the method `summarize_split_space` can be **removed**. Then in `PSParallelCompact::summary_phase`, we can pass a null pointer in this situation (See code below) so that the meaning becomes clearer.
>>
>>
>> // method `PSParallelCompact::summary_phase`
>> } else if (live > 0) {
>> // Attempt to fit part of the source space into the target space.
>> HeapWord* next_src_addr = nullptr;
>> bool done = _summary_data.summarize(_space_info[id].split_info(),
>> space->bottom(), space->top(),
>> &next_src_addr,
>> *new_top_addr, dst_space_end,
>> new_top_addr); // <--- here, can pass `nullptr`
>> assert(!done, "space should not fit into old gen");
>> assert(next_src_addr != nullptr, "sanity");
>
> `target_next` is used in the callee to set up the new-top.
>
> In `summarize_split_space`:
>
>
> // Update new top of target space
> *target_next = new_top;
> Actually, the last parameter target_next of the method summarize_split_space won't be used at any other places.
> target_next is used in the callee to set up the new-top.
I means that the `target_next` is updated in `summarize_split_space` but the updated `target_next` is never used later. The bottom of the new space will be used instead.
// method PSParallelCompact::summary_phase
} else if (live > 0) {
// other code, skip
bool done = _summary_data.summarize(_space_info[id].split_info(),
space->bottom(), space->top(),
&next_src_addr,
*new_top_addr, dst_space_end,
new_top_addr); // <-- `new_top_addr` is updated, but not unnecessary
// other code, skip
new_top_addr = _space_info[id].new_top_addr(); // <-- `new_top_addr` is updated again
done = _summary_data.summarize(_space_info[id].split_info(),
next_src_addr, space->top(),
nullptr,
space->bottom(), dst_space_end, // <-- the bottom of the new space is used
new_top_addr);
// other code, skip
}
A draft diff is shown below:
--- a/src/hotspot/share/gc/parallel/psParallelCompact.cpp
+++ b/src/hotspot/share/gc/parallel/psParallelCompact.cpp
@@ -310,8 +310,7 @@ ParallelCompactData::summarize_dense_prefix(HeapWord* beg, HeapWord* end)
HeapWord* ParallelCompactData::summarize_split_space(size_t src_region,
SplitInfo& split_info,
HeapWord* const destination,
- HeapWord* const target_end,
- HeapWord** target_next) {
+ HeapWord* const target_end) {
assert(destination <= target_end, "sanity");
assert(destination + _region_data[src_region].data_size() > target_end,
"region should not fit into target space");
@@ -373,9 +372,6 @@ HeapWord* ParallelCompactData::summarize_split_space(size_t src_region,
}
}
- // Update new top of target space
- *target_next = new_top;
-
return overflowing_obj;
}
@@ -397,7 +393,6 @@ HeapWord* ParallelCompactData::summarize_split_space(size_t src_region,
if (destination + live_words + obj_size > target_end) {
// Found the overflowing obj
split_info.record(src_region, cur_addr, live_words);
- *target_next = destination + live_words;
return cur_addr;
}
@@ -478,8 +473,8 @@ bool ParallelCompactData::summarize(SplitInfo& split_info,
// target space and the rest is copied elsewhere.
if (dest_addr + words > target_end) {
assert(source_next != nullptr, "source_next is null when splitting");
- *source_next = summarize_split_space(cur_region, split_info, dest_addr,
- target_end, target_next);
+ assert(target_next == nullptr, "target_next is not null when splitting");
+ *source_next = summarize_split_space(cur_region, split_info, dest_addr, target_end);
return false;
}
@@ -931,7 +926,7 @@ void PSParallelCompact::summary_phase()
space->bottom(), space->top(),
&next_src_addr,
*new_top_addr, dst_space_end,
- new_top_addr);
+ nullptr);
assert(!done, "space should not fit into old gen");
assert(next_src_addr != nullptr, "sanity");
diff --git a/src/hotspot/share/gc/parallel/psParallelCompact.hpp b/src/hotspot/share/gc/parallel/psParallelCompact.hpp
index 00b9d8e7f8b..3b27bb37bf2 100644
--- a/src/hotspot/share/gc/parallel/psParallelCompact.hpp
+++ b/src/hotspot/share/gc/parallel/psParallelCompact.hpp
@@ -366,8 +366,7 @@ class ParallelCompactData
void summarize_dense_prefix(HeapWord* beg, HeapWord* end);
HeapWord* summarize_split_space(size_t src_region, SplitInfo& split_info,
- HeapWord* destination, HeapWord* target_end,
- HeapWord** target_next);
+ HeapWord* destination, HeapWord* target_end);
size_t live_words_in_space(const MutableSpace* space,
HeapWord** full_region_prefix_end = nullptr);
> if this assertion fails, it's unclear which field is problematic
Ohh...Got it.
>> src/hotspot/share/gc/parallel/psParallelCompact.cpp line 2271:
>>
>>> 2269: if (partial_obj_start == obj_start) {
>>> 2270: // This obj extends to next region.
>>> 2271: obj_end = partial_obj_end(next_region_start);
>>
>> Question: We know the object start position in this branch. Why can't we use object size (in new line 2271) directly (like new line 2274 shown below)? Why is it not safe?
>>
>>
>> // Completely contained in this region; safe to use size().
>> obj_end = obj_start + cast_to_oop(obj_start)->size();
>
> `size()` uses `klass`, which may lie in the next region (depending on the number of left words in this region), which can belong to another worker.
Thanks for your explanation.
> > But in _split_destination_count, the prefix split means two parts whose destinations locate in different regions but in the same space.
>
> Why are they in the "same" space? The purpose of having "split" to support space-boundary so that the first part and the second part are in two spaces.
The `_split_destination_count` is a size to record how many regions the preceding part occupy. The preceding part is moved to one same space and the second part is moved to another space. So now, it seems you are also misled by this ambiguous name, which is a proof that we need to rename it.
// file psParallelCompact.hpp
// Number of regions the preceding live words are relocated into.
uint split_destination_count() const { return _split_destination_count; }
// file psParallelCompact.cpp, method SplitInfo::record
// How many regions does the preceding part occupy
uint split_destination_count;
if (preceding_live_words == 0) {
split_destination_count = 0;
} else {
if (split_destination + preceding_live_words > sd.region_align_up(split_destination)) {
split_destination_count = 2;
} else {
split_destination_count = 1;
}
}
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/20590#discussion_r1728786276
PR Review Comment: https://git.openjdk.org/jdk/pull/20590#discussion_r1728786233
PR Review Comment: https://git.openjdk.org/jdk/pull/20590#discussion_r1728786527
PR Review Comment: https://git.openjdk.org/jdk/pull/20590#discussion_r1728786849
PR Review Comment: https://git.openjdk.org/jdk/pull/20590#discussion_r1728786723
More information about the hotspot-gc-dev
mailing list