-
-
Notifications
You must be signed in to change notification settings - Fork 962
Fix build performance issues #14895
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix build performance issues #14895
Conversation
…ons.bottom task input
…d of absolute paths
…s instead of absolute paths
@jprinet I updated this branch - we had some known failures that have been fixed since you did this work. |
Alright, thanks for the heads up. I am happy to trim what is already addressed 👍 |
Please note it's still a draft @jdaugherty as I need to confirm the fix for |
@jprinet FYI: There's a known bug with the asset compiler (wondrify/asset-pipeline#177) around concurrent modifications. @davydotcom I think is taking a look at this, but I don't think it will be any time soon. That's the reason we had to serialize on a common output if that task runs in parallel. This should explain the concurrent modification exceptions. Concerning the build info file, I only recently discovered that the metadata loading in grails is broken & discovered this file. We need to apply the same fix to it we did for other property files when the SOURCE_EPOCH_DATE variable is set. Instead of ignoring, I'd rather fix that (will do that today). |
We appreciate help like this, we've come so far on the build that it is very welcome. Thank you for helping! |
grails-gradle/docs-core/src/main/groovy/grails/doc/gradle/PublishGuideTask.groovy
Show resolved
Hide resolved
...adle/plugins/src/main/groovy/org/grails/gradle/plugin/views/json/GsonViewCompilerTask.groovy
Outdated
Show resolved
Hide resolved
This reverts commit 5a6e595. See wondrify/asset-pipeline#177
This reverts commit 46ee033.
It is now ready for review @jdaugherty |
@jprinet |
@jprinet I think you've convinced me the property files should always be reproducible. I will open a ticket and fix this in the coming week. As for these contributions, thank you again so much. They seem to work great locally and once the property files are always reproducible, I think it will help even more per your observations. |
It should be a great move for build performances 👍 |
Issue
The build performances are currently not optimal and could be improved.
Focusing on build cacheability, running twice the
build
task without any change and cache enabled on a fresh Gradle user home surfaces some performance issues, which can result in up to 26mn of CPU time wasted on a build.Experiment
I used an automated experiment to run the build twice (GitHub workflow)
The summary of the experiment can be checked there
We can in particular notice the second build from experiment 3 suffering from 171 unexpected cache misses, totalling almost 26mn of serial time.
Fixes
Several fixes were applied and are isolated on specific commits for review convenience. Impacted tasks per commit are also provided in each subsection.
Here is a build with the fixes applied, demonstrating 100% cache hits
Apply
RelativePath
normalization on task inputs to improve cacheabilityaggregateDataMappingGroovydoc
aggregateGroovydoc
asciidoctor
compileGsonViews
compileProfile
Ignore
grails.build.info
with runtime classpath normalizationcompileIntegrationTestGroovy
compileTestGroovy
integrationTest
test
Improve Javadoc cacheability by using date instead of instant on options.bottom task input
javadoc
Fix AsciiDoc task cache relocatability by using relative paths instead of absolute paths
asciidoc
Fix PublishGuideTask task cache relocatability by using relative paths instead of absolute paths
Fix GroovyPageForkCompileTask cacheability by defining a task output
compileGroovyPages