RFR: 8324649: Shenandoah: refactor implementation of free set [v39]
Roman Kennke
rkennke at openjdk.org
Wed Apr 10 11:54:18 UTC 2024
On Thu, 28 Mar 2024 16:06:58 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:
>
> Remove debugging instrumentation
Only partial review, but addressing the comments likely changes a lot (types, names, etc) all over the place, so I leave it at that for now.
src/hotspot/share/gc/shenandoah/shenandoahFreeSet.cpp line 42:
> 40: case Mutator: return "Mutator";
> 41: case Collector: return "Collector";
> 42: default: return "Unrecognized";
I believe using an 'enum class' for ShenandoahFreeSetPartitionId means you don't need to have a default.
src/hotspot/share/gc/shenandoah/shenandoahFreeSet.cpp line 49:
> 47: assert((start_idx >= 0) && (start_idx < _num_bits), "precondition");
> 48: size_t array_idx = start_idx >> LogBitsPerWord;
> 49: uintx element_bits = _bitmap[array_idx];
Same here, try to stick with what is done in bitMap.hpp (typedef uintptr_t bm_word_t, you could just as well typedef uintx bm_word_t, if you prefer).
src/hotspot/share/gc/shenandoah/shenandoahFreeSet.cpp line 93:
> 91: uintx complement = ~element_bits;
> 92: uintx trailing_ones;
> 93: if (complement) {
complement is not a bool type. If you meant to say 'if (complement != 0)' then say so, to make it easier for the reader. Same applies for a few other places.
src/hotspot/share/gc/shenandoah/shenandoahFreeSet.cpp line 102:
> 100: } else if (trailing_ones == bits_to_examine) {
> 101: // Tail recursion
> 102: return is_forward_consecutive_ones(start_idx + bits_to_examine, count - bits_to_examine);
How bad can this get? I haven't analyzed the algorithm, but if this is not bounded (e.g. we know that it can only go 1 level deep), then I'd prefer to have this as loop, to avoid stack overflow. Same applies for a few other places.
src/hotspot/share/gc/shenandoah/shenandoahFreeSet.cpp line 186:
> 184: }
> 185:
> 186: ssize_t ShenandoahSimpleBitMap::find_prev_consecutive_bits(
It looks weird to me to have an opening ( and then all the args on the next line. I think in HotSpot, it's usually done like:
ssize_t ShenandoahSimpleBitMap::find_prev_consecutive_bits(const size_t num_bits,
ssize_t last_idx,
const ssize_t boundary_idx) const {
src/hotspot/share/gc/shenandoah/shenandoahFreeSet.hpp line 165:
> 163:
> 164: // Each ShenandoahHeapRegion is associated with a ShenandoahFreeSetPartitionId.
> 165: enum ShenandoahFreeSetPartitionId : uint8_t {
Make this an 'enum class' instead.
I see that you are using the enum as index, in this case you can declare the enum class like 'enum class ShenandoahFreeSetPartitionId : uint8_t'
src/hotspot/share/gc/shenandoah/shenandoahFreeSet.inline.hpp line 31:
> 29: #include "gc/shenandoah/shenandoahFreeSet.hpp"
> 30:
> 31: inline ssize_t ShenandoahSimpleBitMap::find_next_set_bit(ssize_t start_idx, ssize_t boundary_idx) const {
bitMap.hpp calls it get_next_one_offset(), maybe keep names, args (and impl?) the same as there, to make future merge easier? I guess that is true for all methods that also exist in bitMap.hpp. If you do, then say in comment that the whole block is taken from bitMap.* to make review and later merge easier.
-------------
Changes requested by rkennke (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/17561#pullrequestreview-1991417328
PR Review Comment: https://git.openjdk.org/jdk/pull/17561#discussion_r1559263831
PR Review Comment: https://git.openjdk.org/jdk/pull/17561#discussion_r1559270594
PR Review Comment: https://git.openjdk.org/jdk/pull/17561#discussion_r1559274704
PR Review Comment: https://git.openjdk.org/jdk/pull/17561#discussion_r1559280964
PR Review Comment: https://git.openjdk.org/jdk/pull/17561#discussion_r1559286212
PR Review Comment: https://git.openjdk.org/jdk/pull/17561#discussion_r1559262690
PR Review Comment: https://git.openjdk.org/jdk/pull/17561#discussion_r1559301482
More information about the shenandoah-dev
mailing list