RFR: 8333769: Pretouching tests dont test pretouching

[Thomas Stuefe]

On Thu, 13 Jun 2024 13:24:42 GMT, Sonia Zaldana Calles <szaldana at openjdk.org> wrote:

> Hi all, 
> 
> This PR addresses [8333769](https://bugs.openjdk.org/browse/JDK-8333769).
> 
> We already have a test for  parallel GC that makes sure pretouching behaviour is correct ([test/hotspot/jtreg/gc/parallel/TestAlwaysPreTouchBehavior.java](https://github.com/openjdk/jdk/blob/master/test/hotspot/jtreg/gc/parallel/TestAlwaysPreTouchBehavior.java)). 
> 
> Unfortunately this test is limited to linux because of the scanning of `/proc/pid/status`. With this patch I propose two changes: 
> 
> - Adding a function to the os namespace `os::rss` and exposing this API via WhiteBox. This in turn allows us to generalize the above test to be used across all platforms. 
> - Running the modified test with all collectors. 
> 
> Additionally, I considered removing other pre-existing pretouch tests (for example, this [z test](https://github.com/openjdk/jdk/blob/master/test/hotspot/jtreg/gc/z/TestAlwaysPreTouch.java)), as this new test is a bit more thorough. However, I noticed that some of these tests run alongside other configurables such as varying numbers of parallel GC threads, varying heap sizes, etc. Therefore, there might not be any harm in running these tests as well. 
> 
> Looking forward to your comments, 
> Sonia

Good in general. Does it run as part of GHAs?

src/hotspot/os/aix/os_aix.cpp line 290:

> 288: }
> 289: 
> 290: julong os::rss() { return (julong)0; }

Please make the return code a size_t

src/hotspot/os/windows/os_windows.cpp line 868:

> 866:   BOOL ret = GetProcessMemoryInfo(
> 867:       GetCurrentProcess(), (PROCESS_MEMORY_COUNTERS *)&pmex, sizeof(pmex));
> 868:   if (ret != 0) {

Suggestion:

  if (ret) {

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

> 1: /*
> 2:  * Copyright (c) 2014, 2024, Alibaba Group Holding Limited. All rights reserved.

Add us pls, this is a significant change.

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

> 23: 
> 24: package gc;
> 25: 

- Exclude for AIX
- We can probably manage with less heap. We need a heap size that clearly sticks out above the background noise of normal RSS, but 512MB or 256 MB should probably do the trick too.

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

> 26: /**
> 27:  * @test id=ParallelCollector
> 28:  * @summary Tests AlwaysPreTouch Behavior, pages of java heap should be pretouched with AlwaysPreTouch enabled. This test reads RSS of test process, which should be bigger than heap size(1g) with AlwaysPreTouch enabled.

Since we do this n times, I'd cut down the subject length to "test AlwaysPreTouch".

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

> 78:  * @run main/othervm -Xbootclasspath/a:. -XX:+UnlockDiagnosticVMOptions -XX:+WhiteBoxAPI -XX:+UseZGC -XX:-ZGenerational -Xmx1g -Xms1g -XX:+AlwaysPreTouch gc.TestAlwaysPreTouchBehavior
> 79:  */
> 80: 

stray newline

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

> 106:     }
> 107:     Runtime runtime = Runtime.getRuntime();
> 108:     long committedMemory = runtime.totalMemory() / 1024; // in kb

Why divide by KB? Seems off.

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

> 107:     Runtime runtime = Runtime.getRuntime();
> 108:     long committedMemory = runtime.totalMemory() / 1024; // in kb
> 109:     Asserts.assertGreaterThanOrEqual(rss, committedMemory, "RSS of this process(" + rss + "kb) should be bigger than or equal to committed heap mem(" + committedMemory + "kb)");

Hmm, should be greater, really. Heap should be fully touched, and then we should have plenty touched memory that is not heap.

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

PR Review: https://git.openjdk.org/jdk/pull/19699#pullrequestreview-2120250479
PR Review Comment: https://git.openjdk.org/jdk/pull/19699#discussion_r1640860382
PR Review Comment: https://git.openjdk.org/jdk/pull/19699#discussion_r1640856604
PR Review Comment: https://git.openjdk.org/jdk/pull/19699#discussion_r1640865166
PR Review Comment: https://git.openjdk.org/jdk/pull/19699#discussion_r1640864776
PR Review Comment: https://git.openjdk.org/jdk/pull/19699#discussion_r1640866947
PR Review Comment: https://git.openjdk.org/jdk/pull/19699#discussion_r1640867216
PR Review Comment: https://git.openjdk.org/jdk/pull/19699#discussion_r1640871782
PR Review Comment: https://git.openjdk.org/jdk/pull/19699#discussion_r1640863065