Adjust JVM heap size for extremely large memory machine #31567
Conversation
sdks/go.mod
Outdated
| @@ -20,7 +20,7 @@ | |||
| // directory. | |||
| module github.com/apache/beam/sdks/v2 | |||
|
|
|||
| go 1.20 | |||
| go 1.21 | |||
There was a problem hiding this comment.
this is required for builtin max.
go 1.20 has already eol so we should bump it anyways.
Last time it was July 2023 bumped go 1.19->1.20.
There was a problem hiding this comment.
A single use of max isn't sufficient cause to bump the minimum SDK version IMO. I'd recommend just writing the standard if instead.
There was a problem hiding this comment.
While I don't think it'll be a problem to do it as a rule, there's just other work that might be good to do when bumping the minimum version as well, and that's very out of scope of this PR.
Basically, bumping the minimum version is a huge change just to avoid a single if statement, compared to writing the conditional.
sdks/java/container/boot.go
Outdated
| } else { | ||
| return limLarge | ||
| } |
There was a problem hiding this comment.
Stylewise, don't have the else wrapper around a return statement like that, just have the return after the if block.
|
Checks are failing. Will not request review until checks are succeeding. If you'd like to override that behavior, comment |
|
actually the change has a bug, either in max(,) or assign (size - 32 << 30) to an uint64 will cause unsigned integer overflow. This caused Dataflow Example PreCommit to fail. |
|
Assigning reviewers. If you would like to opt out of this review, comment R: @damondouglas for label java. Available commands:
The PR bot will only process comments in the main thread (not review comments). |
| // if set_recommended_max_xmx experiment is enabled, sets xmx to 32G. Under 32G | ||
| // JVM enables CompressedOops. CompressedOops utilizes memory more efficiently, | ||
| // and has positive impact on GC performance and cache hit rate. | ||
| func HeapSizeLimit(size uint64) uint64 { |
There was a problem hiding this comment.
Here added a unit test responding the change request from internal. However to do so I need to change the method to take an external value.
I try to keep the logic, the effective order of execution the same as before (e.g. syscallx.PhysicalMemorySize won't be called if set_recommended_max_xmx is set), PTAL
sdks/java/container/boot_test.go
Outdated
| t.Errorf("HeapSizeLimit(0). Actual (%d). want 1 GB", lim) | ||
| } | ||
| if lim := HeapSizeLimit(2 << 30); lim != (2 << 30) * 7 / 10 { | ||
| t.Errorf("HeapSizeLimit(0). Actual (%d). want 700 MB", lim) |
There was a problem hiding this comment.
The parameter wasn't updated in the t.Error call. Here and the last one too.
There was a problem hiding this comment.
sands for catching it. fixed
|
This can be merged ? |
|
thanks, there is an internal counterpart (for legacy worker container) and intended to submit both after 2.57.0 release. We can merge this external one first as it won't get integrated into the ongoing release (the internal one will do for container) |
The current setting (70% of max memory) is not efficient for extremely large memory machine.
Internal context: b/340891295#comment14
Please add a meaningful description for your change here
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
addresses #123), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, commentfixes #<ISSUE NUMBER>instead.CHANGES.mdwith noteworthy changes.See the Contributor Guide for more tips on how to make review process smoother.
To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md
GitHub Actions Tests Status (on master branch)
See CI.md for more information about GitHub Actions CI or the workflows README to see a list of phrases to trigger workflows.