RFR: 8338440: Parallel: Improve fragmentation mitigation in Full GC [v4]

[Guoxiong Li]

On Fri, 23 Aug 2024 11:21:25 GMT, Albert Mingkun Yang <ayang at openjdk.org> wrote:

>>> 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...
>
> `gc/InfiniteList.java` (if running Parallel) fails with this patch on my box.

I know what I miss now. The `new_top_addr` is not only a temporary variable which only influences the summary phase. It actually points to the `SpaceInfo::_new_top` which influences the later phases. So it needs to be updated precisely. Sorry, my mistake.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/20590#discussion_r1729069597