RFR: 8324817: Parallel GC does not pre-touch all heap pages when AlwaysPreTouch enabled and large page disabled [v5]

[Thomas Schatzl]

On Thu, 1 Feb 2024 03:21:15 GMT, Wang Zhuo <wzhuo at openjdk.org> wrote:

>> AlwaysPreTouch requires all freshly committed pages to be pre-touched. While currently PS GC does not pre-touch heap pages with -XX:+AlwaysPreTouch.
>> It is related to [JDK-8315923](https://bugs.openjdk.org/browse/JDK-8315923), which fixes the issue when huge pages are used. But the bug still stands if regular page are used. On linux we can reproduce this bug when /sys/kernel/mm/transparent_hugepage/enabled is madvise or never, but cannot reproduce when it is always.
>> 
>> A simple way to reproduce, please make sure large pages are NOT used.
>> Just run a test with the following parameters
>> -XX:+AlwaysPreTouch -Xms31g -Xmx31g -XX:+UseParallelGC
>> There is no pre-touching during heap initialing stage. After initialization, RSS of the test process is much smaller than memory committed.
>> On the contrary, using -XX:+UseG1GC and run the test again
>> -XX:+AlwaysPreTouch -Xms31g -Xmx31g -XX:+UseG1GC
>> it takes several seconds to pre-touch heap pages during initialization, and RSS usage after initialization is similar to memory committed. This is the expected behavior of AlwaysPreTouch
>> 
>> This issue related to [JDK-8283935](https://bugs.openjdk.org/browse/JDK-8283935), which uses alignment() instead of os::vm_page_size() as pre-touching step size. The value of alignment() is usually bigger than OS page size, which causes most heap pages are not pre-touched.
>
> Wang Zhuo has updated the pull request incrementally with one additional commit since the last revision:
> 
>   update test

Changes requested by tschatzl (Reviewer).

test/hotspot/jtreg/gc/parallel/TestAlwaysPreTouchBehavior.java line 31:

> 29:  * @requires vm.gc.Parallel
> 30:  * @requires os.family == "linux" & os.maxMemory > 2G
> 31:  * @library /test/lib

The test only works in release builds as the debug builds incidentally always pretouch bases.
I.e. add `@requires vm.debug != true`.
There does not seem to be a `@requires vm.release` or something.

test/hotspot/jtreg/gc/parallel/TestAlwaysPreTouchBehavior.java line 69:

> 67: 
> 68:         String content = stringBuilder.toString();
> 69:         return Long.valueOf(content.split("\\s+")[1].trim());

There does not need to be a reason to use the `StringBuilder` here. I also do not see a reason for the `deleteCharAt` call. Just use `line`.

Suggestion:

        String line = null;
        while ((line = reader.readLine()) != null) {
            if (line.startsWith("VmRSS:")) {
                break;
            }
        }
        reader.close();

        return Long.valueOf(line.split("\\s+")[1].trim());

If `line` is `null`, the NPE will be caught in the `catch` in the caller all the same.

test/hotspot/jtreg/gc/parallel/TestAlwaysPreTouchBehavior.java line 76:

> 74:     long committedMemory = runtime.totalMemory() / 1024; // in kb
> 75:     try {
> 76:        rss = getProcessRssInKb();

Inconsistent indentation compared to other indentation - this is 3 spaces, the file uses 4 spaces otherwise.

test/hotspot/jtreg/gc/parallel/TestAlwaysPreTouchBehavior.java line 78:

> 76:        rss = getProcessRssInKb();
> 77:     } catch (Exception e) {
> 78:        rss = EXCEPTION_VALUE;

Indentation.

test/hotspot/jtreg/gc/parallel/TestAlwaysPreTouchBehavior.java line 83:

> 81:         System.out.println("cannot get RSS, just skip");
> 82:         return; // Did not get avaiable RSS, just ignore this test
> 83:     }

The `EXCEPTION_VALUE` constant seems superfluous, and the marked code can be simplified to just

Suggestion:

    } catch (Exception e) {
        System.out.println("cannot get RSS, just skip");
        return; // Did not get avaiable RSS, just ignore this test
    }


imo.

Actually, thinking a bit further, I think there is no reason to actually try to catch anything here. If the `proc` filesystem isn't readable (or something changes in its contents) I would think it is worth looking at the environment or fixing the test.

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

PR Review: https://git.openjdk.org/jdk/pull/17610#pullrequestreview-1856238863
PR Review Comment: https://git.openjdk.org/jdk/pull/17610#discussion_r1474485488
PR Review Comment: https://git.openjdk.org/jdk/pull/17610#discussion_r1474310030
PR Review Comment: https://git.openjdk.org/jdk/pull/17610#discussion_r1474311456
PR Review Comment: https://git.openjdk.org/jdk/pull/17610#discussion_r1474311692
PR Review Comment: https://git.openjdk.org/jdk/pull/17610#discussion_r1474287863