[SPARK-23508][CORE] Fix BlockmanagerId in case blockManagerIdCache cause oom #20667

caneGuy · 2018-02-24T11:01:45Z

… cause oom

What changes were proposed in this pull request?

blockManagerIdCache in BlockManagerId will not remove old values which may cause oom

val blockManagerIdCache = new ConcurrentHashMap[BlockManagerId, BlockManagerId]()
Since whenever we apply a new BlockManagerId, it will put into this map.

This patch will use guava cahce for blockManagerIdCache instead.

A heap dump show in SPARK-23508

How was this patch tested?

Exist tests.

… cause oom

Ngone51

At this point, I'd like to know, can we remove an id from the cache when we delete a block from the BlockManager? Does it helps? WDYT?

Ngone51 · 2018-02-24T15:34:55Z

core/src/main/scala/org/apache/spark/storage/BlockManagerId.scala

-    blockManagerIdCache.get(id)
+    val blockManagerId = blockManagerIdCache.get(id)
+    if (clearOldValues) {
+      blockManagerIdCache.clearOldValues(System.currentTimeMillis - Utils.timeStringAsMs("10d"))


10 days? I don't think time can be a judging criteria to decide whether we should remove a cached id or not, even if you set the time threshold far less/greater than '10d'. Think about a extreamly case that a block could be frequently got all the time during the app‘s running. So, it would be certainly removed from cache due to the time threshold, and recached next time we get it, and repeatedly.

@Ngone51 Thanks.i also though about remove when we delete a block.
In this case, it is history replaying which will trigger this problem,and we do not delete any block actually.
Maybe use weakreference better as @jiangxb1987 mentioned?WDYT?
Thanks again!

Can we simply use com.google.common.cache.Cache? which has a size limitation and we don't need to worry about OOM.

jiangxb1987 · 2018-02-25T12:35:40Z

core/src/main/scala/org/apache/spark/storage/BlockManagerId.scala

  }

-  val blockManagerIdCache = new ConcurrentHashMap[BlockManagerId, BlockManagerId]()
+  val blockManagerIdCache = new TimeStampedHashMap[BlockManagerId, BlockManagerId](true)


Will this cause serious performance regression? TimeStampedHashMap will copy all the old values on update. cc @cloud-fan

jiangxb1987 · 2018-02-25T12:39:46Z

Can we use WeakReference here to keep cached BlockManagerId ?

cloud-fan · 2018-02-26T07:01:43Z

Why we need this cache?

caneGuy · 2018-02-26T07:59:28Z

@cloud-fan I just find commit log below which introduce this cache
Modified StorageLevel and BlockManagerId to cache common objects and use cached object while deserializing.
I can't figure out why we need cache, since i think the cache miss ratio may be high?

…rIdCache cause oom" This reverts commit fc1b6a0.

jiangxb1987 · 2018-02-26T08:46:55Z

Had a offline discussion with @cloud-fan and we feel com.google.common.cache.Cache should be used here. You can find a example at CodeGenerator.cache.

caneGuy · 2018-02-26T08:49:37Z

Nice @jiangxb1987 @cloud-fan I will modify later.Thanks!

This reverts commit c741b58.

This reverts commit e5e3eff.

caneGuy · 2018-02-26T09:25:54Z

Update @jiangxb1987

caneGuy · 2018-02-27T05:13:28Z

core/src/main/scala/org/apache/spark/storage/BlockManagerId.scala


-  val blockManagerIdCache = new ConcurrentHashMap[BlockManagerId, BlockManagerId]()
+  val blockManagerIdCache = CacheBuilder.newBuilder()
+    .maximumSize(500)


here i set 500
since blockmanagerId about 48B per object.
I do not use spark conf since it is not convenient to get spark conf for historyserver when use BlockManagerId

I'm ok to hardcode it for now.

I‘m +1 on hardcode this, but we shall also explain in the comment the reason why this number is chosen.

Actually i think 500 executors can handle most applications.And for historyserver it is no need to cache too much BlockManagerId.If we set this number as 50 the max size of cache will below 30KB.
Agree with that? @jiangxb1987 If ok i will update documentation.

Please feel free to update any comment, and I would set the default value to 10000 since I think cost of 500KB memory is feasible.

Thanks @jiangxb1987 i have updated the comment

cloud-fan · 2018-02-27T08:23:33Z

core/src/main/scala/org/apache/spark/storage/BlockManagerId.scala

