[SPARK-47208][CORE] Allow overriding base overhead memory by jpcorreia99 · Pull Request #45240 · apache/spark

jpcorreia99 · 2024-02-24T16:14:43Z

What changes were proposed in this pull request?

We can already select the desired overhead memory directly via the spark.driver/executor.memoryOverhead flags, however, if that flag is not present the overhead memory calculation goes as follows:

overhead_memory = Max(384, 'spark.driver/executor.memory' * 'spark.driver/executor.memoryOverheadFactor')

[where the 'memoryOverheadFactor' flag defaults to 0.1]

This PR adds two new spark configs: spark.driver.minMemoryOverhead and spark.executor.minMemoryOverhead, which can be used to override the 384Mib minimum value.

The memory overhead calculation will now be :

min_memory = sparkConf.get('spark.driver/executor.minMemoryOverhead') // has default of 384

overhead_memory = Max(min_memory, 'spark.driver/executor.memory' * 'spark.driver/executor.memoryOverheadFactor')

Why are the changes needed?

There are certain times where being able to override the 384Mb minimum directly can be beneficial. We may have a scenario where a lot of off-heap operations are performed (ex: using package managers/native compression/decompression) where we don't have a need for a large JVM heap but we may still need a signficant amount of memory in the spark node.

Using the memoryOverheadFactor config flag may not prove appropriate, since we may not want the overhead allocation to directly scale with JVM memory, as a cost saving/resource limitation problem.

Does this PR introduce any user-facing change?

Yes, as described above, two new flags have been added to the spark config. No break of existing behaviours.

How was this patch tested?

Added tests for 3 cases:

If spark.driver/executor.memoryOverhead is set, then the new changes have no effect.
If spark.driver/executor.minMemoryOverhead is set and its value is higher than 'spark.driver/executor.memory' * 'spark.driver/executor.memoryOverheadFactor', the total memory will be the allocated JVM memory + spark.driver/executor.minMemoryOverhead
If spark.driver/executor.minMemoryOverhead but its value is lower than 'spark.driver/executor.memory' * 'spark.driver/executor.memoryOverheadFactor', the total memory will be the allocated JVM memory + 'spark.driver/executor.memory' * 'spark.driver/executor.memoryOverheadFactor'.

Was this patch authored or co-authored using generative AI tooling?

No

jpcorreia99 · 2024-02-28T10:39:13Z

I order to minimize the changes to existing logic, I chose to make this flag not have a default and instead write the logic to check if it is not set and use the existing constant.

However, from a code cleaniness and maintenance standpoint, I think the constant can be deleted and the flag value defaulted to 384Mib.

@Reviewer, what's your view here?

I think it would be cleaner just to have the defaults of the configs 384.

jpcorreia99 · 2024-03-04T09:42:27Z

Hey @HyukjinKwon, would you happen to have the time to give this a review or point me to someone who might?

HyukjinKwon · 2024-03-04T10:18:56Z

This isn't really my area. cc @tgravescs FYI

tgravescs · 2024-03-04T14:47:32Z

can you provide more details as to why this is needed? Its yet another configuration for memory overhead when we already have multiple. I assume you aren't trying to make the minimum lower because you say using a lot of off-heap memory. You can manually set the memoryoverhead vs setting this new config, so what use case is this trying to fix?

jpcorreia99 · 2024-03-04T15:49:34Z

@tgravescs very fair point!

We preconfigure the spark nodes we provide to users with a global config, but then allow the users to override some of the fields.
We observed a reduction in OOMs when bumping the minimum memory overhead.

The reason why we don't explicitly set the memory overhead is because we could accidentally be reducing the overall memory the user has access to.

As an example, we'd like to have at least 500Mb of overhead. User has requested a 30Gb driver. If we directly set the memory overhead to 500Mb, we'd be cutting the overhead available to the user by 2.5Gb (as overhead memory, when not directly stated, is calculated as 10% of JVM).

