RFR: JDK-8305416: runtime/Thread/TestAlwaysPreTouchStacks.java failed with "Did not find expected NMT output" [v2]

[Gerard Ziemski]

On Wed, 5 Apr 2023 07:05:59 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

>> Fix test. Several issues caused the test to fail intermittently (only on Windows for some reason).
>> 
>> The test starts a a child process with -XX:+AlwaysPreTouchStacks. In this child process, it creates n threads with given stack sizes, then expects them to come up successfully, then checks the final NMT output for the expected stack committed size (which should ideally be very close to the stack reserved size, in contrast to a VM started without -AlwaysPreTouchStacks).
>> 
>> The test had several errors:
>> - the test threads exited, and they were non-daemon threads - therefore, the final NMT report printed during JVM shutdown did not account for the test thread stacks.
>> - The test threads create log output (via Xlog). If that output gets mixed into the final NMT report, the report parser gets confused. This is what happened on Windows, and this made the error intermittent.
>> 
>> ---
>> 
>> The new, fixed test:
>> - creates the test threads as daemon threads and makes them loop endlessly, so they stick around till VM death and show up in the final NMT report. We also create more test threads with noticeably larger stacks for them to have a visible impact on NMT-reported stack sizes.
>> - removed the Xlog output
>> - The test main thread waits until all test threads have successfully started.
>> - I removed the testing of different stack sizes since it was not useful
>> - I made the test stricter: 
>>    - The total committed thread stack size should be more than 3/4ths of the total committed reserved stack size for the test to succeed. This, in combination with large test thread stacks, is enough to prove that PreTouch is working
>>    - I also added a santity test for the total reserved stack size to be larger - but not insanely so - than the combined test thread stack sizes to prove that what we see is caused by our test threads.
>> 
>> Tests: manually ran tests on Windows x64 and Linux x64.
>
> Thomas Stuefe has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains four additional commits since the last revision:
> 
>  - Merge branch 'master' into JDK-8305416-runtime-Thread-TestAlwaysPreTouchStacks-fails
>  - feedback david
>  - Merge branch 'master' into JDK-8305416-runtime-Thread-TestAlwaysPreTouchStacks-fails
>  - JDK-8305416-runtime-Thread-TestAlwaysPreTouchStacks-fails

Marked as reviewed by gziemski (Committer).

test/hotspot/jtreg/ProblemList.txt line 1:

> 1: #

Wouldn't it be simpler to rewrite this as a gtest and allocate using VM os::alloc() once then check that?

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

> 138:                     long max_reserved = memoryCeilingMB * 3 * MB;
> 139:                     long min_reserved = memoryCeilingMB * MB;
> 140:                     if (reserved >= max_reserved || reserved < min_reserved) {

Why is `reserved >= max_reserved` a concern? Would we really want/need to look into this if it did happen?

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

PR Review: https://git.openjdk.org/jdk/pull/13295#pullrequestreview-1400724796
PR Review Comment: https://git.openjdk.org/jdk/pull/13295#discussion_r1177044382
PR Review Comment: https://git.openjdk.org/jdk/pull/13295#discussion_r1177031884