RFR: 8335932: GenShen: Fix old heuristic unit test

[Kelvin Nilsen]

On Mon, 8 Jul 2024 22:03:51 GMT, Kelvin Nilsen <kdnilsen at openjdk.org> wrote:

>> This isn't really a unit test and it must be run with Shenandoah enabled, but it exercises a hard to reach code path. The test has suffered some bit-rot* of late that needs to be addressed.
>> 
>> * We no longer set soft-max-capacity on the old generation.
>
> test/hotspot/gtest/gc/shenandoah/test_shenandoahOldHeuristic.cpp line 89:
> 
>> 87:     _heap->heap_region_iterate(&reset);
>> 88:     _heap->old_generation()->set_capacity(ShenandoahHeapRegion::region_size_bytes() * 10);
>> 89:     _heap->old_generation()->set_evacuation_reserve(ShenandoahHeapRegion::region_size_bytes() * 4);
> 
> I'll approve these changes because I assume you've tested and are satisfied with the results.
> 
> Might be good to leave some comments in this code that warn the user that failure of this test does not necessarily mean we've broken old heuristics.
> 
> What causes me some concern is that setting the capacity and reserves of old generation is very "dangerous".  There are "assumptions" scattered throughout the implementation related to reserves being sufficient to handle anticipated evacuation, and furthermore, any attempt to take control over capacity and reserves may be defeated by the GC's ergonomics...

The fourth argument to finish_rebuild (default value false) is a flag that is supposed to be true when evacuation_reserve() is meaningful.  When this flag is true, we also assume that old_evacuation_reserve() and old_promo_reserve() are valid.  If this flag is false, the freeset rebuilder will ignore the value of evacuation_reserve.

I wonder how we make sure that the evacuation reserve we set in this "unit test" program is not ignored.

(In the share-collector-reserves development branch, I get rid of this flag and assume reserves are always valid.)

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

PR Review Comment: https://git.openjdk.org/shenandoah/pull/458#discussion_r1669388696