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

[Y. Srinivas Ramakrishna]

On Thu, 1 Feb 2024 02:04:43 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:
>> 
>>   Rename and comments for _capacity_of and _used_by
>
> src/hotspot/share/gc/shenandoah/shenandoahFreeSet.hpp line 54:
> 
>> 52: 
>> 53:   // For each type, we track an interval outside of which a region affiliated with that partition is guaranteed
>> 54:   // not to be found. This makes searches for free space more efficient.  For each partition p, _leftmosts[p]
> 
> I am being a bit pedantic here.
> Partition is usually identified with the _set of equivalence classes_. Thus a partition is an equivalence relation, and each equivalence class in the partition has, in this case, a distinct partition id (i.e. each region is either in the Mutator equivalence class aka Mutator free set, the Collector equivalence class aka Collector free set, or the NotFree equivalence class aka NotFree set). In your terminology, each equivalence class is a "free set".

However, upon reading further, I see that you have used "partition" not in the mathematical sense of an equivalence relation on a set, but in the English language sense as a subset of a set. In that case, you can continue to use the terminology you are using, but I'd change the class `ShenandoahRegionPartition` to the plural `ShenandoahRegionPartitions`, since you think of it as the combination of 3 partitions (in the English language sense): a Mutator partition, a Collector partition, and a NotFree partition. Or you could call it `ShenandoahRegionPartitioning`. Indeed, in your comment above, you say "This class represents a partitioning of ...".

> src/hotspot/share/gc/shenandoah/shenandoahFreeSet.hpp line 184:
> 
>> 182:   HeapWord* allocate_single(ShenandoahAllocRequest& req, bool& in_new_region);
>> 183: 
>> 184:   // While holding the heap lock, allocate memory for a humongous object which will span multiple contiguous heap
> 
> `which will` or `which may`? (Is a humongous object allowed to span just a single region as well?)
> 
> Or are objects humongous only if they won't fit in a region? In which case the "will" is correct.
> 
> I was confused by tests that use `ShenandoahHumongousThreshold=50` , `=90`, etc.
> 
> May be in those cases, we go through the `allocate_single()` despite allocating an object (or block) bigger than `ShenandoahHeapRegion::humongous_threshold_words()` ? (That would make the pre-condition of the previous method suspect, though.)

Same remark applies to the precondition comment below (which is correct, but could be made stronger to say `req.size() > ShenandoahHeapRegion::RegionSizeWords` or such?

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

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

PR Review Comment: https://git.openjdk.org/jdk/pull/17561#discussion_r1473717128
PR Review Comment: https://git.openjdk.org/jdk/pull/17561#discussion_r1473736515
PR Review Comment: https://git.openjdk.org/jdk/pull/17561#discussion_r1473892964