Conversation
Generate changelog in
|
✅ Successfully generated changelog entry!Need to regenerate?Simply interact with the changelog bot comment again to regenerate these entries. 📋Changelog Preview✨ Features
|
launchlib/config.go
Outdated
| // NativeImageArguments specifies additional arguments that will be passed to the executable when running in native execution mode. | ||
| NativeImageArguments []string `yaml:"nativeImageArguments"` | ||
| NativeImageExecutablePath string `yaml:"nativeImageExecutablePath"` | ||
| // ShrinkableHeapMaxSize sets an explicit max heap size (e.g., "2g", "512m") and enables heap shrinking. |
There was a problem hiding this comment.
naming nit: curious if something like "VariableHeapMaxSize" or "BoundedHeapMaxSize" would be more descriptive? "Shrinkable" makes me think it will reduce the maximum in some way. I don't feel to strongly though, i guess "heap shrinkage" is more or less the correct JVM terminology.
| // All units are binary (1024-based). No suffix means bytes. | ||
| // Examples: "2g", "512m", "1024k", "1t", "2147483648" | ||
| // Returns the size in bytes. | ||
| func ParseMemorySize(s string) (uint64, error) { |
There was a problem hiding this comment.
is this logic really necessary? these strings are not passed directly to JVM args, right? perhaps we just simplify and always require this value to be in megabytes mebibytes.
alternatively, we could implement a golang equivalent to https://github.com/palantir/human-readable-types/blob/develop/human-readable-types/src/main/java/com/palantir/humanreadabletypes/HumanReadableByteCount.java
There was a problem hiding this comment.
The max heap (Xmx) is just passed straight through.
But I'm parsing it so I can determine a min heap (Xms) of 25% of the max heap and then pass that through.
| } | ||
|
|
||
| func isAlwaysPreTouch(arg string) bool { | ||
| return strings.HasPrefix(arg, "-XX:+AlwaysPreTouch") |
There was a problem hiding this comment.
not sure if this is possible, but shouldn't this capture "-XX:-AlwaysPreTouch" as well since we want to manually add it later? or is a redundant flag just ignored
There was a problem hiding this comment.
Yeah this is a bit redundant since I'm not 100% sure how it works under the hood.
I'm trying to remove any -XX:+AlwaysPreTouch (note the +) flags.
But then I'm also appending a -XX:-AlwaysPreTouch(note the -) flag just in case a -XX:+AlwaysPreTouch is coming from somewhere else (having it in the java command might also be nice to be explicit that we're disabling it)
There was a problem hiding this comment.
Switched it to only do the filtering. I'll verify this works as expected once I get an RC cut
launchlib/launcher_test.go
Outdated
| {"1024k", 1024 * 1024, false}, | ||
| {"1024K", 1024 * 1024, false}, | ||
| {"1073741824", 1073741824, false}, // raw bytes | ||
| {" 2g ", 2 * 1024 * 1024 * 1024, false}, // with whitespace |
There was a problem hiding this comment.
just to confirm, is whitespace between the value and unit considered malformed?
There was a problem hiding this comment.
Actually I don't think it's necessary to handle whitespace here. Removed this test.
launchlib/config.go
Outdated
| // When set: -Xmx is set to this value, -Xms is set to 25% of this value, AlwaysPreTouch is disabled, | ||
| // and G1PeriodicGCInterval is set to 10 minutes to periodically return unused memory to the OS. | ||
| // This is intended for memory-oversubscribed environments where the container memory limit exceeds the request. | ||
| // Takes precedence over heapPercentage when set. | ||
| // Note: This option has no effect in native image execution mode. | ||
| ShrinkableHeapMaxSize string `yaml:"shrinkableHeapMaxSize,omitempty"` |
There was a problem hiding this comment.
Thoughts on making the shrink percentage and the periodic interval configurable as well? (that would let you tweak the values when testing, rather than having to push changes)
aldexis
left a comment
There was a problem hiding this comment.
Overall lgtm - just a few questions/comments before approving
| // MaxHeapSize sets an explicit max heap size (e.g., "2g", "512m"). | ||
| // When MaxHeapSize is set without MinHeapSize, MinHeapSize defaults to MaxHeapSize (no shrinking). | ||
| // When MaxHeapSize and MinHeapSize are both set to different values, heap shrinking is enabled: | ||
| // AlwaysPreTouch is disabled to allow the JVM to return memory to the OS. | ||
| // Takes precedence over heapPercentage when set. | ||
| // Note: This option has no effect in native image execution mode. | ||
| MaxHeapSize string `yaml:"maxHeapSize,omitempty"` | ||
| // MinHeapSize sets the minimum heap size (e.g., "512m", "1g"). | ||
| // Requires MaxHeapSize to also be set. If MinHeapSize > MaxHeapSize, an error is returned. | ||
| // When different from MaxHeapSize, enables heap shrinking behavior. | ||
| MinHeapSize string `yaml:"minHeapSize,omitempty"` | ||
| // EnablePeriodicGC enables periodic GC to return unused memory to the OS. | ||
| // Only effective when shrinkable heap is enabled (MaxHeapSize != MinHeapSize). | ||
| // When enabled, adds -XX:G1PeriodicGCInterval for the G1 collector. | ||
| // Note: This flag is G1-specific and will be ignored (or error) on other collectors like ZGC. | ||
| // ZGC has built-in uncommit behavior enabled by default, so no additional flag is needed. | ||
| EnablePeriodicGC bool `yaml:"enablePeriodicGC,omitempty"` | ||
| // PeriodicGCIntervalMs sets the periodic GC interval in milliseconds. | ||
| // Only used when EnablePeriodicGC is true. Defaults to 300000 (5 minutes). | ||
| PeriodicGCIntervalMs *uint64 `yaml:"periodicGCIntervalMs,omitempty"` |
There was a problem hiding this comment.
Just out of curiosity (I don't know enough about go and our config structure wrt GJL), is it possible to have this be in a dedicate config section e.g. shrinkableHeap (nested under experimental)?
| // MaxHeapSize sets an explicit max heap size (e.g., "2g", "512m"). | ||
| // When MaxHeapSize is set without MinHeapSize, MinHeapSize defaults to MaxHeapSize (no shrinking). | ||
| // When MaxHeapSize and MinHeapSize are both set to different values, heap shrinking is enabled: | ||
| // AlwaysPreTouch is disabled to allow the JVM to return memory to the OS. | ||
| // Takes precedence over heapPercentage when set. | ||
| // Note: This option has no effect in native image execution mode. | ||
| MaxHeapSize string `yaml:"maxHeapSize,omitempty"` |
There was a problem hiding this comment.
What happens if maxHeapSize is set, but not minHeapSize? Do we have a default? Look at the JVM args? Fail due to invalid config? (note: this is just from reading the config doc, without yet reading the code)
| // MinHeapSize sets the minimum heap size (e.g., "512m", "1g"). | ||
| // Requires MaxHeapSize to also be set. If MinHeapSize > MaxHeapSize, an error is returned. | ||
| // When different from MaxHeapSize, enables heap shrinking behavior. | ||
| MinHeapSize string `yaml:"minHeapSize,omitempty"` |
There was a problem hiding this comment.
Just for my understanding: the reason we wouldn't set this to something extremely low, is to avoid the JVM returning memory to the OS that we know it will need quickly thereafter, and thus doing a ton of extra work for no reason, correct? (and/or to protect the minimum amount of heap we need to operate properly, to avoid us returning memory to the OS, it being used by another process, and then us failing to get enough memory when we try to get it back?)
| continue | ||
| } | ||
| // Filter out AlwaysPreTouch when using shrinkable heap | ||
| if shrinkableHeap && isAlwaysPreTouch(arg) { |
There was a problem hiding this comment.
Just for my knowledge: I'd expect AlwaysPreTouch to only touch the min heap pages, so I would still have expected it to be beneficial. Could you help me understand why that's not the case?
| // If MaxHeapSize is set, use explicit heap sizes (takes precedence over heapPercentage) | ||
| if hasExplicitHeapConfig { |
There was a problem hiding this comment.
We might have too many ways to define the heap configs 😄 (just reading this piece of code, I'd think -Xmx and -Xms are explicit heap configs, and down the line)
Summary
Adds new experimental config options for explicit heap size control with optional heap shrinking behavior.
New config options (in
experimentalsection):maxHeapSize: Sets the max heap size (e.g.,"2g","512m"). Takes precedence overheapPercentage.minHeapSize: Sets the min heap size. RequiresmaxHeapSizeto be set. If omitted, defaults tomaxHeapSize(no shrinking).enablePeriodicGC: Whentrueand shrinkable heap is enabled, adds-XX:G1PeriodicGCIntervalto trigger periodic GC.periodicGCIntervalMs: Custom interval for periodic GC (default: 300000ms / 5 minutes).Behavior:
maxHeapSizeis set alone orminHeapSize == maxHeapSize: fixed heap size, no shrinking.minHeapSize < maxHeapSize: shrinkable heap is enabled:-XX:+AlwaysPreTouchis filtered out if present in jvmOpts (allows heap to not be fully committed upfront)enablePeriodicGC: true,-XX:G1PeriodicGCIntervalis added (G1-specific)This allows the JVM to dynamically shrink its heap when memory pressure increases on the node. The periodic GC flag is opt-in and G1-specific (ZGC has built-in uncommit behavior).
Example config:
Note: These options have no effect in native image execution mode.