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

[Kelvin Nilsen]

On Wed, 13 Mar 2024 20:01:25 GMT, Roman Kennke <rkennke at openjdk.org> wrote:

>> 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
>
> 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.

I will see if I can exploit the services in src/hotspot/share/utilities to improve performance.

> 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?

Was trying to avoid impacting beyond the boundaries of Shenandoah.  I can tackle this if we think it preferable.  But also trying to work toward "timely integration".

> 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?

I will change to uintx and intx.  Thanks.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/17561#discussion_r1532762788
PR Review Comment: https://git.openjdk.org/jdk/pull/17561#discussion_r1532764524
PR Review Comment: https://git.openjdk.org/jdk/pull/17561#discussion_r1532765287