jdaugherty commented on PR #16376: URL: https://github.com/apache/grails-core/pull/16376#issuecomment-5779152968
Thanks for the thorough pass @matrei - the verification list is appreciated, particularly checking that the GROOVY-12318 parser-cache bound doesn't make the leak moot. All five points are addressed. **1. Worker JVM lifetime.** Corrected in 725d807. I checked it rather than take it on trust: after a `publishGuide` build the only surviving workers are the `-Xmx2G` Groovy compile daemons - no `-Xmx1500m` process remains - so it is session-scoped exactly as you describe. Worth adding that the ceiling reserves nothing: once idle it held ~284 MB resident, not 1500m. `PublishGuideWorkAction`'s javadoc no longer says "short-lived JVM"; it says the worker is stopped when the build session ends rather than when the task returns, and that it can still be resident while the aggregate groovydoc runs. I kept the worker rather than moving to `javaexec`, deliberately: the guide is build logic and runs on the Groovy that Gradle embeds, which a worker inherits for free and a bare JVM would force us to pin a version for. Groovydoc needed `javaexec` only because of the Groovy 4/5 classloader conflict - without it the fork silently generated docs with Gradle's Groovy 4 and visibly poorer type links. Measured peak is comfortable either way: daemon 1.7 GB + aggregate 1.9 GB + guide 0.9 GB against 7 GB. Say the word if you'd rather have the two paths identical and I'll convert it. The PR description line you flagged overstates it the same way and needs the same edit. **2. SLF4J noise.** Good catch, and your diagnosis was exactly right - reproduced it on `:grails-core:groovydoc` (three lines), fixed in dfede9d by adding `org.slf4j:slf4j-nop` to the `documentation` configuration, mirroring the `cliCompileOnly` treatment for compilation. Now zero SLF4J lines, and I confirmed the aggregate API output is still byte-for-byte identical and `validateDependencyVersions` stays clean. **3. HotSpot wording in the TestKit assertions.** Fixed in 725d807, which crossed with your review. Both assertions use Gradle's own `finished with non-zero exit value` instead - same proof that the heap reached a separate process, no dependency on a JVM's phrasing. **4. `groovydocMaxHeapSize` listed as a `gradle.properties` key.** Right, it was in that table and deliberately not in the file. Removed in dfede9d and replaced with a note under the table saying neither override property belongs there, with each JVM's own default spelled out. **5. Two mechanisms for the same override.** Unified in dfede9d - both now go through `providers.gradleProperty`, the form that survives the configuration cache if #15497 is ever closed. Unrelated to this PR, but worth flagging since I hit it while running the suites: `ConfigurationMetadataPluginSpec` has two failing tests on the unmodified base commit (1fadd5c), so `:build-logic:test` is red on 8.0.x independently of this change. -- 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]
