RFR: 8324649: Shenandoah: replace implementation of free set [v57]

[Roman Kennke]

On Tue, 7 May 2024 14:35:33 GMT, Kelvin Nilsen <kdnilsen at openjdk.org> wrote:

>> Several objectives:
>> 1. Reduce humongous allocation failures by segregating regular regions from humongous regions
>> 2. Do not retire regions just because an allocation failed within the region if the memory remaining within the region is large enough to represent a LAB
>> 3. Track range of empty regions in addition to range of available regions in order to expedite humongous allocations
>> 4. Treat collector reserves as available for Mutator allocations after evacuation completes
>> 5. Improve encapsulation so as to enable an OldCollector reserve for future integration of generational Shenandoah
>> 
>> We have compared performance of existing FreeSet implementation with the proposed PR over a broad set of performance workloads and see that the impact is mostly neutral.
>> 
>> Comparing 105235.0 metrics from control, 220638.0 from experiment.
>> Compare: 0.589s
>>                           Most impacted benchmarks |                              Most impacted metrics
>> -------------------------------------------------------------------------------------------------------
>>                                  Shenandoah/jython |                                          cwr_total
>> 
>> 
>>                                 Only in experiment |                                    Only in control
>> -------------------------------------------------------------------------------------------------------
>>                  crypto.signverify/trigger_failure |                        crypto.rsa/cmr_thread_roots
>>                 extremem-large-31g/adjust_pointers |       scimark.sparse.small/concurrent_thread_roots
>>             extremem-large-31g/calculate_addresses |              xml.transform/concurrent_thread_roots
>>       crypto.signverify/class_unloading_rendezvous |                    mpegaudio/concurrent_weak_roots
>>                                   serial/cmr_total |                        crypto.rsa/ctr_thread_roots
>> 
>> Shenandoah
>> -------------------------------------------------------------------------------------------------------
>> +5.64% jython/cwr_total p=0.00037
>>   Control:      1.928ms (+/-272.40us)        170
>>   Test:         2.037ms (+/-322.73us)        344
>
> Kelvin Nilsen has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Another attempt at improving clarity of comments

I think it's mostly there. Just a few remainging minor nits.

src/hotspot/share/gc/shenandoah/shenandoahFreeSet.cpp line 100:

> 98: }
> 99: 
> 100: ShenandoahRegionPartitions::~ShenandoahRegionPartitions() {

I think there's no need to put this trivial destructor definition in the .cpp file. Just place it with the declaration in the .hpp file.

src/hotspot/share/gc/shenandoah/shenandoahFreeSet.cpp line 144:

> 142:   assert (which_partition < NumPartitions, "selected free partition must be valid");
> 143:   idx_t idx = _rightmosts[int(which_partition)];
> 144:   // _membership[which_partition].is_set(idx) may not be true if we are shrinking the interval

Same here.

src/hotspot/share/gc/shenandoah/shenandoahFreeSet.cpp line 334:

> 332: const char* ShenandoahRegionPartitions::partition_membership_name(idx_t idx) const {
> 333:   ShenandoahFreeSetPartitionId result = membership(idx);
> 334:   return partition_name(result);

Could make it even more succinct by doing `return partition_name(membership(idx));`

src/hotspot/share/gc/shenandoah/shenandoahSimpleBitMap.cpp line 2:

> 1: /*
> 2:  * Copyright (c) 2016, 2021, Red Hat, Inc. All rights reserved.

The copyright seems wrong. This is entirely new code by you, right? Should only have Amazon copyright notice, then.

src/hotspot/share/gc/shenandoah/shenandoahSimpleBitMap.hpp line 3:

> 1: /*
> 2:  * Copyright (c) 2016, 2019, Red Hat, Inc. All rights reserved.
> 3:  * Copyright Amazon.com Inc. or its affiliates. All Rights Reserved.

Same copyright issue.

src/hotspot/share/gc/shenandoah/shenandoahSimpleBitMap.hpp line 92:

> 90:   }
> 91: 
> 92:   inline idx_t alignment() const {

I guess this could even be constexpr.

src/hotspot/share/gc/shenandoah/shenandoahSimpleBitMap.inline.hpp line 2:

> 1: /*
> 2:  * Copyright (c) 2016, 2019, Red Hat, Inc. All rights reserved.

Same copyright issue.

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

Changes requested by rkennke (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/17561#pullrequestreview-2049787865
PR Review Comment: https://git.openjdk.org/jdk/pull/17561#discussion_r1596812726
PR Review Comment: https://git.openjdk.org/jdk/pull/17561#discussion_r1596814322
PR Review Comment: https://git.openjdk.org/jdk/pull/17561#discussion_r1596840631
PR Review Comment: https://git.openjdk.org/jdk/pull/17561#discussion_r1596860599
PR Review Comment: https://git.openjdk.org/jdk/pull/17561#discussion_r1596950049
PR Review Comment: https://git.openjdk.org/jdk/pull/17561#discussion_r1596949379
PR Review Comment: https://git.openjdk.org/jdk/pull/17561#discussion_r1596950394