[GLUTEN-13093][CORE] Fix two dynamic off-heap sizing memory bugs - #13094
Open
LuciferYang wants to merge 2 commits into
Open
LuciferYang wants to merge 2 commits into
LuciferYang wants to merge 2 commits into
Conversation
shrinkOnHeapMemory lowered MinHeapFreeRatio alongside MaxHeapFreeRatio but the finally block restored only MaxHeapFreeRatio, so after any shrink the executor JVM kept MinHeapFreeRatio=5 for its lifetime and aggressively shrunk its heap between GCs. Both ratios are now reverted, each revert best-effort on its own so a failing Max revert cannot skip the Min revert. Generated-by: ZCode (GLM-5.3)
…amic sizing With spark.gluten.memory.dynamic.offHeap.sizing.enabled, GlobalOffHeapMemoryTarget charges Gluten's global reservations to the ON-heap pools, but getCurrentAvailableOffHeapMemory always summed the off-heap pools - so the reported available memory ignored every reservation and over-estimated what native allocation could still use. The metric now selects the pools with the same flag the reservation path uses. Generated-by: ZCode (GLM-5.3)
|
Run Gluten Clickhouse CI on x86 |
Contributor
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
No unresolved issues were identified that would block approval.
Review effort: Lite
Findings: None
What changed in this PR
Fixes two dynamic off-heap sizing bugs: heap free-ratio restoration and incorrect memory-pool accounting.
Changes:
- Restores both JVM heap free-ratio settings independently.
- Uses on-heap pools for availability metrics in dynamic mode.
- Adds regression tests for both fixes.
| File | Description |
|---|---|
| gluten-core/src/test/scala/org/apache/gluten/GlutenDynamicOffHeapSizingSuite.scala | Updated as part of this pull request. |
| gluten-core/src/test/java/org/apache/gluten/memory/memtarget/DynamicOffHeapSizingMemoryTargetTest.java | Updated as part of this pull request. |
| gluten-core/src/main/scala/org/apache/spark/memory/SparkMemoryUtil.scala | Updated as part of this pull request. |
| gluten-core/src/main/java/org/apache/gluten/memory/memtarget/DynamicOffHeapSizingMemoryTarget.java | Updated as part of this pull request. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changes are proposed in this pull request?
Two independent bugs in the dynamic off-heap sizing path (
spark.gluten.memory.dynamic.offHeap.sizing.enabled, experimental, off by default).First,
DynamicOffHeapSizingMemoryTarget.shrinkOnHeapMemorylowers bothMinHeapFreeRatioandMaxHeapFreeRatioto push the JVM to release heap, but thefinallyblock restores onlyMaxHeapFreeRatio. After one shrink the executor keepsMinHeapFreeRatio=5for the rest of its life, so it runs a tighter heap and GCs more often long after the off-heap pressure that triggered the shrink is gone. This patch reverts both ratios, each revert best-effort on its own so a failingMaxrevert cannot skip theMinrevert.Second,
SparkMemoryUtil.getCurrentAvailableOffHeapMemoryreads the off-heap storage and execution pools. Under dynamic sizing the reservations are charged to the on-heap pools instead (GlobalOffHeapMemoryTargetacquires withON_HEAP), and the driver plugin forcesspark.memory.offHeap.size=0, so the off-heap pools are empty and the method returns 0. That value feeds the Velox columnar shuffle writer as itsmemLimit, which then falls back to the minimum buffer size. The metric now reads the on-heap pools when dynamic sizing is enabled, matching where the reservations actually land.How was this patch tested?
DynamicOffHeapSizingMemoryTargetTestgains a case that drives the shrink path and assertsMinHeapFreeRatiois restored afterwards (skipped on JVMs where the flag is not settable).GlutenDynamicOffHeapSizingSuitegains a case asserting the availability metric follows the on-heap pool family under dynamic sizing and the off-heap family otherwise.Was this patch authored or co-authored using generative AI tooling?
Yes. Generated-by: ZCode (GLM-5.3).
Related issue: #13093.