-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Set minimum heap size for native Turbine to 512 MB #20360
Conversation
@cushon I found a minimum heap size of 512 MB to give a very nice speedup on my local machine (can only test on Linux right now unfortunately). Do you happen to know how you ended up with the 3 GB minimum for Google? Do you think that the benchmark I am using is reasonable? |
This all looks reasonable to me. I tuned it in early 2022, and at the time we were giving JavaBuilder 3g of heap, so I tried the same for turbine. I don't remember if I collected measurements for smaller values, and for Google builds I wasn't really focused on the local resource estimate. The benchmark seems like a good measure of average performance. The heap sizing we're using for JavaBuilder at Google gets dragged up by a handful of pathological examples that include very large generated files, etc. I have a TODO to allow tuning jvm flags for JavaBuilder for individual targets, to allow setting smaller defaults and then tuning for those outlier targets. It's possible we'll eventually want to do that for turbine, and I noticed that it also supports runtime |
Edit: Doesn't work yet since the ability to set args is not exposed to the Starlark interface of |
@cushon As you said, I don't think that there is currently any other way than setting the min heap size at image build time. I won't make any further changes, this is ready for review. |
Should this be cherry-picked into 7.1? |
@brentleyjones This entirely lives in java_tools prebuilts, but I do plan to cherry-pick a rules_java update with this change into 7.1.0. Currently it's not even live at HEAD. We can still cherry-pick this change for transparency. |
@bazel-io flag |
@bazel-io fork 7.1.0 |
@keertk If you have the time, could you create a new java_tools release with this change? |
@keertk No worries, this can wait! |
Tracked here: bazelbuild/java_tools#87 |
Done - |
No description provided.