Add the containerV2 flag to compute initial heap percentage using processor count, and disable setting the ActiveProcessorCount JVM option

README.md

@@ -148,7 +148,7 @@ All output from `go-java-launcher` itself, and from the launch of all processes
 
 ## Java heap and container support
 
-By default, when starting a java process inside a container (as indicated by the presence of ``CONTAINER`` env
+By _default_, when starting a java process inside a container (as indicated by the presence of ``CONTAINER`` env
 variable):
 
 1. If `-XX:ActiveProcessorCount` is unset, it will be set based on the discovered cgroup configurations and host
@@ -162,6 +162,16 @@ variable):
 This will cause the JVM 11+ to discover the ``MaxRAM`` value using Linux cgroups, and calculate the heap sizes as the specified
 percentage of ``MaxRAM`` value, e.g. ``max-heap-size = MaxRAM * MaxRamPercentage``.
 
+If the experimental flag `containerV2` is set:
+1. The `-XX:ActiveProcessorCount` is unset, it will remain unset.
+1. Args with prefix``-Xmx|-Xms`` in both static and custom jvm opts will be filtered out. If neither 
+ ``-XX:MaxRAMPercentage=`` nor ``-XX:InitialRAMPercentage=`` prefixes are present in either static or custom jvm opts,
+ ``-Xmx|-Xms`` will both be set to be 75% of the heap size minus 3mb per processor, with a minimum value of 50% of the 
+  heap.
+1. If neither ``-XX:MaxRAMPercentage=`` nor ``-XX:InitialRAMPercentage=`` prefixes are present in either static or
+   custom jvm opts, both will be set to ``75.0`` (i.e. ``-XX:InitialRAMPercentage=75.0 -XX:MaxRAMPercentage=75.0 `` will
+   be appended after all the other jvm opts).
+
 ### Overriding default values
 
 Developers can override the heap percentage in containers by specifying both ``-XX:MaxRAMPercentage=``

changelog/@unreleased/pr-361.v2.yml

@@ -0,0 +1,7 @@
+type: improvement
+improvement:
+  description: When the experimental `containerV2` is set, 1) compute the heap percentage
+    as 75% of the heap minus 3mb per processor, with a minimum value of 50%, and 2)
+    don't set the -XX:ActiveProcessorCount JVM option.
+  links:
+  - https://github.com/palantir/go-java-launcher/pull/361

integration_test/go_java_launcher_integration_test.go

