Repository navigation
[WIP] Improve build times - Part 1 - #7482
Open
alextwoods wants to merge 7 commits into
Open
alextwoods wants to merge 7 commits into
alextwoods wants to merge 7 commits into
Conversation
…process discovery does not need JVM flags BlockHound's BlockHoundTestExecutionListener installs BlockHound in its static initializer. With forkCount > 1, surefire creates a JUnit Platform launcher in the Maven JVM for test discovery, which loaded the listener and installed BlockHound into the Maven JVM. On JDK 13+ that fails unless the Maven JVM runs with -XX:+AllowRedefinitionToAddDeleteMethods. Replace it with an SDK listener in service-test-utils that installs BlockHound in testPlanExecutionStarted, which only runs in the JVMs that execute tests.
The dev profile keeps checkstyle and unit tests, skips spotbugs, dependency analysis, japicmp, javadoc and jacoco (the same properties the quick profile uses, plus jacoco.skip), and runs test classes in parallel forks (forkCount=1C, reuseForks=true). The default build is unchanged.
isNotReferenced compiled a regex per import and scanned the whole file, and isDuplicate did the same for every import. Build the set of \b\w+\b words once per file and check identifier symbols against it, and only try the duplicate regex at positions where "import" occurs. Both give the same answer as before on every JDK, so generated code does not change. UnusedImportRemoverEquivalenceTest checks this against a copy of the previous implementation.
…reams TCK on the JUnit Platform Replace the two surefire providers (surefire-junit-platform and surefire-testng) with the auto-detected JUnit Platform provider plus org.junit.support:testng-engine 1.1.0 as a global test dependency. The TCK tests now run in the same test run and forks as the JUnit tests, each TCK class gets its own report, and modules without TCK tests no longer start an empty TestNG fork. The junit=false provider property was only used by surefire-testng and is removed. 3.6.0 is not used: its console capture fails when a dynamically attached agent (BlockHound, Mockito inline) writes to stderr from the Attach Listener thread (SUREFIRE-3460), so s3 and sqs could not run tests. maven-failsafe-plugin is versioned separately and unchanged.
Add docs/guidelines/building-and-testing.md, the single source for build, test and verification commands per scenario, with rules, agent practices and gotchas. Include it in every agent session through the new always-included .kiro/steering/building-and-testing.md. Replace the inline Build Instructions in .kiro/steering/aws-sdk-java-v2-general.md with a pointer, index the new guideline, and point CONTRIBUTING.md, docs/GettingStarted.md and README.md at it. Replace the dead -Dfindbugs.skip flag with -Dspotbugs.skip and the non-standard --am with -am.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation and Context
Verifying a change to a single module (checkstyle plus that module's unit tests) with the default build takes 25 s
to 8 min. Measured with
mvn clean verify -pl :<module>(macOS, 14 cores, JDK 21, mean of 2 runs): utils 106 s,sdk-core 161 s, s3 224 s, codegen-generated-classes-test 480 s. About 85% of that time is surefire. Surefire runs
with one fork, so test classes run one after another, followed by a second, separate TestNG provider run for the
reactive-streams TCK tests.
This PR is the first in a series that aims to make single-module verification about 10x faster. It adds an opt-in
way to run checkstyle and every unit test of a module with the test classes spread across parallel test JVMs. It
also removes two sources of per-build overhead that every build pays, CI included. CI keeps using the default build
and still runs every check and every test. Follow-up PRs fix the slow test classes that set each module's time once
the forks are parallel.
Modifications
1. Opt-in
devprofile (rootpom.xml)The profile only sets properties:
-P devforkCount=1C,reuseForks=true(one reused JVM per core; classes run in parallel)dependency:analyze, jacocoWhy it's faster. The forks run test classes in parallel. Each module's time drops from the sum of its test
classes to roughly its slowest class plus overhead. The skipped plugins cost 1 to 15 s per module and are about 25% of
the time for small modules such as aws-core and auth. The property names are the same ones the existing
quickprofile already uses.
Why coverage doesn't drop.
-P dev,help:evaluatereportsforkCount,reuseForksandjacoco.skipas undefined.-P devstill runs checkstyle and every unit test. The skipped checks still run in CI and in the default buildthat contributors run before opening a PR.
not enabled: turning it on globally broke 34 tests in utils.
2. BlockHound is installed by an SDK
TestExecutionListenerProblem. With
forkCount > 1, surefire creates a JUnit Platform launcher in the Maven JVM to discover testclasses before it starts the forks. Reactor's
blockhound-junit-platformlistener installs BlockHound in its staticinitializer, so discovery installed BlockHound into the Maven JVM. On JDK 13+ that fails unless Maven itself runs with
-XX:+AllowRedefinitionToAddDeleteMethods(reactor/BlockHound#33).Change.
BlockHoundInstallingTestExecutionListener(intest/service-test-utils, registered throughMETA-INF/services) callsBlockHound.install()intestPlanExecutionStarted. Discovery never executes a test plan,so the listener installs BlockHound only in the JVMs that run tests, before the first test. Those JVMs already get the
flag from the existing
jdk-13-plusargLine.services/pom.xml,test/stability-tests/pom.xmlandservice-test-utilsno longer depend onblockhound-junit-platform.Why not put the flag in
.mvn/jvm.configinstead.-XXflag they don't recognize.Why coverage doesn't drop. This also changes default and CI builds. The same modules still get BlockHound, and it
is installed before any test runs.
BlockHoundInstalledTestpasses, and the s3 test counts are identical to before.3. maven-surefire-plugin 3.1.2 → 3.5.6, one JUnit Platform provider
Change.
surefire-junit-platformandsurefire-testng), thejunit=falseproperty that onlysurefire-testngread, and sdk-core's redeclaration of the provider.org.junit.support:testng-engine1.1.0, a global test dependency next toreactive-streams-tck, runs the TestNGTCK classes on the JUnit Platform.
Why it's faster.
sequential second pass.
-P devthis takes 7% to 14% off utils (44.2 → 38.0 s), auth (23.6 → 20.5 s), aws-core (19.8 → 17.6 s),sdk-core (37.0 → 33.2 s) and codegen (24.8 → 23.1 s).
Why coverage doesn't drop.
auth, codegen, s3, sqs, codegen-generated-classes-test and netty-nio-client. The only exception is codegen's new
test in section 4.
<includes>and<excludes>are unchanged. testng-engine only claims classes with TestNG annotations, so no JUnittest runs under TestNG and no test runs twice in the default build.
difference:
tests="0". The console output shows those tests ran before too.Reporting changes to be aware of.
@DisplayName, the XMLclassnameis now the display name.@Nestedresults are written into the outer class'sTEST-*.xml.classnameto source files (Sonar, CodeBuild report groups) willshow different names for those classes.
Why 3.5.6 and not 3.6.0. 3.6.0 hits
SUREFIRE-3460. Its console capture fails when an agent attached
at runtime (BlockHound, Mockito inline) writes to stderr, so s3 and sqs could not run tests. maven-failsafe-plugin is
versioned separately and is unchanged.
4.
UnusedImportRemoverscans each generated file once (codegen)Problem. JFR on s3 code generation put 79% of CPU samples in
UnusedImportRemover. It compiled a regex perimport and scanned the whole file twice for each one.
Change. The class tokenizes each file once and checks every import against that token set. Anything it doesn't
handle falls back to the original regex logic. The commit includes a changelog entry.
Why it's faster. ec2
codegen:generatewent from 42.5 s to 7.3 s (5.8x, mean of 2). This speeds up every buildthat generates a large service, including CI. s3 is unchanged at about 14.6 s: it is bound by the global
JavaCodeFormatterlock instead, which is a follow-up.Why coverage doesn't drop.
Generated sources are byte-identical between the old and new generator (
diff -rof generated sources, generatedtest sources and generated resources):
.javafilesThe only differences were key order in 17 protocol-tests
*-intermediate.jsonfiles. Jackson writes those files,not
UnusedImportRemover, the parsed JSON is equal, and two runs of the old generator differ the same way.UnusedImportRemoverEquivalenceTest(583 cases) compares the new class against a verbatim copy of the old one. Itcovers static and wildcard imports, names in comments, strings and javadoc
{@link}, qualified names, duplicates,and the same simple name in different packages. It passes on JDK 11, 17 and 21. That covers both the JDK 8–18 and
the JDK 19+ behavior of
\bin regexes.5. Documentation
CONTRIBUTING.mdanddocs/GettingStarted.mddocument the dev loop:Results
Default build =
mvn clean verify -pl :<m>on the base commit.-P dev=mvn verify -pl :<m> -P devwithoutclean, after one earlier build. Both are the mean of 2 runs on macOS, 14 cores, JDK 21, with no other buildsrunning.
-P dev(1) One run. The other failed on the known flaky
AsyncRetrySetupTesttimeout.These gains don't reach 10x because, once forks are parallel, each module's time is set by its slowest test classes,
for example
CachedSupplierTest(28 s) andSync/AsyncRetrySetupTest(about 120 s). The next PR fixes those, and italso speeds up the default build.
Default builds take the same time as before, apart from the removed empty TestNG forks and faster codegen for large
services: utils 104.8 s, sdk-core 153.2 s, codegen 35.4 s, s3 220.2 s.
Testing
Default build (
mvn clean install -pl :<m>, every check: checkstyle, spotbugs, javadoc, japicmp, dependencyanalysis, jacoco) passes for utils, sdk-core, codegen, service-test-utils and s3.
Test-count parity against 3.1.2, per class, for 9 modules (section 3), and over the whole repo with
mvn -T 1C clean verify(checks skipped,-Dmaven.test.failure.ignore=true).Codegen equivalence for the modules in section 4. The ec2 output generated on JDK 17 also matches the JDK 21
output.
Other JDKs:
-P devon JDK 11 and 17 for utils and sdk-core gives the same counts. Fork mode works on JDK 11,17 and 21 without any Maven JVM flag.
Behavior checks:
-Dtest=SimplePublisherTckTestselects a single TCK class, and-Dtest=CachedSupplierTesta single JUnit class.-P quickand-DskipTestsstill skip tests.-DforkCount=1Cwithout-P devworks for sqs (service-test-utils,sqs) without any Maven JVM flag.Not verified locally:
flag before JDK 13.
These need a green CI run.
Known issues that exist without this change:
ServerConnectivityErrorMessageTest.closeTimeHasCorrectMessage(netty-nio-client) fails intermittently, withand without forks.
AppIdResolutionTestandAppIdUserAgentTestfail whenAWS_SDK_UA_APP_IDis set in the environment.-P dev), 16@Nestedtests in codegen-generated-classes-test run twice. The root pom's<excludes>replaces surefire's default**/*$*. Default builds are not affected. This adds runs; it doesn'tremove any.
PR.
Types of changes
Checklist
mvn installsucceedsscripts/new-changescript and following the instructions. Commit the new file created by the script in.changes/next-releasewith your changes.License