Cut the longest CI job from about seven minutes to about two - #217
Open
MarkusPaulsen wants to merge 1 commit into
Open
Cut the longest CI job from about seven minutes to about two#217MarkusPaulsen wants to merge 1 commit into
MarkusPaulsen wants to merge 1 commit into
Conversation
unit-architecture-tests was the only test profile that did not override surefire-reuse-forks, so it inherited the false the coverage profile sets and started a fresh JVM for each of its 19 classes. Measured in CI: 404 s in the surefire plugin against 105 s of actual test time, the difference being roughly 15.5 s of JVM start per fork. That WALA state survives a shared fork is not assumed. The integration matrix has run the WALA modes with -Dsurefire-reuse-forks=true all along, and this was verified here: 19 classes and 188 tests green, twice. One long-lived fork needs a stated heap ceiling rather than the 6g default, since that default exists for the opposite case. At 6g the reused fork peaked at 3.12 and 3.95 GB across two runs; at 3g, three runs peaked at 1.76, 2.20 and 1.83 GB with no change in runtime. The heap becomes a property so a profile can set it without duplicating the agent and bootclasspath arguments of the whole argLine.
Contributor
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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.
Summary
The architecture unit tests started a fresh JVM for each of their 19 classes, which cost about 15 seconds per class against 105 seconds of actual testing. They now share one fork, as the other two unit-test profiles already do, under a stated heap ceiling.
Linked issues
No linked issues
1. Problem
unit-architecture-testswas the only test profile that did not overridesurefire-reuse-forks. It therefore inherited thefalsethat thecoverageprofile sets, and Surefire started a new JVM for every test class.Measured from a CI log: the
surefire:testexecution ran 404 seconds while the 19 classes together report 105 seconds of test time, and the gaps between classes are about 15.5 seconds each. That is the JVM start, the agent attachment and the class loading, paid nineteen times.Nothing is broken. This is the slowest job of the workflow and the reason the pipeline takes about eleven and a half minutes.
2. Improvement from the user's perspective
No Improvement from the user's perspective
3. Improvement from the maintainer's perspective
The step should drop from about 407 to about 130 seconds, and with it the critical path of the whole workflow from roughly eleven and a half to roughly seven minutes. A maintainer waits less for a pull request, and the shared runner pool is occupied for less time.
The heap becomes a property so that a profile can change it without repeating the agent and bootclasspath arguments of the whole
argLine.4. Testing manual
Prerequisites
Steps
Not reproducible from an exercise. This changes the build only.
mvn test -Punit-architecture-tests,coverageand note the number of classes, the number of tests and the wall-clock time.mainand compare.Expected result
Step 1 reports 19 classes and 188 tests, all passing, in roughly one minute. Step 2 reports the same 19 classes and 188 tests in roughly four and a half minutes. Step 3 shows a step duration well below the 407 seconds on
main.Eight runs were made while preparing this change, five of them at the new ceiling; all reported 19 classes and 188 tests with no failures, and peak resident size of the reused fork stayed between 1.76 and 2.20 GB.
Negative case (what must still be rejected)
No test may be skipped or silently dropped. If the run reports fewer than 19 classes or fewer than 188 tests, the change has traded coverage for speed and must be reverted rather than accepted.
Sharing one fork is the risk this change takes. The comment in
pom.xmlsays so and names this line as the first thing to revert if the profile ever turns flaky.Modes exercised
No mode-specific behaviour changed.
5. Test case coverage regarding this PR
No production Java code changed
Breaking changes and migration
No breaking changes or migration.
Checklist
Review progress