RFR: 8324649: Shenandoah: replace implementation of free set [v45]
Roman Kennke
rkennke at openjdk.org
Tue Apr 23 17:33:43 UTC 2024
On Mon, 22 Apr 2024 23:38:04 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:
>
> Fix typo
Ok, I've reviewed all but the shenandoahSimpleBitMap.* stuff, which I want to look at in a separate round.
This is overall much better to read, thanks for the updates! I have a few comments.
src/hotspot/share/gc/shenandoah/shenandoahFreeSet.cpp line 130:
> 128: }
> 129:
> 130: inline idx_t ShenandoahRegionPartitions:: leftmost(ShenandoahFreeSetPartitionId which_partition) const {
There is an excess whitespace after ::
src/hotspot/share/gc/shenandoah/shenandoahFreeSet.cpp line 142:
> 140:
> 141: inline idx_t ShenandoahRegionPartitions::rightmost(ShenandoahFreeSetPartitionId which_partition) const {
> 142: assert (int(which_partition) < NumPartitions, "selected free partition must be valid");
Is the cast needed because NumPartitions is int? You could also make NumPartitions be uintx_8 or make the enum class a intx_8 instead, and avoid some casts? Also seems more natural to have NumPartitions be the same type as the enum.
src/hotspot/share/gc/shenandoah/shenandoahFreeSet.cpp line 164:
> 162: idx_t mutator_leftmost_empty, idx_t mutator_rightmost_empty,
> 163: size_t mutator_region_count, size_t mutator_used) {
> 164: _region_counts[int(ShenandoahFreeSetPartitionId::Mutator)] = mutator_region_count;
uhh all those casts are kinda ugly. I wasn't aware that this is necessary when I suggested enum classes. But, afaict, this is the least intrusive way to use the enum as indices, short of going back to C-style-enums. Too bad (and sorry).
src/hotspot/share/gc/shenandoah/shenandoahFreeSet.cpp line 227:
> 225: }
> 226:
> 227: inline void ShenandoahRegionPartitions::shrink_interval_if_boundary_modified(ShenandoahFreeSetPartitionId partition, idx_t idx) {
This almost looks like a variant of shrink_interval_if_range_modifies_either_boundary(...), maybe it could just call shrink_interval_if_range_modifies_either_boundary(partition, idx, idx)?
src/hotspot/share/gc/shenandoah/shenandoahFreeSet.cpp line 365:
> 363: assert (idx < _max, "index is sane: " SIZE_FORMAT " < " SIZE_FORMAT, idx, _max);
> 364: ShenandoahFreeSetPartitionId result = ShenandoahFreeSetPartitionId::NotFree;
> 365: for (uint partition_id = 0; partition_id < NumPartitions; partition_id++) {
Looks to me like you could just call the below membership() method (and make membership() available outside of ASSERT), and then pass the result to partition_name()?
src/hotspot/share/gc/shenandoah/shenandoahFreeSet.cpp line 401:
> 399: }
> 400:
> 401: inline idx_t ShenandoahRegionPartitions::find_index_of_next_available_region(
If I read this correctly, calling code assumes that the result is >= start_index, right? Otherwise loops might not terminate. Maybe assert that this holds?
src/hotspot/share/gc/shenandoah/shenandoahFreeSet.cpp line 413:
> 411: }
> 412:
> 413: inline idx_t ShenandoahRegionPartitions::find_index_of_previous_available_region(
Same as above but other way around?
src/hotspot/share/gc/shenandoah/shenandoahFreeSet.cpp line 906:
> 904: }
> 905:
> 906: r->set_update_watermark(r->bottom());
Why is that here? It means we would update-refs in this humongous object, but why is it needed in a new object?
-------------
PR Review: https://git.openjdk.org/jdk/pull/17561#pullrequestreview-2017386664
PR Review Comment: https://git.openjdk.org/jdk/pull/17561#discussion_r1576332123
PR Review Comment: https://git.openjdk.org/jdk/pull/17561#discussion_r1576481126
PR Review Comment: https://git.openjdk.org/jdk/pull/17561#discussion_r1576510630
PR Review Comment: https://git.openjdk.org/jdk/pull/17561#discussion_r1576519664
PR Review Comment: https://git.openjdk.org/jdk/pull/17561#discussion_r1576589712
PR Review Comment: https://git.openjdk.org/jdk/pull/17561#discussion_r1576605590
PR Review Comment: https://git.openjdk.org/jdk/pull/17561#discussion_r1576606244
PR Review Comment: https://git.openjdk.org/jdk/pull/17561#discussion_r1576612773
More information about the hotspot-gc-dev
mailing list