RFR: 8334513: New test gc/TestAlwaysPreTouchBehavior.java is failing [v2]

[Albert Mingkun Yang]

On Fri, 28 Jun 2024 19:22:32 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

>> See JBS issue.
>> 
>> It is not completely obvious what the problem is in Oracle's CI, but the current assumption is that RSS of the testee VM gets reduced after it started and before we measured due to memory pressure.
>> 
>> The patch:
>> - exposes os::available_memory via Whitebox
>> - For the test to count as failed, we require a certain minimum size of available memory both before and during the start of the testee JVM. Otherwise, we throw a `SkippedException`
>> 
>> I have some misgivings about this solution, though:
>> 1) obviously, it is not bullet-proof either, since it is vulnerable to fast changes in machine memory load. 
>> 2) On MacOS, we have the problem that 'os::available_memory()' totally underreports how much memory is available. Therefore, as an estimate of whether the test is valid, it is too conservative. I opened https://bugs.openjdk.org/browse/JDK-8334767 to track that issue. As long as it is not fixed, the tests will likely fall below the threshold on MacOS and, therefore, be skipped. Still, this is somewhat better than outright excluding the test for MacOS (or is it? Open to opinions)
>> 3) `SkippedException` leads to the test counting as "passed", not "skipped". I think that is a usability issue with jtreg. I cannot easily see which tests had been skipped due to SkippedException.
>> 
>> Despite my doubts, I think this is the best we can come up with if we want to have such a test.
>> 
>> Note: One way to go about (3) would be to make "minimum available memory" a `@requires` tag, similar to os.maxMemory. However, I fear that this may be easily misused and cause many tests to be excluded without notice.
>
> Thomas Stuefe has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Update TestAlwaysPreTouchBehavior.java

Some readability suggestions.

test/hotspot/jtreg/gc/TestAlwaysPreTouchBehavior.java line 164:

> 162:             if (rss < committed) {
> 163:                 if (avail < requiredAvailableDuring) {
> 164:                     throw new SkippedException("Not enough memory for this  test (" + avail + ")");

This is essentially an early-return; why is this inside the `rss < committed` comparison? Does it work if it's lifted up? The structure I have in mind is like:


if (avail < ....) {
  skip-test;
}
assert(rss >= committed, error-msg);

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

Marked as reviewed by ayang (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/19803#pullrequestreview-2151337452
PR Review Comment: https://git.openjdk.org/jdk/pull/19803#discussion_r1661031216