@@ -45,9 +45,10 @@ func TestMainMethod(t *testing.T) {
 
 func TestMainMethodContainerSupportEnabled(t *testing.T) {
 	for _, tc := range []struct {
-		name            string
-		launcherCustom  string
-		expectedJVMArgs string
+		name               string
+		launcherCustom     string
+		expectedJVMArgs    string
+		expectedJVMArgKeys []string
 	}{
 		{
 			name:            "sets defaults",
@@ -69,9 +70,33 @@ func TestMainMethodContainerSupportEnabled(t *testing.T) {
 			launcherCustom:  "testdata/launcher-custom-initial-and-max-ram-percentage-override.yml",
 			expectedJVMArgs: "-XX\\:InitialRAMPercentage=79.9 -XX\\:MaxRAMPercentage=80.9 -XX\\:ActiveProcessorCount=2",
 		},
+		{
+			name:               "using containerV2 sets Xms and Xmx and does not set ActiveProcessorCount",
+			launcherCustom:     "testdata/launcher-custom-experimental-container-v2.yml",
+			expectedJVMArgs:    "",
+			expectedJVMArgKeys: []string{"-Xmx", "-Xms"},
+		},
+		{
+			name: "using containerV2 with InitialRAMPercentage does not set Xms, Xmx, or " +
+				"ActiveProcessorCount",
+			launcherCustom:  "testdata/launcher-custom-experimental-container-v2-with-initial-ram-percentage.yml",
+			expectedJVMArgs: "-XX\\:InitialRAMPercentage=70.0",
+		},
+		{
+			name: "using containerV2 with MaxRAMPercentage does not set Xms, Xmx, or " +
+				"ActiveProcessorCount",
+			launcherCustom:  "testdata/launcher-custom-experimental-container-v2-with-max-ram-percentage.yml",
+			expectedJVMArgs: "-XX\\:MaxRAMPercentage=70.0",
+		},
+		{
+			name:               "using containerV2 does not use user-provided Xms or Xmx",
+			launcherCustom:     "testdata/launcher-custom-experimental-container-v2.yml",
+			expectedJVMArgs:    "",
+			expectedJVMArgKeys: []string{"-Xmx", "-Xms"},
+		},
 	} {
 		t.Run(tc.name, func(t *testing.T) {
-			testContainerSupportEnabled(t, tc.launcherCustom, tc.expectedJVMArgs)
+			testContainerSupportEnabled(t, tc.launcherCustom, tc.expectedJVMArgs, tc.expectedJVMArgKeys)
 		})
 	}
 }
@@ -97,7 +122,7 @@ func TestMainMethodContainerSupportDisabled(t *testing.T) {
 		},
 	} {
 		t.Run(tc.name, func(t *testing.T) {
-			testInContainer(t, tc.launcherCustom, tc.containerSupportMessage, tc.expectedJVMArgs)
+			testInContainer(t, tc.launcherCustom, tc.containerSupportMessage, tc.expectedJVMArgs, []string{})
 		})
 	}
 }
@@ -119,7 +144,7 @@ func TestMainMethodWithoutCustomConfig(t *testing.T) {
 }
 
 func TestMainMethodContainerWithoutCustomConfig(t *testing.T) {
-	output := testContainerSupportEnabled(t, "foo", "-XX\\:InitialRAMPercentage=75.0 -XX\\:MaxRAMPercentage=75.0 -XX\\:ActiveProcessorCount=2")
+	output := testContainerSupportEnabled(t, "foo", "-XX\\:InitialRAMPercentage=75.0 -XX\\:MaxRAMPercentage=75.0 -XX\\:ActiveProcessorCount=2", []string{})
 	assert.Regexp(t, `Failed to read custom config file, assuming no custom config: foo`, output)
 }
 
@@ -178,6 +203,42 @@ func TestSubProcessesParsedMonitorSignals(t *testing.T) {
 	assert.Len(t, trapped.FindAll(output.Bytes(), -1), 2, "expect two messages that SIGPOLL was caught")
 }
 