We also don't want to be changing the overhead factor since these are global config. If we'd like to have at least a 500Mb overhead and we have a default memory of 2Gb, we'd need to set up the factor at 0.25. But then, if the user comes and asks for a 30Gb driver, he'd also receive 7.5Gb in overhead, which is excessive.

As such, defining a global overhead memory minimum would work best if you want to have configurable memory settings and reasonable overhead defaults.

tgravescs

ok, for an overall cluster config that makes sense, but on the other hand you are increasing the minimum for everyone as well and if they don't need the extra memory its wasting it but I guess that is a trade off. I can see the use case, left some comments

tgravescs · 2024-03-04T16:30:11Z

this should be 4.0.0. This is a new feature so generally wouldn't be back ported, at least without discussion.

tgravescs · 2024-03-04T16:30:18Z

tgravescs · 2024-03-04T16:44:46Z

driver and AM aren't necessarily the same process. In cluster mode it is but in client mode it isn't (see spark.yarn.am.memoryOverheah) - https://spark.apache.org/docs/latest/running-on-yarn.html.
Unless you have a use case for the app master doing native stuff, which I don't see, I think this should stay the same (384) or if you have a use case for it we should have a separate config for it since its a YARN specific thing.

I may be misunderstanding something, but doesn't the fact that above we have

private val driverMinimumMemoryOverhead = if (isClusterMode) { sparkConf.get(DRIVER_MIN_MEMORY_OVERHEAD) } else { ResourceProfile.MEMORY_OVERHEAD_MIN_MIB }

guarantee that the changes are only applied in cluster mode, which is case where it actually corresponds to the driver?

ah yes that works

tgravescs · 2024-03-04T16:46:07Z

I think it would be cleaner just to have the defaults of the configs 384.

mridulm · 2024-03-06T07:41:06Z

I would like to understand the usecase better here - It is still unclear to me what characteristics you are shooting for by this PR.

Reduction in OOM is mentioned as a usecase - but overhead does not impact that, heap memory does.

The reason why we don't explicitly set the memory overhead is because we could accidentally be reducing the overall memory the user has access to.

Any increase in overhead memory implies reduction in available heap memory if the max container memory has a upper limit and it is at that limit (if container memory is not limited - there is no impact of memory available to user, tune memory and overhead independently).

The reason behind the question is, I am trying to understand if the default min (which was estimated quite a while ago) needs to be relooked or not - and understanding why your deployment needs a higher min will help.

mridulm

We can simplify the PR by setting default of spark.driver|executor.minMemoryOverhead to 384, and replace existing use of 384 with this config.

mridulm · 2024-03-06T07:42:47Z

set the default here to 384 - and use this instead of existing 384

jpcorreia99 · 2024-03-12T22:09:26Z

Hey @tgravescs I've now addressed your comments and also answered @mridulm, whenever you have the time, I'd highly appreciate if you could give it another review.

jpcorreia99 · 2024-03-12T22:10:48Z

 </tr>
+<tr>
+  <td><code>spark.driver.minMemoryOverhead</code></td>
+  <td>384Mib</td>


@tgravescs just want to make sure I'm being correct here.
I create this flag with .createWithDefaultString("384m"). Is it correct for me to write here Mib instead of m for clarity to the reader or should I stick to exactly the value it contains?

I think I would put it at "384m" to be more consistent with other docs and be format user would specify if changing.

mridulm

Looks good to me, just one point for discussion

mridulm · 2024-03-13T02:57:02Z

      overheadFactor: Double): Long = {
    overHeadMemFromConf.getOrElse(math.max((overheadFactor * executorMemoryMiB).toInt,
-        ResourceProfile.MEMORY_OVERHEAD_MIN_MIB))
+      minimumOverHeadMemoryFromConf))


Discuss:
Do we need MEMORY_OVERHEAD_MIN_MIB after this PR is merged ?
If no, remove that field ?

yarn client mode AM overhead can be specified in that file, does not need to be in ResourceProfile ... ditto for suites.

