sbglasius commented on code in PR #16376:
URL: https://github.com/apache/grails-core/pull/16376#discussion_r4073079661
##########
gradle.properties:
##########
@@ -96,8 +93,23 @@ org.gradle.daemon=true
# other native memory, the Gradle client, and forked Java/Groovy compiler
workers (which
# CompilePlugin gives their own -Xmx2G). Treat it as a floor when sizing a
runner.
#
-# On the 4-CPU / ~16 GB Linux and Windows runners that floor is 5G + 4x768m =
8G, which
-# fits. On the 3-CPU / ~7 GB macOS runner it is 5G + 3x768m = 7.25G, which
does not - so
-# .github/workflows/gradle.yml caps --max-workers there to reduce memory
pressure, rather
-# than shrinking this daemon and slowing groovydoc.
-org.gradle.jvmargs=-Dfile.encoding=UTF-8 -Xmx5G
+# Documentation does NOT run on this heap. Every groovydoc task is launched as
a JVM of its
+# own by GroovydocEnhancerPlugin, and the user guide in a worker process by
PublishGuideTask.
+# That is what lets this number be 3G: before, groovydoc kept a Groovy runtime
per documented
+# module alive inside the daemon for the whole build, which needed 5G and
still exhausted it
+# on the smallest runner.
+#
+# 3G is measured, not guessed. `build :grails-shell-cli:installDist groovydoc
-PskipTests
+# --max-workers=2` - 3253 tasks, the macOS job's graph without test execution
- runs it with
+# ZERO full GCs; mixed collections settle the daemon at about 2.0 GB
throughout. Re-measure
+# with -Xlog:gc* before changing it, and read the MIXED collections: a
young-only "after GC"
+# figure does not touch the old generation and reads about 700 MB higher than
the truth.
+# Raising this on a memory-constrained runner can backfire - a larger heap
lets G1 defer
+# collection, so the daemon's resident size grows to meet it and leaves the
forked doc JVMs
+# and test forks less room, not more.
+org.gradle.jvmargs=-Dfile.encoding=UTF-8 -Xmx3G
Review Comment:
**The 3G figure is measured without test execution, but every CI job runs
the full `build`.**
The comment above says so explicitly: `build :grails-shell-cli:installDist
groovydoc -PskipTests` - "the macOS job's graph without test execution". Moving
groovydoc and the guide out of the daemon genuinely removes their heap, but
test execution adds daemon-side load that measurement never exercised - Gradle
streams and buffers test events across ~all modules, generates the aggregated
HTML/XML test reports, and runs JaCoCo/report aggregation in the daemon.
So a 40% heap cut validated on a tests-free graph may move the OOM from
`publishGuide` to `:test` reporting / `check` aggregation rather than remove
it. Worth re-measuring with tests in the graph (or keeping a higher floor until
that run is clean) before this lands on the macOS job, which is the one with no
headroom.
##########
build-logic/docs-core/src/main/groovy/grails/doc/gradle/PublishGuideTask.groovy:
##########
@@ -110,8 +133,11 @@ class PublishGuideTask extends DefaultTask {
propertiesFiles = objects.fileCollection()
sourceDir =
objects.directoryProperty().convention(project.layout.projectDirectory.dir('src'))
resourcesDir =
objects.directoryProperty().convention(project.layout.projectDirectory.dir('resources'))
- macros = objects.listProperty(Object).convention([])
+ macros = objects.listProperty(String).convention([])
targetDir =
objects.directoryProperty().convention(project.layout.buildDirectory.dir('docs'))
+ maxHeapSize = objects.property(String)
Review Comment:
**`guideMaxHeapSize` is wired as the *convention*, so a build script
silently beats the command line** - the opposite of the javadoc just above and
of what the groovydoc side does.
A convention only applies while nothing has been set explicitly. Once any
build script does:
```groovy
publishGuide.maxHeapSize = '2g'
```
then `./gradlew publishGuide -PguideMaxHeapSize=4g` keeps `2g`, the guide
still OOMs, and nothing says why. The groovydoc path deliberately does the
reverse - `GroovydocEnhancerPlugin.resolveMaxHeapSize` reads the project
property first and only falls back to the extension - and both the javadoc at
line 108 ("A `guideMaxHeapSize` project property beats whatever is set here")
and `gradle.properties` ("either beats the build script") promise the groovydoc
behaviour.
Resolving it in the task action the same way `resolveMaxHeapSize` does would
make the two consistent.
Relatedly: `gradle.properties` says the doc JVM heap is "raised ... by
grails-doc for the guide", but `grails-doc/build.gradle` never sets
`maxHeapSize` on `publishGuide` - the guide runs on the `1500m` default.
##########
build-logic/docs-core/src/main/groovy/grails/doc/gradle/PublishGuideTask.groovy:
##########
@@ -124,81 +150,39 @@ class PublishGuideTask extends DefaultTask {
}
@TaskAction
- def publishGuide() {
- Properties combinedProperties = new Properties()
-
- File workingDir =
Files.createTempDirectory('grails-doc-publish-guide').toFile()
-
- File resources = resourcesDir.get().asFile
- File docProperties = new File(resources, 'doc.properties')
- if (docProperties.exists()) {
- docProperties.withInputStream { input ->
- combinedProperties.load(input)
- }
- }
-
- // Add properties from any optional properties files too.
- for (File f : propertiesFiles) {
- f.withInputStream { input ->
- combinedProperties.load(input)
- }
- }
- combinedProperties.putAll(properties.get())
- combinedProperties.putAll(propertiesWithFilePaths.get())
-
- File apiDir = targetDir.get().asFile
- apiDir.deleteDir()
- apiDir.mkdirs()
-
- def publisher = new DocPublisher(sourceDir.get().asFile, apiDir)
- publisher.ant = ant
- publisher.asciidoc = asciidoc
- publisher.workDir = workingDir
- publisher.apiDir = apiDir
- publisher.language = language.getOrElse('')
- publisher.sourceRepo = sourceRepo.getOrElse('')
- publisher.images = new File(resources, 'img')
- publisher.css = new File(resources, 'css')
- publisher.fonts = new File(resources, 'fonts')
- publisher.js = new File(resources, 'js')
- publisher.style = new File(resources, 'style')
- publisher.version = combinedProperties['grails.version']
-
- // Override doc.properties properties with their language-specific
counterparts (if
- // those are defined). You just need to add entries like es.title or
pt_PT.subtitle.
- if (language.isPresent()) {
- String lang = language.get()
- def pos = lang.size() + 1
- def languageProps = combinedProperties.findAll { k, v ->
k.startsWith("${lang}.") }
- languageProps.each { k, v -> combinedProperties[k[pos..-1]] = v }
+ void publishGuide() {
+ // Everything is read into locals first. Both the fork options and the
work parameters
+ // have members of their own named like this task's properties -
maxHeapSize,
+ // properties - and inside the configuration closures those would win.
+ String workerHeap = this.maxHeapSize.get()
+ String languageValue = this.language.getOrNull()
+ String sourceRepoValue = this.sourceRepo.getOrNull()
+ Boolean asciidocValue = this.asciidoc.get()
+ Map<String, Object> engineProperties = this.properties.get()
+ Map<String, File> filePathProperties =
this.propertiesWithFilePaths.get()
+ FileCollection propertiesFileValues = this.propertiesFiles
+ Directory sourceDirValue = this.sourceDir.get()
+ Directory resourcesDirValue = this.resourcesDir.get()
+ Directory targetDirValue = this.targetDir.get()
+ List<String> macroNames = this.macros.get()
+ Boolean verboseAntValue = this.verboseAnt.get()
+
+ WorkQueue queue = workerExecutor.processIsolation { spec ->
Review Comment:
**A `processIsolation` worker is a pooled worker daemon, not a per-task
process** - so the guide JVM is not released when `publishGuide` finishes.
Gradle keeps process-isolation workers in `WorkerDaemonClientsManager` and
reuses them for compatible work; they are torn down when the build session ends
or the daemon expires them, not at the end of the task action. That means a JVM
sized by `maxHeapSize` (1500m) stays resident while the rest of the
documentation chain runs - `aggregateGroovydoc` at 3g,
`aggregateDataMappingGroovydoc` at 2g - which is the memory pressure this PR is
trying to remove.
The groovydoc side does not have this problem: `ExecOperations.javaexec`
really does exit with the task. Worth either aligning the guide on `javaexec`
too, or softening the class docs on `PublishGuideWorkAction` - they currently
read as though the burst is handed straight back to the OS.
##########
build-logic/plugins/src/test/groovy/org/apache/grails/buildsrc/GroovydocEnhancerPluginSpec.groovy:
##########
@@ -51,4 +89,74 @@ class GroovydocEnhancerPluginSpec extends Specification {
groovydocFiles.any { it.name == runtimeOnlyJar.name }
groovydocFiles.any { it.name == compileOnlyJar.name }
}
+
+ void 'groovydoc documents the sources with the groovy the project builds
against'() {
+ given: 'a project whose documentation classpath pins its own groovy'
+ writeGroovydocProject()
+
+ when: 'groovydoc runs'
+ def result = GradleRunner.create()
+ .withProjectDir(projectDir)
+ .withArguments('groovydoc')
+ .withPluginClasspath()
+ .build()
+
+ then: 'the task succeeds and documents the source'
+ result.task(':groovydoc').outcome == TaskOutcome.SUCCESS
+ File documented = new File(projectDir,
'build/docs/groovydoc/com/example/Documented.html')
+ documented.exists()
+
+ and: "the project's groovy generated it, not the older one Gradle runs
build logic on"
+ documented.text.contains("Generated by groovydoc (${groovyVersion})")
+ }
+
+ void 'the groovydoc heap is the forked jvm heap, not the daemon heap'() {
+ given: 'a project that would document happily on the daemon heap'
+ writeGroovydocProject()
+
+ when: 'groovydoc is given a heap no JVM can start with'
+ def result = GradleRunner.create()
+ .withProjectDir(projectDir)
+ .withArguments('groovydoc', '-PgroovydocMaxHeapSize=1m')
+ .withPluginClasspath()
+ .buildAndFail()
+
+ then: 'a JVM refused to start on it - which only a forked JVM could
report'
+ result.task(':groovydoc').outcome == TaskOutcome.FAILED
+ result.output.contains('Too small maximum heap')
Review Comment:
**These assertions pin HotSpot's exact wording, and the new fixtures pull
this spec onto the network.**
`'Too small maximum heap'` and `'Error occurred during initialization of
VM'` are HotSpot's phrasing for `-Xmx1m`. On a JVM that words it differently -
or if HotSpot rewords it - the test fails while the plugin is correct.
Asserting on the forked JVM's non-zero exit (the `ExecException` surfacing as
the task failure) proves the same thing - that the heap setting reached a
*separate* JVM - without depending on a message string.
Separately, `writeGroovydocProject` declares `repositories { mavenCentral()
}` and real Groovy dependencies, so `:build-logic:plugins:test` - previously a
fast ProjectBuilder-only spec - now needs network access and a populated
dependency cache. In an offline or air-gapped build it fails at dependency
resolution rather than reporting a plugin defect.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
