Skip to content

[VL] Count total JVM memory as the on-heap portion for the off-heap sizing feature#9321

Merged
zhztheplayer merged 5 commits intoapache:mainfrom
zhztheplayer:wip-fix-offheap-sizing
Apr 15, 2025
Merged

[VL] Count total JVM memory as the on-heap portion for the off-heap sizing feature#9321
zhztheplayer merged 5 commits intoapache:mainfrom
zhztheplayer:wip-fix-offheap-sizing

Conversation

@zhztheplayer
Copy link
Copy Markdown
Member

@zhztheplayer zhztheplayer commented Apr 14, 2025

Previously the feature uses total memory - free memory as the on-heap occupied size, which was wrong. The free memory is unused by JVM but not yet returned to the OS. The heap occupation from operating system is always the total JVM memory.

The patch corrects the calculation by using the total JVM memory instead.

Related:

#5439
#9276
#8600

With some minor code cleanups in passing.

@github-actions github-actions bot added CORE works for Gluten Core VELOX INFRA labels Apr 14, 2025
@github-actions
Copy link
Copy Markdown

Run Gluten Clickhouse CI on x86

@apache apache deleted a comment from github-actions bot Apr 14, 2025
@zhztheplayer zhztheplayer marked this pull request as ready for review April 15, 2025 10:02
Copy link
Copy Markdown
Member

@zhouyuan zhouyuan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

if (size + usedOffHeapBytesNow + usedOnHeapBytes > MAX_MEMORY_IN_BYTES) {
// Adds the total JVM memory which is the actual memory the JVM occupied from the operating
// system into the counter.
if (size + usedOffHeapBytesNow + totalHeapMemory > MAX_MEMORY_IN_BYTES) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I got the idea and this fix looks correct to me
Cc: @zhli1142015

@zhztheplayer zhztheplayer merged commit c2c8271 into apache:main Apr 15, 2025
51 checks passed
warrenzhu25 pushed a commit to warrenzhu25/gluten that referenced this pull request Jan 10, 2026
…izing feature (apache#9321)

(cherry picked from commit c2c8271)
Change-Id: I0c9affbdc6db4d2c48118ffd8fd0f20fde576692
Reviewed-on: https://bigdataoss-internal-review.googlesource.com/c/third_party/apache/incubator-gluten/+/115776
Reviewed-by: Preetesh Verma <preeteshverma@google.com>
Tested-by: Srinivas S T <srst@google.com>
Reviewed-by: Revanth Venkat Mikkilineni <revanthvenkat@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CORE works for Gluten Core INFRA VELOX

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants