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

[Kelvin Nilsen]

On Tue, 14 May 2024 00:03:47 GMT, Y. Srinivas Ramakrishna <ysr at openjdk.org> wrote:

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

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

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