+  val blockManagerIdCache = CacheBuilder.newBuilder()
+    .maximumSize(500)
+    .build(new CacheLoader[BlockManagerId, BlockManagerId]() {
+      override def load(id: BlockManagerId) = {


nit: override def load(id: BlockManagerId) = id

cloud-fan · 2018-02-27T08:23:56Z

core/src/main/scala/org/apache/spark/storage/BlockManagerId.scala

  }

-  val blockManagerIdCache = new ConcurrentHashMap[BlockManagerId, BlockManagerId]()
+  val blockManagerIdCache = CacheBuilder.newBuilder()


is it thread-safe?

I think it is thread-safe which i refer from:
https://google.github.io/guava/releases/22.0/api/docs/com/google/common/cache/LoadingCache.html
and https://stackoverflow.com/questions/11124856/using-guava-for-high-performance-thread-safe-caching

cloud-fan · 2018-02-28T03:31:57Z

LGTM

cloud-fan · 2018-02-28T03:32:08Z

ok to test

cloud-fan · 2018-02-28T03:32:29Z

add to whitelist

jiangxb1987 · 2018-02-28T03:39:55Z

core/src/main/scala/org/apache/spark/storage/BlockManagerId.scala


-  val blockManagerIdCache = new ConcurrentHashMap[BlockManagerId, BlockManagerId]()
+  /**
+   * Here we set max cache size as 10000.Since the size of a BlockManagerId object


nit:

The max cache size is hardcoded to 10000, since the size of a BlockManagerId object is about 48B, the total memory cost should be below 1MB which is feasible.

Ngone51 · 2018-02-28T04:52:45Z

Hi, @caneGuy , sorry for my previous comment as I mixed up BlockId with BlockManagerId, and leave some wrong comments. And thanks for your reply.

Back to now, I have the same question with @cloud-fan ,

Why we need this cache?

though, we have a better cache way(guava cache) now.

My confusions:

It is weird that we need to create a BlockManagerId before we get a same one from the cache.
And on executor side, when BlockManagerId registered to master and return with an updated BlockManagerId , the new BlockManagerId does not be updated to blockManagerIdCache. So, it seems executor side's BlockManagerId has little relevance with blockManagerIdCache.

jiangxb1987 · 2018-02-28T05:01:50Z

In case the same BlockManagerId being created multiple times, this cache will ensure we always use the first one that is created, which make it possible for the rest BlockManagerId instances being recycled shortly. The downside is we have to persist all the distinct BlockManagerId created.

Since the code is added long times ago, and it's actually hard to examine the performance with/without the cache, we'd like to keep it for now.

Ngone51 · 2018-02-28T05:10:55Z

Hi, @jiangxb1987 , thanks for your kindly explanation.

SparkQA · 2018-02-28T07:21:55Z

Test build #87746 has finished for PR 20667 at commit 3379899.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2018-02-28T07:34:16Z

Test build #87748 has finished for PR 20667 at commit bf79f4d.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2018-02-28T08:05:02Z

Test build #87744 has finished for PR 20667 at commit 3379899.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

cloud-fan · 2018-02-28T08:08:39Z

retest this please

SparkQA · 2018-02-28T12:04:56Z

Test build #87761 has finished for PR 20667 at commit bf79f4d.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

kiszk · 2018-02-28T13:12:38Z

LGTM if we need caching.

…use oom … cause oom ## What changes were proposed in this pull request? blockManagerIdCache in BlockManagerId will not remove old values which may cause oom `val blockManagerIdCache = new ConcurrentHashMap[BlockManagerId, BlockManagerId]()` Since whenever we apply a new BlockManagerId, it will put into this map. This patch will use guava cahce for blockManagerIdCache instead. A heap dump show in [SPARK-23508](https://issues.apache.org/jira/browse/SPARK-23508) ## How was this patch tested? Exist tests. Author: zhoukang <[email protected]> Closes #20667 from caneGuy/zhoukang/fix-history. (cherry picked from commit 6a8abe2) Signed-off-by: Wenchen Fan <[email protected]>

cloud-fan · 2018-02-28T15:17:23Z

thanks, merging to master/2.3/2.2!

…use oom … cause oom ## What changes were proposed in this pull request? blockManagerIdCache in BlockManagerId will not remove old values which may cause oom `val blockManagerIdCache = new ConcurrentHashMap[BlockManagerId, BlockManagerId]()` Since whenever we apply a new BlockManagerId, it will put into this map. This patch will use guava cahce for blockManagerIdCache instead. A heap dump show in [SPARK-23508](https://issues.apache.org/jira/browse/SPARK-23508) ## How was this patch tested? Exist tests. Author: zhoukang <[email protected]> Closes #20667 from caneGuy/zhoukang/fix-history. (cherry picked from commit 6a8abe2) Signed-off-by: Wenchen Fan <[email protected]>

caneGuy · 2018-02-28T15:27:15Z

Thanks @cloud-fan @jiangxb1987 @kiszk @Ngone51

…use oom … cause oom ## What changes were proposed in this pull request? blockManagerIdCache in BlockManagerId will not remove old values which may cause oom `val blockManagerIdCache = new ConcurrentHashMap[BlockManagerId, BlockManagerId]()` Since whenever we apply a new BlockManagerId, it will put into this map. This patch will use guava cahce for blockManagerIdCache instead. A heap dump show in [SPARK-23508](https://issues.apache.org/jira/browse/SPARK-23508) ## How was this patch tested? Exist tests. Author: zhoukang <[email protected]> Closes apache#20667 from caneGuy/zhoukang/fix-history. (cherry picked from commit 6a8abe2) Signed-off-by: Wenchen Fan <[email protected]>

Use timeStampedHashMap for BlockmanagerId in case blockManagerIdCache…

fc1b6a0

… cause oom

Ngone51 reviewed Feb 24, 2018

View reviewed changes

jiangxb1987 reviewed Feb 25, 2018

View reviewed changes

caneGuy added 3 commits February 26, 2018 16:20

Revert "Use timeStampedHashMap for BlockmanagerId in case blockManage…

59f34e1

…rIdCache cause oom" This reverts commit fc1b6a0.

Use softreference instead

e5e3eff

Remove unused import

c741b58

caneGuy changed the title ~~[SPARK-23508][CORE] Use timeStampedHashMap for BlockmanagerId in case blockManagerIdCache…~~ [SPARK-23508][CORE] Use softreference for BlockmanagerId in case blockManagerIdCache… Feb 26, 2018

caneGuy added 3 commits February 26, 2018 16:54

Revert "Remove unused import"

36d66e1

This reverts commit c741b58.

Revert "Use softreference instead"

e5b793d

This reverts commit e5e3eff.

Use guava cache instead

8f3cd47

caneGuy changed the title ~~[SPARK-23508][CORE] Use softreference for BlockmanagerId in case blockManagerIdCache…~~ [SPARK-23508][CORE] Fix BlockmanagerId in case blockManagerIdCache cause oom Feb 26, 2018

Update

dc522fc

caneGuy commented Feb 27, 2018

View reviewed changes

cloud-fan reviewed Feb 27, 2018

View reviewed changes

caneGuy added 2 commits February 27, 2018 17:41

Refine comment

b28985b

Update documentation

3379899

jiangxb1987 reviewed Feb 28, 2018

View reviewed changes

Refine comment

bf79f4d

asfgit closed this in 6a8abe2 Feb 28, 2018

caneGuy deleted the zhoukang/fix-history branch March 1, 2018 06:03

[SPARK-23508][CORE] Fix BlockmanagerId in case blockManagerIdCache cause oom #20667

[SPARK-23508][CORE] Fix BlockmanagerId in case blockManagerIdCache cause oom #20667

Uh oh!

Conversation

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

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

Ngone51 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!

jiangxb1987 commented Feb 25, 2018

Uh oh!

cloud-fan commented Feb 26, 2018

Uh oh!

caneGuy commented Feb 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jiangxb1987 commented Feb 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

caneGuy commented Feb 26, 2018

Uh oh!

caneGuy commented Feb 26, 2018

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!

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!

cloud-fan commented Feb 28, 2018

Uh oh!

cloud-fan commented Feb 28, 2018

Uh oh!

cloud-fan commented Feb 28, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ngone51 commented Feb 28, 2018

Uh oh!

jiangxb1987 commented Feb 28, 2018

Uh oh!

Ngone51 commented Feb 28, 2018

Uh oh!

SparkQA commented Feb 28, 2018

Uh oh!

SparkQA commented Feb 28, 2018

Uh oh!

SparkQA commented Feb 28, 2018

Uh oh!

cloud-fan commented Feb 28, 2018

Uh oh!

SparkQA commented Feb 28, 2018

Uh oh!

kiszk commented Feb 28, 2018

Uh oh!

cloud-fan commented Feb 28, 2018

Uh oh!

caneGuy commented Feb 24, 2018 •

edited

Loading

caneGuy commented Feb 26, 2018 •

edited

Loading

jiangxb1987 commented Feb 26, 2018 •

edited

Loading