RFR: 8324649: Shenandoah: refactor implementation of free set [v6]

[Y. Srinivas Ramakrishna]

On Thu, 1 Feb 2024 07:10:17 GMT, Y. Srinivas Ramakrishna <ysr at openjdk.org> wrote:

>> src/hotspot/share/gc/shenandoah/shenandoahFreeSet.hpp line 221:
>> 
>>> 219:   //
>>> 220:   // Note that we plan to replenish the Collector reserve at the end of update refs, at which time all
>>> 221:   // of the regions recycled from the collection set will be available.
>> 
>> I see that you are trying to motivate this API. I feel that these comments belong in the caller. The API should not need to motivate where the caller must call this from. The API came about because there was a need for this in its clients. A good API spec should state its actions.
>> 
>> Motivating its uses drags in context that detracts from clarity of the class and method.
>> 
>> I realize this is a somewhat subjective stance but from experience it makes for better documentation and more maintainable/readable code.
>> 
>> The place for such documentation is usually in a block comment motivating the general design of the class and why it offers the APIs that it does, and who its clients are.
>
> So the documentation here might be:
> 
> // Move cset_regions number of regions from being available to the collector to
> // being available to the mutator.
> //
> // Typical usage is at the end of evacuation, when the collector no longer needs
> // the regions that were reserved for evacuation, and these can now be
> // made available for mutator allocation.
> 
> BTW, why call the number of regions `cset_regions`?

Also, the concept of partition is itself an internal implementation detail that you have carefully encapsulated in this class. There is no point in leaking that out in the naming of the method.

The method can just be called `move_regions_from_collector_to_mutator(size_t num)` and be done? "Partition" here adds no value and can be confusing leakage of abstraction.

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

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