Enable build parallelism by default. (deephaven#4048)

jjbrosnan · Jun 22, 2023 · 40af6a3 · 40af6a3
1 parent 178f961
commit 40af6a3
Show file tree

Hide file tree

Showing 14 changed files with 88 additions and 102 deletions.
diff --git a/.github/env/Linux/gradle.properties b/.github/env/Linux/gradle.properties
diff --git a/.github/scripts/gradle-properties.sh b/.github/scripts/gradle-properties.sh
@@ -0,0 +1,50 @@
+#!/usr/bin/env bash
+
+set -o errexit
+set -o pipefail
+set -o nounset
+
+# We know the gradle daemon takes 1GiB (see gradle.properties)
+DAEMON_BYTES=1073741824
+
+# We'll leave some buffer space for other system resources / processes
+OTHER_BYTES=2147483648
+
+TOTAL_SYSTEM_BYTES="$(free --bytes | grep Mem | awk -F " " '{print $2}')"
+
+# This is accounting for "worst case", assuming every single worker is using the theoretical maximum.
+# Currently, engine/table/build.gradle sets a heap size of 6GiB, so that's the maximum.
+PER_WORKER_BYTES=6442450944
+
+# See https://github.com/gradle/gradle/issues/14431#issuecomment-1601724453 for why we need to have this sort of logic
+# here
+MAX_WORKERS="$(( (TOTAL_SYSTEM_BYTES - DAEMON_BYTES - OTHER_BYTES) / PER_WORKER_BYTES ))"
+MAX_WORKERS="$(( MAX_WORKERS > 0 ? MAX_WORKERS : 1 ))"
+
+# A bit zealous, but this handles the https://github.com/actions/setup-java output
+JAVA_INSTALL_PATHS=""
+JAVA_INSTALL_PATHS="${JAVA_INSTALL_PATHS}${JAVA_HOME_8_X64:+$JAVA_HOME_8_X64,}"
+JAVA_INSTALL_PATHS="${JAVA_INSTALL_PATHS}${JAVA_HOME_9_X64:+$JAVA_HOME_9_X64,}"
+JAVA_INSTALL_PATHS="${JAVA_INSTALL_PATHS}${JAVA_HOME_10_X64:+$JAVA_HOME_10_X64,}"
+JAVA_INSTALL_PATHS="${JAVA_INSTALL_PATHS}${JAVA_HOME_11_X64:+$JAVA_HOME_11_X64,}"
+JAVA_INSTALL_PATHS="${JAVA_INSTALL_PATHS}${JAVA_HOME_12_X64:+$JAVA_HOME_12_X64,}"
+JAVA_INSTALL_PATHS="${JAVA_INSTALL_PATHS}${JAVA_HOME_13_X64:+$JAVA_HOME_13_X64,}"
+JAVA_INSTALL_PATHS="${JAVA_INSTALL_PATHS}${JAVA_HOME_14_X64:+$JAVA_HOME_14_X64,}"
+JAVA_INSTALL_PATHS="${JAVA_INSTALL_PATHS}${JAVA_HOME_15_X64:+$JAVA_HOME_15_X64,}"
+JAVA_INSTALL_PATHS="${JAVA_INSTALL_PATHS}${JAVA_HOME_16_X64:+$JAVA_HOME_16_X64,}"
+JAVA_INSTALL_PATHS="${JAVA_INSTALL_PATHS}${JAVA_HOME_17_X64:+$JAVA_HOME_17_X64,}"
+JAVA_INSTALL_PATHS="${JAVA_INSTALL_PATHS}${JAVA_HOME_18_X64:+$JAVA_HOME_18_X64,}"
+JAVA_INSTALL_PATHS="${JAVA_INSTALL_PATHS}${JAVA_HOME_19_X64:+$JAVA_HOME_19_X64,}"
+JAVA_INSTALL_PATHS="${JAVA_INSTALL_PATHS}${JAVA_HOME_20_X64:+$JAVA_HOME_20_X64,}"
+JAVA_INSTALL_PATHS="${JAVA_INSTALL_PATHS}${JAVA_HOME_21_X64:+$JAVA_HOME_21_X64,}"
+JAVA_INSTALL_PATHS="${JAVA_INSTALL_PATHS}${JAVA_HOME_22_X64:+$JAVA_HOME_22_X64,}"
+JAVA_INSTALL_PATHS="${JAVA_INSTALL_PATHS}${JAVA_HOME_23_X64:+$JAVA_HOME_23_X64,}"
+
+# Our CI JDKs should be pre-provisioned and invoked correctly,
+# we shouldn't rely on gradle for any of this logic.
+cat << EOF
+org.gradle.java.installations.auto-download=false
+org.gradle.java.installations.auto-detect=false
+org.gradle.workers.max=${MAX_WORKERS}
+org.gradle.java.installations.paths=${JAVA_INSTALL_PATHS}
+EOF
diff --git a/.github/workflows/build-ci.yml b/.github/workflows/build-ci.yml
@@ -30,9 +30,7 @@ jobs:
 
       - name: Setup gradle properties
         run: |
-          cat .github/env/${{ runner.os }}/gradle.properties >> gradle.properties
-          echo >> gradle.properties
-          echo "org.gradle.java.installations.paths=${{ steps.setup-java-11.outputs.path }},${{ steps.setup-java-17.outputs.path }}," >> gradle.properties
+          .github/scripts/gradle-properties.sh >> gradle.properties
           cat gradle.properties
 
       - name: Get Semver
@@ -161,9 +159,7 @@ jobs:
 
       - name: Setup gradle properties
         run: |
-          cat .github/env/${{ runner.os }}/gradle.properties >> gradle.properties
-          echo >> gradle.properties
-          echo "org.gradle.java.installations.paths=${{ steps.setup-java-11.outputs.path }}" >> gradle.properties
+          .github/scripts/gradle-properties.sh >> gradle.properties
           cat gradle.properties
 
       - name: Set up Docker Buildx

diff --git a/.github/workflows/check-ci.yml b/.github/workflows/check-ci.yml
@@ -32,9 +32,7 @@ jobs:
 
       - name: Setup gradle properties
         run: |
-          cat .github/env/${{ runner.os }}/gradle.properties >> gradle.properties
-          echo >> gradle.properties
-          echo "org.gradle.java.installations.paths=${{ steps.setup-java-11.outputs.path }},${{ steps.setup-java-17.outputs.path }}," >> gradle.properties
+          .github/scripts/gradle-properties.sh >> gradle.properties
           cat gradle.properties
 
       - name: Check

diff --git a/.github/workflows/docs-ci.yml b/.github/workflows/docs-ci.yml
@@ -32,9 +32,7 @@ jobs:
 
       - name: Setup gradle properties
         run: |
-          cat .github/env/${{ runner.os }}/gradle.properties >> gradle.properties
-          echo >> gradle.properties
-          echo "org.gradle.java.installations.paths=${{ steps.setup-java-11.outputs.path }},${{ steps.setup-java-17.outputs.path }}" >> gradle.properties
+          .github/scripts/gradle-properties.sh >> gradle.properties
           cat gradle.properties
 
       - name: All Javadoc
@@ -88,9 +86,7 @@ jobs:
 
       - name: Setup gradle properties
         run: |
-          cat .github/env/${{ runner.os }}/gradle.properties >> gradle.properties
-          echo >> gradle.properties
-          echo "org.gradle.java.installations.paths=${{ steps.setup-java-11.outputs.path }},${{ steps.setup-java-17.outputs.path }}" >> gradle.properties
+          .github/scripts/gradle-properties.sh >> gradle.properties
           cat gradle.properties
 
       - name: Generate Python Docs
@@ -150,9 +146,7 @@ jobs:
 
       - name: Setup gradle properties
         run: |
-          cat .github/env/${{ runner.os }}/gradle.properties >> gradle.properties
-          echo >> gradle.properties
-          echo "org.gradle.java.installations.paths=${{ steps.setup-java-11.outputs.path }}" >> gradle.properties
+          .github/scripts/gradle-properties.sh >> gradle.properties
           cat gradle.properties
 
       - name: Generate C++ Docs

diff --git a/.github/workflows/nightly-check-ci.yml b/.github/workflows/nightly-check-ci.yml
@@ -48,9 +48,7 @@ jobs:
 
       - name: Setup gradle properties
         run: |
-          cat .github/env/${{ runner.os }}/gradle.properties >> gradle.properties
-          echo >> gradle.properties
-          echo "org.gradle.java.installations.paths=${{ steps.setup-java-11.outputs.path }},${{ steps.setup-java-17.outputs.path }},${{ steps.setup-java-18.outputs.path }}," >> gradle.properties
+          .github/scripts/gradle-properties.sh >> gradle.properties
           cat gradle.properties
 
       - name: Run gradle ${{ matrix.gradle-task }} on java ${{ matrix.test-jvm-version }}

diff --git a/.github/workflows/nightly-image-check.yml b/.github/workflows/nightly-image-check.yml
@@ -24,9 +24,7 @@ jobs:
 
       - name: Setup gradle properties
         run: |
-          cat .github/env/${{ runner.os }}/gradle.properties >> gradle.properties
-          echo >> gradle.properties
-          echo "org.gradle.java.installations.paths=${{ steps.setup-java-11.outputs.path }}" >> gradle.properties
+          .github/scripts/gradle-properties.sh >> gradle.properties
           cat gradle.properties
 
       - name: Run gradle

diff --git a/.github/workflows/publish-ci.yml b/.github/workflows/publish-ci.yml
@@ -32,9 +32,7 @@ jobs:
 
       - name: Setup gradle properties
         run: |
-          cat .github/env/${{ runner.os }}/gradle.properties >> gradle.properties
-          echo >> gradle.properties
-          echo "org.gradle.java.installations.paths=${{ steps.setup-java-11.outputs.path }},${{ steps.setup-java-17.outputs.path }}," >> gradle.properties
+          .github/scripts/gradle-properties.sh >> gradle.properties
           cat gradle.properties
 
       # TODO(deephaven-core#2614): Improve gradle/CI assemble and publishing of artifacts
@@ -52,8 +50,7 @@ jobs:
         uses: burrunan/gradle-cache-action@v1
         with:
           job-id: publish
-          # Note: even though we specify org.gradle.parallel=false in our CI gradle.properties, we want to be explicit
-          # here about no parallelism to ensure we don't create disjointed staging repositories.
+          # We need to be explicit here about no parallelism to ensure we don't create disjointed staging repositories.
           arguments: --no-parallel server-netty-app:build server-jetty-app:build py-server:build py-embedded-server:build publish
           gradle-version: wrapper
         env:

diff --git a/.github/workflows/quick-ci.yml b/.github/workflows/quick-ci.yml
@@ -28,9 +28,7 @@ jobs:
 
       - name: Setup gradle properties
         run: |
-          cat .github/env/${{ runner.os }}/gradle.properties >> gradle.properties
-          echo >> gradle.properties
-          echo "org.gradle.java.installations.paths=${{ steps.setup-java-11.outputs.path }}" >> gradle.properties
+          .github/scripts/gradle-properties.sh >> gradle.properties
           cat gradle.properties
 
       - name: Quick Task

diff --git a/.github/workflows/tag-base-images.yml b/.github/workflows/tag-base-images.yml
@@ -34,9 +34,7 @@ jobs:
 
       - name: Setup gradle properties
         run: |
-          cat .github/env/${{ runner.os }}/gradle.properties >> gradle.properties
-          echo >> gradle.properties
-          echo "org.gradle.java.installations.paths=${{ steps.setup-java-11.outputs.path }},${{ steps.setup-java-17.outputs.path }}," >> gradle.properties
+          .github/scripts/gradle-properties.sh >> gradle.properties
           cat gradle.properties
 
       - name: Login to ghcr.io

diff --git a/buildSrc/src/main/groovy/TestTools.groovy b/buildSrc/src/main/groovy/TestTools.groovy
@@ -1,6 +1,4 @@
-import org.gradle.api.JavaVersion
 import org.gradle.api.Project
-import org.gradle.api.Task
 import org.gradle.api.artifacts.Dependency
 import org.gradle.api.plugins.JavaPluginConvention
 import org.gradle.api.reporting.Report
@@ -26,37 +24,24 @@ class TestTools {
 
     static final String TEST_GROUP = "~Deephaven Test"
 
-    static boolean allowParallel(Project project) {
-        return Boolean.parseBoolean(project.findProperty('allowParallel') as String ?: 'false')
-    }
-
     static Test addEngineSerialTest(Project project) {
-        def allowParallel = allowParallel(project)
-        // testSerial: non-parallel, not a engine test, isolated
-        return TestTools.addEngineTest(project, 'Serial', allowParallel, false, true)
+        return addEngineTest(project, 'Serial', false, true)
     }
 
     static Test addEngineParallelTest(Project project) {
-        def allowParallel = allowParallel(project)
         // Add @Category(ParallelTest.class) to have your tests run in parallel
         // Note: Supports JUnit4 or greater only (you use @Test annotations to mark test methods).
-
-        // The Parallel tests are now by default functionally equivalent to the Serial test logic
-        // TODO (deephaven-core#643): Fix "leaking" parallel tests
-        return TestTools.addEngineTest(project, 'Parallel', allowParallel, false, !allowParallel)
+        return addEngineTest(project, 'Parallel', true, false)
     }
 
     static Test addEngineOutOfBandTest(Project project) {
-        def allowParallel = allowParallel(project)
-        // testOutOfBand: non-parallel, not a engine test, not isolated
-        return TestTools.addEngineTest(project, 'OutOfBand', allowParallel, false, false)
+        return addEngineTest(project, 'OutOfBand', true, false)
     }
 
-    static Test addEngineTest(
+    private static Test addEngineTest(
         Project project,
         String type,
         boolean parallel = false,
-        boolean passwordEnabled = false,
         boolean isolated = false
     ) {
         Test mainTest = project.tasks.getByName('test') as Test
@@ -84,11 +69,10 @@ By default only runs in CI; to run locally:
             dependsOn project.tasks.findByName('testClasses')
 
             if (parallel) {
-                if (project.hasProperty('maxParallelForks')) {
-                    maxParallelForks = project.property('maxParallelForks') as int
-                } else {
-                    maxParallelForks = 12
-                }
+                // We essentially want to set maxParallelForks for all "normal" tests. It will be capped by the number
+                // of gradle workers, org.gradle.workers.max. We aren't able to set set this property for all Test types,
+                // because testSerial needs special handling.
+                maxParallelForks = Runtime.runtime.availableProcessors()
                 if (project.hasProperty('forkEvery')) {
                     forkEvery = project.property('forkEvery') as int
                 }
@@ -124,24 +108,6 @@ By default only runs in CI; to run locally:
             SourceSetContainer sources = project.convention.getPlugin(JavaPluginConvention).sourceSets
             setClasspath project.files(sources.getByName('test').output, sources.getByName('main').output, project.configurations.getByName(TEST_RUNTIME_CLASSPATH_CONFIGURATION_NAME))
 
-            if (passwordEnabled) {
-                def host = project.findProperty("${type.toLowerCase()}.test.host") ?: '0.0.0.0'
-                def password = project.findProperty("${type.toLowerCase()}.test.password")
-                t.systemProperty("${type.toLowerCase()}.test.host", host)
-                t.systemProperty("${type.toLowerCase()}.test.password", password)
-
-                if (password) {
-                    // When running with a valid password, make sure task `check` (and, by dependence, `build`) depends on us.
-                    project.tasks.getByName('check').dependsOn(t)
-                } else {
-                    // no password?  skip these tasks.
-                    t.onlyIf { false }
-                }
-                // This is necessary for the mysql/sqlserver tests, which must all be run in a suite.
-                // Do not cargo-cult this when extending @Category support for other test types.
-                t.include("**/*Suite.class")
-            }
-
             // we also need to adjust the reporting output directory of the alt task,
             // so we don't stomp over top of previous reports.
             reports.all {

diff --git a/buildSrc/src/main/groovy/io.deephaven.java-test-conventions.gradle b/buildSrc/src/main/groovy/io.deephaven.java-test-conventions.gradle
@@ -23,7 +23,16 @@ artifacts {
     archives testJar
 }
 
-project.tasks.withType(Test).all { Test t ->
+// This only applies to the standard "test" task
+project.tasks.named('test', Test, { test ->
+    // We essentially want to set maxParallelForks for all "normal" tests. It will be capped by the number
+    // of gradle workers, org.gradle.workers.max. We aren't able to set set this property for all Test types,
+    // because testSerial needs special handling.
+    test.maxParallelForks = Runtime.runtime.availableProcessors()
+})
+
+// This applies to all test types, including the testOutOfBand, testSerial, and testParallel
+project.tasks.withType(Test).configureEach { Test t ->
     t.with {
         t.defaultCharacterEncoding = 'UTF-8'
 
@@ -45,10 +54,6 @@ project.tasks.withType(Test).all { Test t ->
             maxHeapSize = '3g'
         }
 
-        if (!maxParallelForks) {
-            maxParallelForks = 4
-        }
-
         if (findProperty('shortTests') == 'true') {
             systemProperty 'TstUtils.shortTests', 'true'
         }

diff --git a/engine/table/build.gradle b/engine/table/build.gradle
@@ -128,8 +128,6 @@ spotless {
 }
 
 test {
-    maxParallelForks = Integer.parseInt(findProperty('maxParallelForks') as String ?: '1')
-    forkEvery = Integer.parseInt(findProperty('forkEvery') as String ?: '1')
     //  For now, if you apply @Category(ParallelTest.class) to tests which are not huge CPU/RAM hogs, you can get parallelism
     //  If you have CPU/RAM-heavy tasks that you don't want gumming up :engine-table:test runs, apply @Category(SerialTest.class) instead
     //  (note that the above only works for junit 4 tests; see the documentation on SerialTest class and others for porting instructions)
@@ -147,6 +145,7 @@ TestTools.addEngineParallelTest(project)
 TestTools.addEngineSerialTest(project)
 TestTools.addEngineOutOfBandTest(project)
 
+// If changing, be sure to update .github/scripts/print-gradle-workers-max.sh
 def maxHeapSize = findProperty('maxHeapSize') as String ?: '6g'
 
 tasks.testParallel.maxHeapSize = maxHeapSize

diff --git a/gradle.properties b/gradle.properties
@@ -15,6 +15,13 @@
 # The gradle workers often have their own methods to control additional heap.
 # For example, compilation and java tests are able to set their own memory usage needs through their own configurations.
 org.gradle.jvmargs=-Xmx1g
+org.gradle.parallel=true
+
+# If you are running the full test suite locally, or running gradle builds on infrastructure that has a lot of CPUs but
+# is memory-limited, you may need to explicitly tune your max workers.
+# See .github/scripts/gradle-properties.sh for the logic we use there.
+# See https://github.com/gradle/gradle/issues/14431#issuecomment-1601724453.
+#org.gradle.workers.max=...
 
 # Setting debugCI to true will cause failed builds to keepalive for three hours so we can do post-mortem inspection of jenkins workspace
 debugCI=false