Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable build parallelism by default. #4048

Merged
merged 3 commits into from
Jun 22, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 4 additions & 14 deletions .github/env/Linux/gradle.properties
Original file line number Diff line number Diff line change
@@ -1,18 +1,8 @@
# TODO (deephaven-core#639): Customize gradle settings per-CI job
#
# https://docs.github.com/en/actions/using-github-hosted-runners/about-github-hosted-runners#supported-runners-and-hardware-resources
# GitHub hosted linux runner environment has 2 CPUs and 7G RAM.
#
# testParallel seems to run poorly on GH Actions, and is unable to take advantage it seems of 2
# workers.
#
# Our engine-table tests currently request 6g of heap (engine/table/build.gradle); this means we can't have more than
# 1 worker at a time.

org.gradle.parallel=false
org.gradle.workers.max=1

# Our CI JDKs should be pre-provisioned and invoked correctly,
# we shouldn't rely on gradle for any of this logic.
org.gradle.java.installations.auto-download=false
org.gradle.java.installations.auto-detect=false

# This will be set on a per environment basis by .github/scripts/print-gradle-workers-max.sh
# See https://github.com/gradle/gradle/issues/14431#issuecomment-1601724453.
#org.gradle.workers.max=...
22 changes: 22 additions & 0 deletions .github/scripts/print-gradle-workers-max.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
#!/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}')"
rcaudy marked this conversation as resolved.
Show resolved Hide resolved

# 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

MAX_WORKERS="$(( (TOTAL_SYSTEM_BYTES - DAEMON_BYTES - OTHER_BYTES) / PER_WORKER_BYTES ))"
jcferretti marked this conversation as resolved.
Show resolved Hide resolved
MAX_WORKERS="$(( MAX_WORKERS > 0 ? MAX_WORKERS : 1 ))"

echo "org.gradle.workers.max=${MAX_WORKERS}"
4 changes: 4 additions & 0 deletions .github/workflows/build-ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@ jobs:
run: |
cat .github/env/${{ runner.os }}/gradle.properties >> gradle.properties
echo >> gradle.properties
.github/scripts/print-gradle-workers-max.sh >> gradle.properties
echo >> gradle.properties
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The block below is almost the same. Too hard to factor the common parts out?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've taken your advice and consolidated it down to a single script.

echo "org.gradle.java.installations.paths=${{ steps.setup-java-11.outputs.path }},${{ steps.setup-java-17.outputs.path }}," >> gradle.properties
cat gradle.properties

Expand Down Expand Up @@ -163,6 +165,8 @@ jobs:
run: |
cat .github/env/${{ runner.os }}/gradle.properties >> gradle.properties
echo >> gradle.properties
.github/scripts/print-gradle-workers-max.sh >> gradle.properties
echo >> gradle.properties
echo "org.gradle.java.installations.paths=${{ steps.setup-java-11.outputs.path }}" >> gradle.properties
cat gradle.properties

Expand Down
2 changes: 2 additions & 0 deletions .github/workflows/check-ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@ jobs:
run: |
cat .github/env/${{ runner.os }}/gradle.properties >> gradle.properties
echo >> gradle.properties
.github/scripts/print-gradle-workers-max.sh >> 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
cat gradle.properties

Expand Down
6 changes: 6 additions & 0 deletions .github/workflows/docs-ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@ jobs:
run: |
cat .github/env/${{ runner.os }}/gradle.properties >> gradle.properties
echo >> gradle.properties
.github/scripts/print-gradle-workers-max.sh >> 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
cat gradle.properties

Expand Down Expand Up @@ -90,6 +92,8 @@ jobs:
run: |
cat .github/env/${{ runner.os }}/gradle.properties >> gradle.properties
echo >> gradle.properties
.github/scripts/print-gradle-workers-max.sh >> 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
cat gradle.properties

Expand Down Expand Up @@ -152,6 +156,8 @@ jobs:
run: |
cat .github/env/${{ runner.os }}/gradle.properties >> gradle.properties
echo >> gradle.properties
.github/scripts/print-gradle-workers-max.sh >> gradle.properties
echo >> gradle.properties
echo "org.gradle.java.installations.paths=${{ steps.setup-java-11.outputs.path }}" >> gradle.properties
cat gradle.properties

Expand Down
2 changes: 2 additions & 0 deletions .github/workflows/nightly-check-ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,8 @@ jobs:
run: |
cat .github/env/${{ runner.os }}/gradle.properties >> gradle.properties
echo >> gradle.properties
.github/scripts/print-gradle-workers-max.sh >> 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
cat gradle.properties

Expand Down
2 changes: 2 additions & 0 deletions .github/workflows/nightly-image-check.yml
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ jobs:
run: |
cat .github/env/${{ runner.os }}/gradle.properties >> gradle.properties
echo >> gradle.properties
.github/scripts/print-gradle-workers-max.sh >> gradle.properties
echo >> gradle.properties
echo "org.gradle.java.installations.paths=${{ steps.setup-java-11.outputs.path }}" >> gradle.properties
cat gradle.properties

Expand Down
5 changes: 3 additions & 2 deletions .github/workflows/publish-ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@ jobs:
run: |
cat .github/env/${{ runner.os }}/gradle.properties >> gradle.properties
echo >> gradle.properties
.github/scripts/print-gradle-workers-max.sh >> 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
cat gradle.properties

Expand All @@ -52,8 +54,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:
Expand Down
2 changes: 2 additions & 0 deletions .github/workflows/quick-ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,8 @@ jobs:
run: |
cat .github/env/${{ runner.os }}/gradle.properties >> gradle.properties
echo >> gradle.properties
.github/scripts/print-gradle-workers-max.sh >> gradle.properties
echo >> gradle.properties
echo "org.gradle.java.installations.paths=${{ steps.setup-java-11.outputs.path }}" >> gradle.properties
cat gradle.properties

Expand Down
2 changes: 2 additions & 0 deletions .github/workflows/tag-base-images.yml
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,8 @@ jobs:
run: |
cat .github/env/${{ runner.os }}/gradle.properties >> gradle.properties
echo >> gradle.properties
.github/scripts/print-gradle-workers-max.sh >> 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
cat gradle.properties

Expand Down
50 changes: 8 additions & 42 deletions buildSrc/src/main/groovy/TestTools.groovy
Original file line number Diff line number Diff line change
@@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -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 {
Expand Down
15 changes: 10 additions & 5 deletions buildSrc/src/main/groovy/io.deephaven.java-test-conventions.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -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'

Expand All @@ -45,10 +54,6 @@ project.tasks.withType(Test).all { Test t ->
maxHeapSize = '3g'
}

if (!maxParallelForks) {
maxParallelForks = 4
}
Comment on lines -48 to -50
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This logic never worked; the inner code is never executed.


if (findProperty('shortTests') == 'true') {
systemProperty 'TstUtils.shortTests', 'true'
}
Expand Down
3 changes: 1 addition & 2 deletions engine/table/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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
Expand Down
7 changes: 7 additions & 0 deletions gradle.properties
Original file line number Diff line number Diff line change
Expand Up @@ -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/print-gradle-workers-max.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
Expand Down
Loading