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

[Y. Srinivas Ramakrishna]

On Wed, 15 May 2024 13:51:54 GMT, Kelvin Nilsen <kdnilsen at openjdk.org> wrote:

>> src/hotspot/share/gc/shenandoah/shenandoahFreeSet.cpp line 105:
>> 
>>> 103:   ShenandoahHeapRegion* r = _heap->get_region(idx);
>>> 104:   return can_allocate_from(r);
>>> 105: }
>> 
>> Some of this abstraction leakage can be avoided by making
>> 
>> "can_allocate_from(size_t idx)" a method on the `_heap` object, and calling that, letting the heap do the test:
>> 
>> ``` 
>> bool ShenandoahHeap::can_allocate_from(size_t idx) {
>>   ShenandoahHeapRegion* r = get_region(idx);
>>   return r->is_empty() || (r->is_trash() && !is_concurrent_weak_root_in_progress());
>> }
>> 
>> 
>> with ShFreeSet merely delegating to its `_heap`.
>> 
>> Probably similar pattern for `alloc_capacity` method below.
>> 
>> (I'll keep leaving single comments, so I don't slow down the review/feedback across potential interruptions. I don't expect there will be many, but still.)
>
> I'm not sure I understand the rationale for these changes.  In my mind, the "notions" of can_allocate_from() and alloc_capacity() as used here within ShenandoahFreeSet are not universal.  These notions are specific to what the free set understands about the life-cycle of regions.  I think moving this functionality into heap actually leaks more encapsulation, in that nobody else cares to use these novel interpretations of can_allocate_from() and alloc_capacity().
> 
> Maybe you can elaborate?

I was looking at the structural effect of moving the methods there and failed to see that these were policies encoded in the free set abstraction rather than policies of (or characteristics of) ShenandoahHeap _used_ by the free set (as I was thinking).

No change is needed in that case. Sorry for my confusion.

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

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