+func TestComputeJVMHeapSize(t *testing.T) {
+	for _, tc := range []struct {
+		name                string
+		numHostProcessors   int
+		memoryLimit         uint64
+		expectedMaxHeapSize uint64
+	}{
+		{
+			name:              "at least 50% of heap",
+			numHostProcessors: 1,
+			memoryLimit:       10 * launchlib.BytesInMebibyte,
+			// 75% of heap - 3mb*processors = 4.5mb
+			expectedMaxHeapSize: 5 * launchlib.BytesInMebibyte,
+		},
+		{
+			name:              "computes 75% of heap minus 3mb per processor",
+			numHostProcessors: 1,
+			memoryLimit:       16 * launchlib.BytesInMebibyte,
+			// 75% of heap - 3mb*processors = 9mb
+			expectedMaxHeapSize: 9 * launchlib.BytesInMebibyte,
+		},
+		{
+			name:              "multiple processors",
+			numHostProcessors: 3,
+			memoryLimit:       120 * launchlib.BytesInMebibyte,
+			// 75% of heap - 3mb*processors = 81mb
+			expectedMaxHeapSize: 81 * launchlib.BytesInMebibyte,
+		},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			heapSizeInBytes := launchlib.ComputeJVMHeapSizeInBytes(tc.numHostProcessors, tc.memoryLimit)
+			assert.Equal(t, heapSizeInBytes, tc.expectedMaxHeapSize)
+		})
+	}
+}
+
 func runMainWithArgs(t *testing.T, staticConfigFile, customConfigFile string, env ...string) (string, error) {
 	jdkDir := "jdk"
 	javaHome, err := filepath.Abs(jdkDir)
@@ -226,18 +287,23 @@ func runMultiProcess(t *testing.T, cmd *exec.Cmd) map[string]int {
 	return children
 }
 
-func testContainerSupportEnabled(t *testing.T, launcherCustom string, expectedJvmArgs string) string {
-	return testInContainer(t, launcherCustom, "Container support enabled", expectedJvmArgs)
+func testContainerSupportEnabled(t *testing.T, launcherCustom string, expectedJvmArgs string, expectedJvmArgKeys []string) string {
+	return testInContainer(t, launcherCustom, "Container support enabled", expectedJvmArgs, expectedJvmArgKeys)
 }
 
-func testInContainer(t *testing.T, launcherCustom string, containerSupportMessage string, jvmArgs string) string {
+func testInContainer(t *testing.T, launcherCustom string, containerSupportMessage string, jvmArgs string, jvmArgKeys []string) string {
 	output, err := runMainWithArgs(t, "testdata/launcher-static.yml", launcherCustom, "CONTAINER=")
 	require.NoError(t, err, "failed: %s", output)
 
 	// part of expected output from launcher
-	assert.Regexp(t, `Argument list to executable binary: \[.+/bin/java `+jvmArgs+` -classpath .+/go-java-launcher/integration_test/testdata Main arg1\]`, output)
+	if jvmArgs != "" {
+		assert.Regexp(t, `Argument list to executable binary: \[.+/bin/java `+jvmArgs+` -classpath .+/go-java-launcher/integration_test/testdata Main arg1\]`, output)
+	}
 	// container support detected and running inside container
 	assert.Regexp(t, containerSupportMessage, output)
+	for _, key := range jvmArgKeys {
+		assert.Regexp(t, key, output)
+	}
 	// expected output of Java program
 	assert.Regexp(t, `\nmain method\n`, output)
 	return output

integration_test/testdata/launcher-custom-experimental-container-v2-with-initial-ram-percentage.yml

@@ -0,0 +1,6 @@
+configType: java
+configVersion: 1
+jvmOpts:
+  - '-XX:InitialRAMPercentage=70.0'
+experimental:
+  containerV2: true

integration_test/testdata/launcher-custom-experimental-container-v2-with-max-ram-percentage.yml

@@ -0,0 +1,6 @@
+configType: java
+configVersion: 1
+jvmOpts:
+  - '-XX:MaxRAMPercentage=70.0'
+experimental:
+  containerV2: true

integration_test/testdata/launcher-custom-experimental-processor-count.yml → integration_test/testdata/launcher-custom-experimental-container-v2-with-xms-and-xmx.yml

@@ -1,6 +1,7 @@
 configType: java
 configVersion: 1
 jvmOpts:
+  - '-Xms1g'
   - '-Xmx1g'
 experimental:
-  overrideActiveProcessorCount: true
+  containerV2: true

integration_test/testdata/launcher-custom-experimental-container-v2.yml

@@ -0,0 +1,5 @@
+configType: java
+configVersion: 1
+jvmOpts: []
+experimental:
+  containerV2: true

launchlib/config.go

@@ -71,7 +71,9 @@ type CustomLauncherConfig struct {
 	DisableContainerSupport bool                       `yaml:"dangerousDisableContainerSupport"`
 }
 
-type ExperimentalLauncherConfig struct{}
+type ExperimentalLauncherConfig struct {
+	ContainerV2 bool `yaml:"containerV2"`
+}
 
 type PrimaryCustomLauncherConfig struct {
 	VersionedConfig      `yaml:",inline"`

launchlib/launcher.go

@@ -21,6 +21,7 @@ import (
 	"os/exec"
 	"path"
 	"regexp"
+	"runtime"
 	"strings"
 
 	"github.com/pkg/errors"
@@ -31,6 +32,7 @@ const (
 	TemplateDelimsClose = "}}"
 	// ExecPathBlackListRegex matches characters disallowed in paths we allow to be passed to exec()
 	ExecPathBlackListRegex = `[^\w.\/_\-]`
+	BytesInMebibyte        = 1048576
 )
 
 type ServiceCmds struct {
@@ -99,7 +101,10 @@ func compileCmdFromConfig(
 		combinedJvmOpts = append(combinedJvmOpts, staticConfig.JavaConfig.JvmOpts...)
 		combinedJvmOpts = append(combinedJvmOpts, customConfig.JvmOpts...)
 
-		jvmOpts := createJvmOpts(combinedJvmOpts, customConfig, logger)
+		jvmOpts, err := createJvmOpts(combinedJvmOpts, customConfig, logger)
+		if err != nil {
+			return nil, err
+		}
 
 		executable, executableErr = verifyPathIsSafeForExec(path.Join(javaHome, "/bin/java"))
 		if executableErr != nil {
@@ -276,12 +281,24 @@ func delim(str string) string {
 	return fmt.Sprintf("%s%s%s", TemplateDelimsOpen, str, TemplateDelimsClose)
 }
 
-func createJvmOpts(combinedJvmOpts []string, customConfig *CustomLauncherConfig, logger io.WriteCloser) []string {
+func createJvmOpts(combinedJvmOpts []string, customConfig *CustomLauncherConfig, logger io.WriteCloser) ([]string, error) {
 	if isEnvVarSet("CONTAINER") && !customConfig.DisableContainerSupport && !hasMaxRAMOverride(combinedJvmOpts) {
 		_, _ = fmt.Fprintln(logger, "Container support enabled")
-		combinedJvmOpts = filterHeapSizeArgs(combinedJvmOpts)
-		combinedJvmOpts = ensureActiveProcessorCount(combinedJvmOpts, logger)
-		return combinedJvmOpts
+		if customConfig.Experimental.ContainerV2 {
+			jvmOptsWithUpdatedHeapSizeArgs, err := filterHeapSizeArgsV2(combinedJvmOpts)
+			if err != nil {
+				// When we fail to get the memory limit from the cgroups files, fallback to using percentage-based heap
+				// sizing. While this method doesn't take into account the per-processor memory offset, it is supported
+				// by all platforms using Java.
+				combinedJvmOpts = filterHeapSizeArgs(combinedJvmOpts)
+			} else {
+				combinedJvmOpts = jvmOptsWithUpdatedHeapSizeArgs
+			}
+		} else {
+			combinedJvmOpts = filterHeapSizeArgs(combinedJvmOpts)
+			combinedJvmOpts = ensureActiveProcessorCount(combinedJvmOpts, logger)
+		}
+		return combinedJvmOpts, nil
 	}
 
 	if isEnvVarSet("CONTAINER") {
@@ -292,7 +309,7 @@ func createJvmOpts(combinedJvmOpts []string, customConfig *CustomLauncherConfig,
 		}
 	}
 
-	return combinedJvmOpts
+	return combinedJvmOpts, nil
 }
 
 func filterHeapSizeArgs(args []string) []string {
@@ -317,6 +334,34 @@ func filterHeapSizeArgs(args []string) []string {
 	return filtered
 }
 
+// Used when the containerV2 flag is set
+func filterHeapSizeArgsV2(args []string) ([]string, error) {
+	var filtered []string
+	var hasMaxRAMPercentage, hasInitialRAMPercentage bool
+	for _, arg := range args {
+		if !isHeapSizeArg(arg) {
+			filtered = append(filtered, arg)
+		}
+
+		if isMaxRAMPercentage(arg) {
+			hasMaxRAMPercentage = true
+		} else if isInitialRAMPercentage(arg) {
+			hasInitialRAMPercentage = true
+		}
+	}
+
+	if !hasInitialRAMPercentage && !hasMaxRAMPercentage {
+		cgroupMemoryLimitInBytes, err := DefaultMemoryLimit.MemoryLimitInBytes()
+		if err != nil {
+			return filtered, errors.Wrap(err, "failed to get cgroup memory limit")
+		}
+		jvmHeapSizeInBytes := ComputeJVMHeapSizeInBytes(runtime.NumCPU(), cgroupMemoryLimitInBytes)
+		filtered = append(filtered, fmt.Sprintf("-Xms%d", jvmHeapSizeInBytes))
+		filtered = append(filtered, fmt.Sprintf("-Xmx%d", jvmHeapSizeInBytes))
+	}
+	return filtered, nil
+}
+
 func ensureActiveProcessorCount(args []string, logger io.Writer) []string {
 	filtered := make([]string, 0, len(args)+1)
 
@@ -376,3 +421,12 @@ func isMaxRAMPercentage(arg string) bool {
 func isInitialRAMPercentage(arg string) bool {
 	return strings.HasPrefix(arg, "-XX:InitialRAMPercentage=")
 }
+
+// ComputeJVMHeapSizeInBytes If the experimental `ContainerV2` is set, compute the heap size to be 75% of
+// the heap minus 3mb per processor, with a minimum value of 50% of the heap.
+func ComputeJVMHeapSizeInBytes(hostProcessorCount int, cgroupMemoryLimitInBytes uint64) uint64 {
+	var memoryLimit = float64(cgroupMemoryLimitInBytes)
+	var processorAdjustment = 3 * BytesInMebibyte * float64(hostProcessorCount)
+	var computedHeapSize = max(0.5*memoryLimit, 0.75*memoryLimit-processorAdjustment)
+	return uint64(computedHeapSize)
+}

launchlib/memory.go

@@ -0,0 +1,71 @@
+// Copyright 2023 Palantir Technologies, Inc.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package launchlib
+
+import (
+	"io"
+	"io/fs"
+	"os"
+	"path/filepath"
+	"strconv"
+	"strings"
+
+	"github.com/pkg/errors"
+)
+
+const (
+	memGroupName = "memory"
+	memLimitName = "memory.limit_in_bytes"
+)
+
+type MemoryLimit interface {
+	MemoryLimitInBytes() (uint64, error)
+}
+
+var DefaultMemoryLimit = NewCGroupMemoryLimit(os.DirFS("/"))
+
+type CGroupMemoryLimit struct {
+	pather CGroupPather
+	fs     fs.FS
+}
+
+func NewCGroupMemoryLimit(filesystem fs.FS) MemoryLimit {
+	return CGroupMemoryLimit{
+		pather: NewCGroupV1Pather(filesystem),
+		fs:     filesystem,
+	}
+}
+
+func (c CGroupMemoryLimit) MemoryLimitInBytes() (uint64, error) {
+	memoryCGroupPath, err := c.pather.Path(memGroupName)
+	if err != nil {
+		return 0, errors.Wrap(err, "failed to get memory cgroup path")
+	}
+
+	memLimitFilepath := filepath.Join(memoryCGroupPath, memLimitName)
+	memLimitFile, err := c.fs.Open(convertToFSPath(memLimitFilepath))
+	if err != nil {
+		return 0, errors.Wrapf(err, "unable to open memory.limit_in_bytes at expected location: %s", memLimitFilepath)
+	}
+	memLimitBytes, err := io.ReadAll(memLimitFile)
+	if err != nil {
+		return 0, errors.Wrapf(err, "unable to read memory.limit_in_bytes")
+	}
+	memLimit, err := strconv.Atoi(strings.TrimSpace(string(memLimitBytes)))
+	if err != nil {
+		return 0, errors.New("unable to convert memory.limit_in_bytes value to expected type")
+	}
+	return uint64(memLimit), nil
+}