-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Build tuning #1492
Build tuning #1492
Conversation
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
fbf3f37
to
d88f85e
Compare
Could you split this PR into the The |
@alexvanyo sorry I didn't see your message, happy to split up the PR. Do you also want a separate issue? |
d88f85e
to
12ea7d1
Compare
I've removed jemalloc from this PR. |
@kaeawc could you explain the reason for each new flag? I'm not sure they are doing anything worth including in this project. Also, since this project is meant as a showcase, many devs will likely copy/paste the code, it would be best to not add "magic" and unexplained JVM tuning. 🙏 |
@SimonMarquis of course! Metaspace, CodeCache, and Heap are all separate memory stores in the JVM. This goes all the way back to JDK 8. Regarding the removal of MaxMetaspaceSize and addition of MetaspaceSize as 256MB - I outlined my research on the topic here for why Gradle projects that target JDK versions newer than 17 should try removing it. Setting a maximum without understanding of what the maximum limits in a project others will copy is arguably worse than allowing users to run into classloader memory leaks that have mostly been fixed. CodeCache has a similar story that I'm also putting together my research for. The default for a 64bit system of 48MB is small relative to the 64-96MB that starter Android projects could take up and benefit from performance from. CodeCache is meant to have a relatively small limit. Lastly SoftRefLRUPolicyMSPerMB is a pretty straightforward option - based on whether the JVM is configured to run as client or server mode it will release soft references in a number of milliseconds per megabyte of current heap or maximum heap sizes. The default value is 1000, which means for projects with 4GB heap size the JVM will release soft references after 68 minutes (~1 hour). 8GB heaps would release it after 2 hours. Hopefully everyone's builds are faster than that, which means their builds never release any soft references. I have consistently seen setting this to 1 instead of the default of 1000 is a win for releasing memory which can then be used for Metaspace, CodeCache, or the Heap. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes and explantions look good to me, thank you for the research done here!
It looks like the CLA check is failing, if you can accept that I can merge these changes in.
And yes, please open a new issue and PR for the jemalloc change to discuss further, that'd be great.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See suggestion to add link to blog post so that developers can learn more about why these memory options were chosen.
I'm gonna pause on merging this because the JVM args for Kotlin Daemon are not behaving as I expected, see https://youtrack.jetbrains.com/issue/KT-71564/Kotlin-Daemon-JVM-args-behavior-does-not-match-documentation |
So after a lot of digging I found all the JVM inheritance and defaults for Kotlin daemon and have a fix up for Kotlin's documentation on the subject JetBrains/kotlin-web-site#4454. Knowing that we should definitely also set the same args in both as that was the original intention, however specifically for CacheCode and Metaspace:
Those changes are now in this PR |
e918734
to
fde8615
Compare
fde8615
to
15a6b58
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the extra investigation!
@kaeawc Did you actually measure performance of G1 vs ParallelGC? Because you replaced |
Yes I have, both in this and other projects. It does result in a faster build time, but at the cost of larger memory footprint. The difference in speed is not very large in the latest JVMs and in a project like this that will be copied for its settings the community should not blindly follow ParallelGC when most projects will not have the resources to spend on huge CI runners and the most expensive developer machines. |
G1GC is not obsolete. It is the default for a reason. Parallel used to be the default in JDK8 and prior versions, G1GC serves the widest number of applications the best, especially in the latest JDK versions. |
@kaeawc Hmm, your results about GC seem not to match with the benchmarks I found so far.
That's based on assumptions.
I didn't meant G1GC itself is obsolete but the jvmArg Also another aspect: Google itself also recommends testing ParallelGC. Don't get me wrong. I really like this PR and thanks again for all the investigation and explanations in your blog posts. I am really into this whole build optimization topic. Therefore I would suggest an follow up PR which either removes |
@G00fY2 I hear you on using something that is the JVM default. I think let's try to establish the goals more clearly and we can figure out which path is best to take. If we do remove it from the args I'd still like to call out in a comment that we're using G1GC. @alexvanyo @dturner what are your goals be for NowInAndroid? Would you rather have the project be efficient on resources or speed, even if that speed is actually the same? I can redo the perf tests on this size project but also want to do them on a few other sizes to show what these settings look like when scaling over time. |
@kaeawc Our goals for the NowInAndroid build are in order of priority:
|
@dturner in that case, considering @G00fY2's comment:
Based on all that I think this project should keep G1GC arg even though its a default, comment about Google's recommendation for using Parallel. If y'all want to discuss it more lets keep that going. If everyone thinks that's a decent path forward give a 👍🏻 and I'll make a PR with those changes. |
What I have done and why
It was pointed out in a discussion that NowInAndroid uses a 6GB heap size which seemed high. I did a couple perf runs with current settings and no build cache or config cache. I did this at a few heap sizes like 12G, 6G, 4G, and 3G. Only at 3G do we observe a regression in build speed which is pretty easily explained by the 27 seconds spent in garbage collection - the build process was encountering GC thrashing in attempting to complete the build.
I then changed the JVM arg settings to be what I would use for tuning and while the first build at 3G was only slightly faster the subsequent ones are proving to be just as fast as all the other higher heap size ones.
Settings changed so far is just to this:
In this PR I also want to add jemalloc to the GitHub CI as that should give roughly 10% memory savings by fixing native memory fragmentation. I did this through a custom action that caches jemalloc.
I also tried out other heap sizes with the new configuration. Anything higher than 3G didn't make a performance difference and anything lower (2G) ran the risk of the build not completing.
I'm happy to alter anything about this contribution, the goal is to help show how its possible to reduce resource usage while increasing performance.