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

[Y. Srinivas Ramakrishna]

On Mon, 13 May 2024 21:31:40 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 copyright notice on test file

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

> 91:   void dump_bitmap_row(ssize_t idx) const;
> 92:   void dump_bitmap_range(ssize_t start_idx, ssize_t end_idx) const;
> 93:   void dump_bitmap_all() const;

In the first case you can may be say `region_idx` instead of `idx` ?

Also, it seems `dump_bitmap()` would be sufficient (at least for the case of `dump_bitmap_all()`, although in general for all).

However, given that these are private methods that will only be found in the implementation and nowhere else in its clients' code, may be current naming is ok (indeed, it looks like the first two are work functions in service of the last).

PS: I don't see any current usage of `dump_bitmap_all()`. May be you wanted it in some debug tracing code or something, or keeping it for possible future debugging usage? (Or may be my naive web-page search of this review page is naive/faulty.) It's perhaps useful to keep around for debug/tracing though.

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

> 101: 
> 102:   // Set the partition id for a particular region without adjusting interval bounds or usage/capacity tallies
> 103:   inline void raw_set_membership(size_t idx, ShenandoahFreeSetPartitionId p) {

I realize that the word `set` can be a little ambiguous here (from the mathematical notion of a set as  a collection of things), although the fact that it has a void return type tells us that `set` is the imperative verb here, not a collection of things. May be `raw_assign_membership(size_t region_idx, ShFSPId p)` if you want to, but I may be splitting hairs here :-)

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

PR Review Comment: https://git.openjdk.org/jdk/pull/17561#discussion_r1599257135
PR Review Comment: https://git.openjdk.org/jdk/pull/17561#discussion_r1599257281