RFR: 8335932: GenShen: Fix old heuristic unit test [v2]

[William Kemper]

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

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

The tests are configuring the old generation with enough capacity and reserves to make sure old regions can be added to the collection set. Indeed, these tests are not true unit tests because they must be run with `-XX:+UseShenandoahGC` and if a _real_ collection happens during test execution, all bets are off. There is a lengthy comment at the top of the file explaining these risks and limitations.

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

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