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

[Albert Mingkun Yang]

On Thu, 22 Aug 2024 11:50:24 GMT, Guoxiong Li <gli at openjdk.org> wrote:

>> Albert Mingkun Yang has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains three additional commits since the last revision:
>> 
>>  - review
>>  - Merge branch 'master' into pgc-split-region
>>  - pgc-split-region
>
> 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.

> src/hotspot/share/gc/parallel/psParallelCompact.cpp line 1560:
> 
>> 1558:                                       HeapWord* end,
>> 1559:                                       HeapWord* destination,
>> 1560:                                       size_t live_words) {
> 
> We can pass the precise destination and then the last parameter `live_words` of the method `PSParallelCompact::forward_to_new_addr` can be removed. Just like:
> 
> 
> --- a/src/hotspot/share/gc/parallel/psParallelCompact.cpp
> +++ b/src/hotspot/share/gc/parallel/psParallelCompact.cpp
> @@ -1556,8 +1556,7 @@ void PSParallelCompact::forward_to_new_addr() {
>      static void forward_objs_in_range(ParCompactionManager* cm,
>                                        HeapWord* start,
>                                        HeapWord* end,
> -                                      HeapWord* destination,
> -                                      size_t live_words) {
> +                                      HeapWord* destination) {
>        HeapWord* cur_addr = start;
>  
>        while (cur_addr < end) {
> @@ -1566,14 +1565,13 @@ void PSParallelCompact::forward_to_new_addr() {
>            return;
>          }
>          assert(mark_bitmap()->is_marked(cur_addr), "inv");
> -        HeapWord* new_addr = destination + live_words;
>          oop obj = cast_to_oop(cur_addr);
> -        if (new_addr != cur_addr) {
> +        if (destination != cur_addr) {
>            cm->preserved_marks()->push_if_necessary(obj, obj->mark());
> -          obj->forward_to(cast_to_oop(new_addr));
> +          obj->forward_to(cast_to_oop(destination));
>          }
>          size_t obj_size = obj->size();
> -        live_words += obj_size;
> +        destination += obj_size;
>          cur_addr += obj_size;
>        }
>      }
> @@ -1613,14 +1611,14 @@ void PSParallelCompact::forward_to_new_addr() {
>              // Part 1: will be relocated to space-1
>              HeapWord* split_destination = split_info.split_destination();
>              HeapWord* split_point = split_info.split_point();
> -            forward_objs_in_range(cm, region_start + live_words, split_point, split_destination, live_words);
> +            forward_objs_in_range(cm, region_start + live_words, split_point, split_destination + live_words);
>  
>              // Part 2: will be relocated to space-2
>              HeapWord* destination = region_ptr->destination();
> -            forward_objs_in_range(cm, split_point, region_end, destination, 0);
> +            forward_objs_in_range(cm, split_point, region_end, destination);
>            } else {...

Thank you for the suggestion. I have made minor adjustment so that the arg stays immutable.

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

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