RFR: 8335167: Test runtime/Thread/TestAlwaysPreTouchStacks.java failed with Expected a higher ratio between stack committed and reserved [v5]

[Thomas Stuefe]

On Wed, 18 Sep 2024 14:04:29 GMT, Afshin Zafari <azafari at openjdk.org> wrote:

>> The test program runs once with PreTouch and another without PreTouch stacks. For each run the amounts of reserved and committed stack regions are held and then compared the ratio of with and without PreTouch. The ratio of PreTouch test should be greater than the other (because there will be higher committed regions due to pre-touching the pages).
>> 
>> In other words, the old version of this test ran two tests: 1) `preTouchTest` and 2) `noPreTouchTest` and extracted the amount of reserved and committed memory for stack. The ratio of the committed to reserved for preTouchTest ($ratio = \frac{committed}{reserved}$) is expected to be high (>75%) and for the noPreTouchTest is expected to be small (<50%). These expected amounts are not robust for all cases and resulted in 8335167.
>> 
>> In this PR, only one test is run with two sets of options for preTouch and noPreTouch stack memory. The amount of reserved and committed are stored after each run and the ratio of two runs are compared. It is expected that the preTouch has a greater ratio than the noPreTouch (`preTouch.ratio > noPreTouch.ratio`). 
>> 
>> ### Tests
>> The specific test is run on linux-x64, windows-x64 and macosx-aarch64 in tiers1-5 (100+ times).
>
> Afshin Zafari has updated the pull request incrementally with one additional commit since the last revision:
> 
>   two new expectations added

Hi @afshin-zafari , sorry for the sluggish response. Comments inline.

test/hotspot/jtreg/runtime/Thread/TestAlwaysPreTouchStacks.java line 72:

> 70:                 },
> 71:                 "TestThread-" + num, threadStackSizeMB * MB);
> 72:         t.setDaemon(true);

Since you are here, could you add a comment here?

Suggestion:

        // Let test threads run as daemons to ensure that they are still running and
        // that their stacks are still allocated when the JVM shuts down and the final
        // NMT report is printed.
        t.setDaemon(true);

test/hotspot/jtreg/runtime/Thread/TestAlwaysPreTouchStacks.java line 179:

> 177:           if (ratio_with < ratio_without) {
> 178:             throw new RuntimeException("Expected a higher ratio between stack committed and reserved.");
> 179:           }

I think you can simplify. You improved the test, so we don't really need to look at "reserved" save for some basic sanity tests. The reserved-committed ratio is of no interest save to ensure that committed <= reserved (and we do this in other NMT tests).

For both runs, "reserved" should be near identical: It is the combined stack size for all threads running at VM stop. All test threads should still be alive, so the only thing that can vary randomly is whether or not other threads managed to finish before the NMT report is printed. So, I think, you can simplify to just test that "Reserved", in both runs, is >= combined size of test thread stacks (128MB).

test/hotspot/jtreg/runtime/Thread/TestAlwaysPreTouchStacks.java line 181:

> 179:           }
> 180:           double estimated_stack_usage = 1 * MB;
> 181:           double expected_stack_use_with_pretouch = 0.75 * threadStackSizeMB * MB;

Can you hard-code both values up where the other constants are?

I also would name them clearly for what they are. "estimated_stack_usage" sounds wrong, since we don't really estimate that, the real number should be far lower (for no pretouch) and far higher (for pretouch).

Proposal: `max_stack_usage_with_pretouch` and `min_stack_usage_with_pretouch`

test/hotspot/jtreg/runtime/Thread/TestAlwaysPreTouchStacks.java line 187:

> 185:           double expected_delta = numThreads * (expected_stack_use_with_pretouch - estimated_stack_usage);
> 186:           if ((pretouch_result.committed - no_pretouch_result.committed) < expected_delta) {
> 187:             throw new RuntimeException("Expected a higher delta between stack committed of with and without pretouch.");

Can you print out the expected values, please?

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

PR Review: https://git.openjdk.org/jdk/pull/20531#pullrequestreview-2315943580
PR Review Comment: https://git.openjdk.org/jdk/pull/20531#discussion_r1767106876
PR Review Comment: https://git.openjdk.org/jdk/pull/20531#discussion_r1767083104
PR Review Comment: https://git.openjdk.org/jdk/pull/20531#discussion_r1767101384
PR Review Comment: https://git.openjdk.org/jdk/pull/20531#discussion_r1767102557