RFR: 8324649: Shenandoah: refactor implementation of free set [v32]

[Roman Kennke]

On Wed, 13 Mar 2024 19:12:46 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 two additional commits since the last revision:
> 
>  - Remove comment that pertains only to generational mode
>  - Rewrite comment describing rationale for Collector free set membership

Not a complete review by any means, only a few comments/questions on design choices, the answer to which may affect the rest of the review.

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

> 44: }
> 45: 
> 46: size_t ShenandoahSimpleBitMap::count_leading_ones(ssize_t start_idx) const {

Are you sure that you can't use the BitMap class in src/hotspot/share/utilities/bitMap.*? Or maybe only src/hotspot/share/utilities/count_leading_zeros.hpp or src/hotspot/share/utilities/count_trailing_zeros.hpp? That would emit CPU instructions that do the counting really fast, if possible. I.e. reverse all bits after loading, then find leading/trailing zeros, problem solved real fast. I would even argue to augment the BitMap class with count_* methods as needed, using the CPU-optimized routines.

src/hotspot/share/gc/shenandoah/shenandoahFreeSet.hpp line 41:

> 39: 
> 40: 
> 41: // ShenandoahSimpleBitMap resembles CHeapBitMap but adds missing support for find_next_consecutive_bits() and

Ah I see you have considered using bitMap.* ... but why not 'simply' add the required methods there, instead?

src/hotspot/share/gc/shenandoah/shenandoahFreeSet.hpp line 48:

> 46:   const ssize_t _num_bits;
> 47:   const size_t _num_words;
> 48:   size_t* const _bitmap;

I'd question the choice of size_t as type for the bitmap elements. size_t is typically used as size of index of arrays. If you want word-sized type, use intx or uintx instead?

src/hotspot/share/gc/shenandoah/shenandoahFreeSet.inline.hpp line 1:

> 1: /*

I don't think any of the code in shenandoahFreeSet.inline.hpp is public/used by any code outside of shenandoahFreeSet.cpp, which means that all of it should go into shenandoahFreeSet.cpp. The methods can still be 'inline'. The convention for .inline.hpp is mostly used to allow sharing inlined code and resolving circular dependencies.

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

PR Review: https://git.openjdk.org/jdk/pull/17561#pullrequestreview-1935079978
PR Review Comment: https://git.openjdk.org/jdk/pull/17561#discussion_r1523846507
PR Review Comment: https://git.openjdk.org/jdk/pull/17561#discussion_r1523854593
PR Review Comment: https://git.openjdk.org/jdk/pull/17561#discussion_r1523860910
PR Review Comment: https://git.openjdk.org/jdk/pull/17561#discussion_r1523834166