RFR: 8332362: Implement os::committed_in_range for MacOS and AIX [v6]

[Thomas Stuefe]

On Fri, 14 Jun 2024 13:50:54 GMT, Robert Toyonaga <duke at openjdk.org> wrote:

>> ### Summary
>> This change adds `os::committed_in_range` to `src/hotspot/os/posix/os_posix.cpp` so that AIX and MacOS can use it. It is mostly the same as the prior implementation in `os_linux.cpp`, but takes into account AIX may have unaligned stacks and variable page sizes, and that mincore uses a different argument type on linux.  This will allow AIX and MacOS to report true liveness information for thread stacks reported by NMT. Previously this was only possible on Windows and Linux.
>> 
>> **Testing**
>> - `TestAlwaysPreTouchStacks.java` is updated to also test the case when stacks are large but mostly not paged in. 
>> - A few new unit tests in `test_committed_virtualmemory.cpp` to check`os::committed_in_range`. I've excluded these tests on Windows because it reports the committed region as fully paged in. 
>> - tier1
>> 
>> I've tested on Linux and Windows. I don't have a Mac to test on, but the Mac GHA tests are passing. Not yet tested on AIX.
>
> Robert Toyonaga has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Add #else branch for non-AIX platforms.

Some nits remain, but we are almost done

src/hotspot/os/posix/os_posix.cpp line 161:

> 159: 
> 160:   int mincore_return_value;
> 161:   const size_t stripe = 1024;  // query this many pages each time

constexpr

src/hotspot/os/posix/os_posix.cpp line 162:

> 160:   int mincore_return_value;
> 161:   const size_t stripe = 1024;  // query this many pages each time
> 162:   mincore_vec_t* vec = (mincore_vec_t*) malloc(stripe + 1, mtInternal);

This is only 1KB. I would allocate this on-stack. Lets avoid doing malloc for every invocation of this function. Also saves you the work of manually freeing each time.

src/hotspot/os/posix/os_posix.cpp line 164:

> 162:   mincore_vec_t* vec = (mincore_vec_t*) malloc(stripe + 1, mtInternal);
> 163: 
> 164:   if (vec == NULL) {

Note, we use nullptr, not NULL

src/hotspot/os/posix/os_posix.cpp line 169:

> 167: 
> 168:   // set a guard
> 169:   vec[stripe] = 'X';

- only for ASSERT (DEBUG_ONLY)

src/hotspot/os/posix/os_posix.cpp line 189:

> 187: 
> 188:     // Get stable read
> 189:     while ((mincore_return_value = mincore(loop_base, pages_to_query * page_sz, vec)) == -1 && errno == EAGAIN);

I would like a safety fuse here, e.g. return error after 1000 consecutive fails

test/hotspot/gtest/runtime/test_committed_virtualmemory.cpp line 218:

> 216: }
> 217: 
> 218: #if !defined(_WINDOWS) && !defined(_AIX)

Please also execute for windows. We should use VirtualQuery on windows, that should work.

test/hotspot/gtest/runtime/test_committed_virtualmemory.cpp line 219:

> 217: 
> 218: #if !defined(_WINDOWS) && !defined(_AIX)
> 219: TEST_VM(CommittedVirtualMemory, test_full_committed_in_range){

These two tests are almost the same. Please factor out into a single file static test function, with num pages as arguments, and call that from both TESTs

test/hotspot/gtest/runtime/test_committed_virtualmemory.cpp line 240:

> 238:   for (index = 0; index < num_pages; index ++) {
> 239:   *(base + index * page_sz) = 'a';
> 240:   }

- indentation
- why not just base[index * page_sz]?

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

> 106: 
> 107:         } else {
> 108:             boolean noPreTouch = args.length == 1 && args[0].equals("noPreTouch");

We typically want RuntimeException on parse error, so please parse all possible values. Alternatively make an enum and use its parse function. Or, simply, use Booelan.parse and pass in false and true.

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

> 111:                     "-Xmx100M",
> 112:                     "-XX:NativeMemoryTracking=summary", "-XX:+PrintNMTStatistics"));
> 113:             if (!noPreTouch){

Lets not use double negation. Just `boolean pretouch` ?

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

> 117:             if (isLinux()) {
> 118:                 options.add( "-XX:-UseMadvPopulateWrite");
> 119:             }

Please remove this. There have been discussions about this in [JDK-8324781](https://bugs.openjdk.org/browse/JDK-8324781), and I changed my mind about this. I think we should test the default, whatever the default is (and if pretouching by default is broken on Oracle Linux, so be it).

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

PR Review: https://git.openjdk.org/jdk/pull/19455#pullrequestreview-2120145723
PR Review Comment: https://git.openjdk.org/jdk/pull/19455#discussion_r1640790825
PR Review Comment: https://git.openjdk.org/jdk/pull/19455#discussion_r1640797194
PR Review Comment: https://git.openjdk.org/jdk/pull/19455#discussion_r1640794340
PR Review Comment: https://git.openjdk.org/jdk/pull/19455#discussion_r1640799500
PR Review Comment: https://git.openjdk.org/jdk/pull/19455#discussion_r1640788045
PR Review Comment: https://git.openjdk.org/jdk/pull/19455#discussion_r1640777499
PR Review Comment: https://git.openjdk.org/jdk/pull/19455#discussion_r1640787419
PR Review Comment: https://git.openjdk.org/jdk/pull/19455#discussion_r1640781840
PR Review Comment: https://git.openjdk.org/jdk/pull/19455#discussion_r1640779331
PR Review Comment: https://git.openjdk.org/jdk/pull/19455#discussion_r1640779641
PR Review Comment: https://git.openjdk.org/jdk/pull/19455#discussion_r1640780918