Improve project dependency substitution logic - #16069
Conversation
matrei
changed the title
…st configuration This speeds up the configuration phase.
1872843 to
cc471e4
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 8.0.x #16069 +/- ##
==================================================
- Coverage 53.1945% 51.8614% -1.3331%
- Complexity 17798 18111 +313
==================================================
Files 1980 2046 +66
Lines 92519 96274 +3755
Branches 16075 16727 +652
==================================================
+ Hits 49215 49929 +714
- Misses 36076 38974 +2898
- Partials 7228 7371 +143 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Note
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Speeds up Gradle configuration (notably functional test setup) by precomputing dependency-substitution metadata for eligible subprojects and reusing it across configuration resolution.
Changes:
- Precomputes a filtered list of substitutable subprojects (including resolved artifact IDs) once, instead of recalculating per-configuration.
- Refactors the dependency substitution loop to iterate over the precomputed substitution metadata.
- Keeps special-case substitutions (BOM platform +
grails-gebtest-fixtures variant) while applying the new precomputed structure.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for (def substitution : substitutableProjects) { | ||
| def possibleProject = substitution.project | ||
| def substitutedArtifact = "$possibleProject.group:${substitution.artifactId}" | ||
| //TODO: This does not handle libraries that are both test fixtures & a libraries like grails-data-mongodb, |
| def substitutableProjects = rootProject.subprojects | ||
| .findAll { !(it.name in testProjects) && !(it.name in docProjects) && !(it.name in cliProjects) } | ||
| .each { project.evaluationDependsOn(it.path) } | ||
| .collect { | ||
| project.evaluationDependsOn(it.path) | ||
| return [ | ||
| project: it, | ||
| artifactId: it.findProperty('pomArtifactId') ?: it.name, | ||
| cliArtifactId: it.findProperty('cliArtifactId') | ||
| ] | ||
| } |
| .collect { | ||
| project.evaluationDependsOn(it.path) | ||
| return [ | ||
| project: it, | ||
| artifactId: it.findProperty('pomArtifactId') ?: it.name, | ||
| cliArtifactId: it.findProperty('cliArtifactId') | ||
| ] | ||
| } |
jdaugherty
left a comment
jdaugherty
left a comment
There was a problem hiding this comment.
Verified this is behavior-preserving and that the performance claim holds.
Configuration time (./gradlew help, warm daemon, 3 runs each, --profile → "Configuring Projects"): 7.95s → 3.69s (−54%); total wall time 17.3s → 10.8s. "Roughly in half" checks out.
Equivalence checks
- Snapshot vs. lazy read — instrumented the precomputed snapshot and compared each entry against the value the previous code would have observed later, at resolution time: 134 substitutable projects × 104 applying test projects, zero mismatches (9 projects carry a
cliArtifactId, 7 a custompomArtifactId). - Resolved graph —
:grails-test-examples-app1:dependencies --configuration testRuntimeClasspathis identical. A first cross-checkout comparison showedgrails-core-cliandgrails-consolegaining a requested version, but a controlled same-directory A/B (base version of this file swapped into the PR worktree) reproduced the PR output exactly, so that delta was checkout/build state, not this change. - Published BOM —
:grails-bom:generatePomFileForMavenPublicationoutput is byte-identical, including all 9*-cliconstraints. - Flat paths —
project(":$name")stays safe: all 134 substitutable projects satisfypath == ":$name", so hoisting the filter introduces no nested-path regression.
No correctness issues found. Two non-blocking notes inline.
| return [ | ||
| project: it, | ||
| artifactId: it.findProperty('pomArtifactId') ?: it.name, | ||
| cliArtifactId: it.findProperty('cliArtifactId') |
There was a problem hiding this comment.
This moves the cliArtifactId read from lazy (per configuration, when the substitution rules are registered) to eager (immediately after evaluationDependsOn). That matters because cliArtifactId is only set inside an afterEvaluate in GrailsCliArtifactGradlePlugin, which gradle/cli-companion-bom-constraints.gradle documents as the reason it uses a lazy withDependencies window.
It is correct as written — evaluationDependsOn flushes the target's afterEvaluate before returning, and I confirmed the snapshot matches the late value for all 134 substitutable projects across all 104 applying test projects with zero mismatches.
The only thing I would add is a short comment recording that invariant. The snapshot is now silently order-dependent: if cliArtifactId ever moves to a hook later than afterEvaluate, or gets set cross-project, the cli substitutions would quietly stop being registered rather than fail loudly, which is an unpleasant failure mode to debug.
| def substitutableProjects = rootProject.subprojects | ||
| .findAll { !(it.name in testProjects) && !(it.name in docProjects) && !(it.name in cliProjects) } | ||
| .each { project.evaluationDependsOn(it.path) } | ||
| .collect { |
There was a problem hiding this comment.
Optional follow-up, not a blocker. This snapshot is rebuilt independently by each of the 104 test projects that apply this script, so the findAll plus 134 evaluationDependsOn/findProperty calls all run 104 times over.
Computing it once and sharing it (keeping a cheap per-project evaluationDependsOn loop so evaluation ordering is unchanged) measured a further ~10% off configuration time, 3.69s → 3.32s, with identical resolution output.
For completeness, I also tried precomputing the "$group:$artifactId" coordinate strings into the snapshot so the loop body does no GString work at all — that was only 3.69s → 3.53s, near noise, so it is not worth doing for performance alone. The bulk of the win is already in this PR.
✅ All tests passed ✅🏷️ Commit: d4945db Learn more about TestLens at testlens.app. |
This cuts the Gradle configuration time roughly in half.