RFR: Restrict generational mode to adaptive heuristic [v2]

[Aleksey Shipilev]

On Mon, 27 Mar 2023 16:30:17 GMT, William Kemper <wkemper at openjdk.org> wrote:

>> test/hotspot/jtreg/gc/shenandoah/TestAllocObjectArrays.java line 167:
>> 
>>> 165:  * @run main/othervm -XX:+UnlockDiagnosticVMOptions -XX:+UnlockExperimentalVMOptions -Xmx1g -Xms1g
>>> 166:  *      -XX:+UseShenandoahGC -XX:ShenandoahGCHeuristics=adaptive -XX:ShenandoahGCMode=generational
>>> 167:  *      -XX:-UseTLAB -XX:+ShenandoahVerify
>> 
>> This `-XX:+ShenandoahVerify` seems excessive. This config is supposed to run without verification. Copy-paste error, it seems.
>
> The first execution in this `no-tlab` configuration runs the default mode and heuristic with verification enabled, so I'm doing the same for the generational mode configuration.

Ah right, it differs from the above by mode. Nevermind!

>> test/hotspot/jtreg/gc/shenandoah/TestRegionSamplingLogging.java line 31:
>> 
>>> 29:  * @run main/othervm -Xmx1g -Xms1g -XX:+UnlockDiagnosticVMOptions -XX:+UnlockExperimentalVMOptions
>>> 30:  *      -XX:+ShenandoahRegionSampling -XX:+ShenandoahRegionSampling
>>> 31:  *      -Xlog:gc+region=trace:region-snapshots-%p.log::filesize=100,filecount=3
>> 
>> So, `trace` is enough here? This looks like something we need to fix upstream, rather than do in a bulk change like this.
>
> This feature of writing the region sampling data out to logs is only on the generational branch at this time. It wouldn't be hard to pull it out into a separate upstream PR.

`TestRegionSamplingLogging` is only in generational branch? Then disregard this comment :)

>> test/hotspot/jtreg/gc/shenandoah/TestRetainObjects.java line 107:
>> 
>>> 105:  * @run main/othervm -Xmx1g -Xms1g -XX:+UnlockDiagnosticVMOptions -XX:+UnlockExperimentalVMOptions
>>> 106:  *      -XX:+UseShenandoahGC -XX:ShenandoahGCHeuristics=adaptive -XX:ShenandoahGCMode=generational
>>> 107:  *      -XX:+ShenandoahAllocFailureALot -XX:+ShenandoahVerify
>> 
>> I think the intent here is to run without `-XX:+ShenandoahVerify`.
>
> This configuration runs with `ShenandoahAllocFailureALot`, the preceeding configuration runs with `ShenandoahOOMDuringEvacALot`. I enabled verification for both to make the tests more rigorous. Verification may find errors in card marking that wouldn't necessarily fail the test otherwise.

True. Apparently, I only read the `*ALot` part, which confused me into believing this is a duplicate block.

>> test/hotspot/jtreg/gc/shenandoah/TestSieveObjects.java line 107:
>> 
>>> 105:  * @run main/othervm -Xmx1g -Xms1g -XX:+UnlockDiagnosticVMOptions -XX:+UnlockExperimentalVMOptions
>>> 106:  *      -XX:+UseShenandoahGC -XX:ShenandoahGCHeuristics=adaptive -XX:ShenandoahGCMode=generational
>>> 107:  *      -XX:+ShenandoahAllocFailureALot -XX:+ShenandoahVerify
>> 
>> Excessive `-XX:+ShenandoahVerify`?
>
> Same rationale for `TestRetainObjects` - are you concerned about the runtime length of the tests?

Yes, same overlook on my side. This is fine.

>> test/hotspot/jtreg/gc/shenandoah/compiler/BarrierInInfiniteLoop.java line 35:
>> 
>>> 33:  */
>>> 34: 
>>> 35:  /**
>> 
>> This looks like a compiler regression test, so we don't need to run it in all these different GC modes.
>
> I added the generational mode to these tests because we've added a "post-write" barrier for card marking and it touches many of the same files. Figured it's better to error on the side of caution.

Yes, but this test chases the particular compiler bug. Running in multiple modes with `-Xcomp` and `-XX:CompileOnly` might not be fast, so it is better to keep these regression tests targeted. It is not the issue for compiler tests that are not regression tests, like `TestCloneBarrier`.

>> test/hotspot/jtreg/gc/shenandoah/generational/TestCLIModeGenerational.java line 34:
>> 
>>> 32:  * @library /testlibrary /test/lib /
>>> 33:  * @build jdk.test.whitebox.WhiteBox
>>> 34:  * @run driver jdk.test.lib.helpers.ClassFileInstaller jdk.test.whitebox.WhiteBox
>> 
>> Generally, these changes should be done separately, not to clobber the actual "generational"-related test change. It is fine to do them here, if these only touch the files than exist only in GenShen.
>
> Right - these files were added to the generational branch, so they were not updated when we merged the WhiteBox renaming changes.

Okay, great. Nevermind then.

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

PR Review Comment: https://git.openjdk.org/shenandoah/pull/233#discussion_r1149607705
PR Review Comment: https://git.openjdk.org/shenandoah/pull/233#discussion_r1149609301
PR Review Comment: https://git.openjdk.org/shenandoah/pull/233#discussion_r1149611649
PR Review Comment: https://git.openjdk.org/shenandoah/pull/233#discussion_r1149612279
PR Review Comment: https://git.openjdk.org/shenandoah/pull/233#discussion_r1149615359
PR Review Comment: https://git.openjdk.org/shenandoah/pull/233#discussion_r1149606273