Skip to content

[CORE] Decrease offheap memory size in resource profile for whole stage fallback case#8911

Merged
jackylee-ch merged 2 commits intoapache:mainfrom
philo-he:decreass-offheap
Apr 9, 2025
Merged

[CORE] Decrease offheap memory size in resource profile for whole stage fallback case#8911
jackylee-ch merged 2 commits intoapache:mainfrom
philo-he:decreass-offheap

Conversation

@philo-he
Copy link
Copy Markdown
Member

@philo-he philo-he commented Mar 5, 2025

What changes were proposed in this pull request?

For whole stage fallback case, we should not request the same offheap memory size as offload case.

How was this patch tested?

Fix a feature that is still experimental.

@github-actions github-actions bot added the CORE works for Gluten Core label Mar 5, 2025
@github-actions
Copy link
Copy Markdown

github-actions bot commented Mar 5, 2025

Thanks for opening a pull request!

Could you open an issue for this pull request on Github Issues?

https://github.com/apache/incubator-gluten/issues

Then could you also rename commit message and pull request title in the following format?

[GLUTEN-${ISSUES_ID}][COMPONENT]feat/fix: ${detailed message}

See also:

@github-actions
Copy link
Copy Markdown

github-actions bot commented Mar 5, 2025

Run Gluten Clickhouse CI on x86

@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 2, 2025

Run Gluten Clickhouse CI on x86

@philo-he philo-he marked this pull request as ready for review April 2, 2025 05:43
executorResource.put(ResourceProfile.MEMORY, newExecutorMemory)

val newExecutorOffheap =
new ExecutorResourceRequest(ResourceProfile.OFFHEAP_MEM, offheapRequest.get.amount / 10)
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@zjuwangg, could you take a look? This is just an empirical setting. And I am not sure which setting is better.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM in whole stage fallback case!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This would work only for Spark 3.5.4 or higher as apache/spark#48963

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@philo-he Should we consider making this setting configurable?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@JkSelf, let's first use this empirical fixed setting and wait for feedback from user side. In theory, user don't need to configure too much offheap memory in stage fallback case. Thanks!

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.

@philo-he
Thanks for improving on this!
just to confirm, I can see the code logic is to reduce the offheap memory size to 1/10, but on-heap memory size is not increased, is this intended?

Thanks, -yuan

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@zhouyuan, the on-heap memory is increased by specifying a larger value for ResourceProfile.MEMORY prior to the off-heap adjustment. If I have missed something, please let me know. Thanks!

executorResource.put(ResourceProfile.MEMORY, newExecutorMemory)

val newExecutorOffheap =
new ExecutorResourceRequest(ResourceProfile.OFFHEAP_MEM, offheapRequest.get.amount / 10)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM in whole stage fallback case!

@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 7, 2025

Run Gluten Clickhouse CI on x86

Copy link
Copy Markdown
Contributor

@JkSelf JkSelf left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks

@jackylee-ch jackylee-ch merged commit ab4c4b8 into apache:main Apr 9, 2025
50 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CORE works for Gluten Core

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants