Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SPARK-1777 (partial)] bugfix: make size of requested memory correctly #1892

Closed

Conversation

liyezhang556520
Copy link
Contributor

No description provided.

@AmplabJenkins
Copy link

Can one of the admins verify this patch?

@andrewor14
Copy link
Contributor

Hi @liyezhang556520, can you explain your changes?

The idea of the existing code is that after requesting, we will end up at currentSize * memoryGrowthFactor, so the delta between what we will have and what we have now is currentSize * (memoryGrowthThreshold - 1). The problem with using memoryThreshold instead is that the current size of the buffer may overshoot the threshold by a lot, e.g. if we have one very large item.

@liyezhang556520
Copy link
Contributor Author

Hi @andrewor14,
Yes, after requesting, memoryThreshold is supposed to be currentSize * memoryGrowthFactor. And the memory we have now is equals to memoryThreshold instead of the currentSize, so the delta between what we will have and what we have now should be currentSize * memoryGrowthFactor - memoryThreshold, if you are using currentSize * (memoryGrowthThreshold - 1) as the delta, will memory size currentSize - memoryThreshold missed (not calculated in memory request by the current thread in unrollMemoryMap)?

@andrewor14
Copy link
Contributor

Ah I see. memoryThreshold is the amount of memory we currently hold in the global memory map. To end up at currentSize * memoryGrowthFactor, we need a delta of currentSize * memoryGrowthFactor - memoryThreshold. This makes sense.

@@ -254,7 +254,7 @@ private[spark] class MemoryStore(blockManager: BlockManager, maxMemory: Long)
}
}
// New threshold is currentSize * memoryGrowthFactor
memoryThreshold = currentSize + amountToRequest
memoryThreshold += amountToRequest
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this becomes currentSize * memoryGrowthFactor if you do the math. Why not just set it to this value? (Then you can remove the comment above)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's better use memoryThreshold += amountToRequest than memoryThreshold = currentSize * memoryGrowthFactor, because memoryThreshold += amountToRequest is more obvious for coder reader how memoryThreshold increases, and the comment above is more easy to understand.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, that's also fine

@andrewor14
Copy link
Contributor

add to whitelist

@SparkQA
Copy link

SparkQA commented Aug 13, 2014

QA tests have started for PR 1892. This patch merges cleanly.
View progress: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/18404/consoleFull

@SparkQA
Copy link

SparkQA commented Aug 13, 2014

QA results for PR 1892:
- This patch PASSES unit tests.
- This patch merges cleanly
- This patch adds no public classes

For more information see test ouptut:
https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/18404/consoleFull

@andrewor14
Copy link
Contributor

LGTM

@rxin
Copy link
Contributor

rxin commented Aug 13, 2014

Thanks. Merging in master & branch-1.1

asfgit pushed a commit that referenced this pull request Aug 13, 2014
Author: Zhang, Liye <liye.zhang@intel.com>

Closes #1892 from liyezhang556520/lazy_memory_request and squashes the following commits:

335ab61 [Zhang, Liye] [SPARK-1777 (partial)] bugfix: make size of requested memory correctly

(cherry picked from commit 2bd8126)
Signed-off-by: Reynold Xin <rxin@apache.org>
@asfgit asfgit closed this in 2bd8126 Aug 13, 2014
@liyezhang556520 liyezhang556520 deleted the lazy_memory_request branch August 26, 2014 10:49
xiliu82 pushed a commit to xiliu82/spark that referenced this pull request Sep 4, 2014
Author: Zhang, Liye <liye.zhang@intel.com>

Closes apache#1892 from liyezhang556520/lazy_memory_request and squashes the following commits:

335ab61 [Zhang, Liye] [SPARK-1777 (partial)] bugfix: make size of requested memory correctly
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants