RFR: 8309191: Reduce JDK dependencies of cgroup support

[Severin Gehwolf]

On Tue, 30 May 2023 13:03:27 GMT, Aleksandar Pejović <duke at openjdk.org> wrote:

> The current code for cgroup support in the JDK has large and expensive dependencies: it uses NIO, streams, and regular expressions. This leads to unnecessary class loading and slows down startup, especially when the code is executed early during an application startup. This is especially a problem for GraalVM, which executes this code during VM startup.
> 
> This PR reduces the dependencies:
> - NIO is replaced with regular `java.io` for file access.
> - Streams are replaced with loops (a side effect of this is that files are read in full whereas previously they could be read up to a certain point, e.g., until a match is found).
> - Regular expressions are replaced with manual tokenization (and for usages of `String.split`, the "regex" is changed to single characters for which `String.split` has a fast-path implementation that avoids the regular expression engine).

I'm concerned about the hard-coding of delimiter values and the added accidential complexity in order to avoid the Regex engine. Note that this test fails due to the delimiter hard-coding:


jdk/internal/platform/cgroup/TestCgroupSubsystemFactory.java


This change seems hard to maintain. How would you ensure this won't regress?

src/java.base/linux/classes/jdk/internal/platform/CgroupInfo.java line 110:

> 108:      */
> 109:     static CgroupInfo fromCgroupsLine(String line) {
> 110:         String[] tokens = line.split("\t");

With this change, we now hard-code the expected delimiter and, thus, depend on what the kernel does. Do we have sufficient evidence this hasn't changed/won't change in the future?

src/java.base/linux/classes/jdk/internal/platform/CgroupSubsystemFactory.java line 97:

> 95:         Logger logger = System.getLogger("jdk.internal.platform");
> 96:         logger.log(Level.DEBUG, msg);
> 97:     }

This seems fine and uncontroversial. Suggested name change `log` over `warn`. Perhaps apply this as a separate change?

src/java.base/linux/classes/jdk/internal/platform/CgroupSubsystemFactory.java line 347:

> 345:             int separatorOrdinal = -1;
> 346:             // loop over space-separated tokens
> 347:             for (int tOrdinal = 1, tStart = 0, tEnd = line.indexOf(' '); tEnd != -1; tOrdinal++, tStart = tEnd + 1, tEnd = line.indexOf(' ', tStart)) {

AFAIK, this now also hard-codes the delimiter: A single space. If we really want this custom parser, please add a unit test for it and extract it to a separate class.

A hypothetical line like the following would confuse the parser, setting ordinal to wrong values:

36   35 98:0 /mnt1 /mnt2 rw,noatime master:1 - cgroup /dev/root rw,errors=continue

We'd have: `mountRoot = 98:0`, `mountPath = /mnt1`, `fsType = cgroup` since it expects single space separated values. Seems fragile.

src/java.base/linux/classes/jdk/internal/platform/cgroupv1/CgroupV1SubsystemController.java line 109:

> 107:     }
> 108: 
> 109:     public static long convertHierachicalLimitLine(String line) {

Pre-existing: typo `convertHierarchicalLimitLine`

src/java.base/linux/classes/jdk/internal/platform/cgroupv1/CgroupV1SubsystemController.java line 110:

> 108: 
> 109:     public static long convertHierachicalLimitLine(String line) {
> 110:         String[] tokens = line.split(" ");

Again, assumes single space (` `) delimited entries in `memory.stat`. I'm not sure we should hard-code this.

src/java.base/linux/classes/jdk/internal/platform/cgroupv2/CgroupV2Subsystem.java line 144:

> 142:         }
> 143:         // $MAX $PERIOD
> 144:         String[] tokens = cpuMaxRaw.split(" ");

This seems OK. According to https://docs.kernel.org/admin-guide/cgroup-v2.html#format

src/java.base/linux/classes/jdk/internal/platform/cgroupv2/CgroupV2Subsystem.java line 362:

> 360:             return Long.valueOf(0);
> 361:         }
> 362:         String[] tokens = line.split(" ");

This seems OK. According to https://docs.kernel.org/admin-guide/cgroup-v2.html#format

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

PR Review: https://git.openjdk.org/jdk/pull/14216#pullrequestreview-1454929344
PR Review Comment: https://git.openjdk.org/jdk/pull/14216#discussion_r1212816038
PR Review Comment: https://git.openjdk.org/jdk/pull/14216#discussion_r1212909205
PR Review Comment: https://git.openjdk.org/jdk/pull/14216#discussion_r1212877800
PR Review Comment: https://git.openjdk.org/jdk/pull/14216#discussion_r1212882937
PR Review Comment: https://git.openjdk.org/jdk/pull/14216#discussion_r1212891278
PR Review Comment: https://git.openjdk.org/jdk/pull/14216#discussion_r1212906805
PR Review Comment: https://git.openjdk.org/jdk/pull/14216#discussion_r1212907029