RFR: Enforce max regions [v3]

[Kelvin Nilsen]

On Wed, 7 Dec 2022 20:50:45 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 white space and add an assertion
>
> src/hotspot/share/gc/shenandoah/shenandoahFreeSet.cpp line 126:
> 
>> 124:       for (size_t idx = _mutator_leftmost; idx <= _mutator_rightmost; idx++) {
>> 125:         ShenandoahHeapRegion* r = _heap->get_region(idx);
>> 126:         if (is_mutator_free(idx) && (allow_new_region || r->affiliation() != ShenandoahRegionAffiliation::FREE)) {
> 
> Aside: Does Shanandoah have a concept of an allocation cursor per mutator in shared space independent of its TLAB? This is because firstly it might make first fit searches more efficient, and secondly we might end up with spatial locality of allocations that are temporally in close proximity from the same mutator, which might help reduce fragmentation and potentially evacuation costs.
> 
> One might consider resetting the cursors following each minor gc.

There is no concept of an allocation cursor per mutator.  In tracing some "anomalous" behaviors, I observed that the search for a heap region with memory available to be allocated can be very cumbersome.  As young memory becomes more scarce, the effort consumed by each thread trying to allocate (under lock by the way) becomes more and more costly, having to sequentially examine large numbers of regions (possibly more than a thousand regions) to find the first region with sufficient space to satisfy the allocation request.  We could definitely make some improvements here, especially because the allocating threads holds the lock throughout this traversal.  Another possible improvement is to not require the global heap lock while searching for a region to serve tlab allocation request.

> src/hotspot/share/gc/shenandoah/shenandoahFreeSet.cpp line 171:
> 
>> 169:             ShenandoahHeapRegion* r = _heap->get_region(idx);
>> 170:             if (can_allocate_from(r)) {
>> 171:               flip_to_gc(r);
> 
> Does the flipping have to strictly precede the allocation attempt? Otherwise the flip is futile and we steal space from mutators but to no advantage.
> 
> I also notice the asymmetry in the existence of `flip_to_gc()` but no corresponding `flip_to_mutator()`. I suppose that's because regions freed by GC as a result of evacuation will be available to mutators, so the flipping to GC may be considered temporary in that sense. However, I suspect futile flipping may strand space in GC territory for no good reason.
> 
> In any case, take my comments here with the right grain of salt because I am lacking the philosophical foundations of the need for this mutator & collector view dichotomy here. It would be good if in the `.hpp` file we expended a few sentences listing the rationale for that design choice; e.g. the allocate from left and allocate from right could still hold without necessarily having strict collector/mutator affiliations (as indicated by the `flip` above)?

There's no flip to mutator because we do not allow a mutator allocation request to take memory that had been set aside for use by the collector (for evacuations).  If a mutator alloc "fails", we can stall that single mutating thread.  If a GC evacuation fails, we have to force all threads into a safepoint so that we can perform a FULL GC.  This is the reason we don't allow mutators to flip_to_mutator().

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

PR: https://git.openjdk.org/shenandoah/pull/179