Thoughts @tgravescs, @jpcorreia99 ?

I do agree, at this point, outside of tests, ResourceProfile.MEMORY_OVERHEAD_MIN_MIB has only one usage in client AM overhead, so will make that change, can revert if @tgravescs disagrees

Addressed in 3abd57e

I agree with removing it.

tgravescs · 2024-03-13T13:52:09Z

 </tr>
+<tr>
+  <td><code>spark.driver.minMemoryOverhead</code></td>
+  <td>384Mib</td>


I think I would put it at "384m" to be more consistent with other docs and be format user would specify if changing.

tgravescs · 2024-03-13T14:07:16Z

ah yes that works

jpcorreia99 · 2024-03-13T14:43:01Z

@tgravescs thank you for the comments, I've addressed them in cd03ec8

tgravescs · 2024-03-13T15:03:14Z

Looks good, thanks @jpcorreia99

jpcorreia99 · 2024-03-14T09:13:10Z

@tgravescs great! Any timeline on merging this?

tgravescs · 2024-03-14T13:26:45Z

Merged to master, thanks @mridulm @jpcorreia99

github-actions Bot added KUBERNETES YARN CORE SQL STRUCTURED STREAMING BUILD DOCS INFRA PYTHON CONNECT and removed SQL STRUCTURED STREAMING BUILD INFRA PYTHON CONNECT labels Feb 24, 2024

jpcorreia99 changed the title ~~Enable user to override base overhead memory~~ [SPARK-47208] Allow overriding base overhead memory Feb 28, 2024

jpcorreia99 marked this pull request as ready for review February 28, 2024 10:28

jpcorreia99 commented Feb 28, 2024

View reviewed changes

Comment thread docs/configuration.md Outdated

HyukjinKwon changed the title ~~[SPARK-47208] Allow overriding base overhead memory~~ [SPARK-47208][CORE] Allow overriding base overhead memory Mar 4, 2024

tgravescs reviewed Mar 4, 2024

View reviewed changes

mridulm reviewed Mar 6, 2024

View reviewed changes

github-actions Bot added SQL ML labels Mar 12, 2024

Finish implementation for yarn

37ea530

github-actions Bot removed SQL ML STRUCTURED STREAMING WEB UI BUILD EXAMPLES INFRA PYTHON R PANDAS API ON SPARK CONNECT labels Mar 12, 2024

jpcorreia99 added 5 commits March 12, 2024 22:07

add docs + fix test

8d73245

Add proper spark jira link

cdc03dc

correct version in docs

509cbe7

remove extra newlines

cbee4da

address comments

499a724

jpcorreia99 commented Mar 12, 2024

View reviewed changes

mridulm reviewed Mar 13, 2024

View reviewed changes

Remove ResourceProfile.MEMORY_OVERHEAD_MIN_MIB

3abd57e

tgravescs reviewed Mar 13, 2024

View reviewed changes

address formatting

cd03ec8

asfgit closed this in 4b073fd Mar 14, 2024

Conversation

jpcorreia99 commented Feb 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Was this patch authored or co-authored using generative AI tooling?

Uh oh!

jpcorreia99 Feb 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jpcorreia99 commented Mar 4, 2024

Uh oh!

HyukjinKwon commented Mar 4, 2024

Uh oh!

tgravescs commented Mar 4, 2024

Uh oh!

jpcorreia99 commented Mar 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tgravescs left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jpcorreia99 Mar 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mridulm commented Mar 6, 2024

Uh oh!

mridulm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jpcorreia99 commented Mar 12, 2024

Uh oh!

jpcorreia99 Mar 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mridulm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jpcorreia99 commented Feb 24, 2024 •

edited

Loading

jpcorreia99 Feb 28, 2024 •

edited

Loading

jpcorreia99 commented Mar 4, 2024 •

edited

Loading

jpcorreia99 Mar 12, 2024 •

edited

Loading

jpcorreia99 Mar 12, 2024 •

edited

Loading