[SPARK-4393] Fix memory leak in ConnectionManager ACK timeout TimerTasks; use HashedWheelTimer #3259

JoshRosen · 2014-11-14T00:51:38Z

This patch is intended to fix a subtle memory leak in ConnectionManager's ACK timeout TimerTasks: in the old code, each TimerTask held a reference to the message being sent and a cancelled TimerTask won't necessarily be garbage-collected until it's scheduled to run, so this caused huge buildups of messages that weren't garbage collected until their timeouts expired, leading to OOMs.

This patch addresses this problem by capturing only the message ID in the TimerTask instead of the whole message, and by keeping a WeakReference to the promise in the TimerTask. I've also modified this code to use Netty's HashedWheelTimer, whose performance characteristics should be better for this use-case.

Thanks to @cristianopris for narrowing down this issue!

The old code caused memory leaks.

JoshRosen · 2014-11-14T00:53:20Z

/cc @andrewor14 and @rxin for review.

SparkQA · 2014-11-14T00:55:02Z

Test build #23342 has started for PR 3259 at commit 3200c33.

This patch merges cleanly.

vanzin · 2014-11-14T01:12:20Z

core/src/main/scala/org/apache/spark/network/nio/ConnectionManager.scala

@@ -913,8 +918,10 @@ private[nio] class ConnectionManager(
      }
    }

+    val timoutTaskHandle = ackTimeoutMonitor.newTimeout(timeoutTask, ackTimeout, TimeUnit.SECONDS)


vanzin · 2014-11-14T01:12:43Z

LGTM aside from typo.

SparkQA · 2014-11-14T02:23:46Z

Test build #23342 has finished for PR 3259 at commit 3200c33.

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

AmplabJenkins · 2014-11-14T02:23:49Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/23342/
Test PASSed.

rxin · 2014-11-14T08:04:09Z

LGTM too.

cristianopris · 2014-11-14T14:09:53Z

Hi Josh, thanks for looking into this.

I'm not sure this fully fixes the issue though. In my heap dump it looks like the memory leak is caused by the TimerTask holding a reference to the Promise, which would hold a reference to the message when it's set succesfully. A suggested fix would be to override cancel in the TimerTask to set the promise reference to null if the task is cancelled (and the run() method will not actually get to run).

Edit: Oh I see you also changed to HashedWheelTimer which is much more prompt in removing canceled tasks, so this must be fine. Could be worth confirming this though..

Thanks

JoshRosen · 2014-11-14T20:17:48Z

Ah, good catch regarding the promise field. I don't like the idea of relying on HashedWheelTimer in order to fix this issue; it would be better to prevent the capture of a message reference, since that's going to be a less brittle fix. I'll see if there's a clean way to refactor this so that TimerTask doesn't hold a promise reference.

Add additional documentation.

JoshRosen · 2014-11-14T22:17:21Z

@cristianopris I've updated this so that the TimerTask keeps a WeakReference to promise, which I think should address that source of memory leaks. Thanks to @andrewor14 for discussing this with me; based on our chat, I've added several clarifying comments.

SparkQA · 2014-11-14T22:17:43Z

Test build #23390 has started for PR 3259 at commit 2a2e92d.

This patch merges cleanly.

SparkQA · 2014-11-14T23:42:30Z

Test build #23390 has finished for PR 3259 at commit 2a2e92d.

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

AmplabJenkins · 2014-11-14T23:42:34Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/23390/
Test PASSed.

andrewor14 · 2014-11-14T23:53:51Z

Latest changes LGTM

rxin · 2014-11-15T06:21:40Z

core/src/main/scala/org/apache/spark/network/nio/ConnectionManager.scala

            val e = new IOException("sendMessageReliably failed because ack " +
              s"was not received within $ackTimeout sec")
-            if (!promise.tryFailure(e)) {
-              logWarning("Ignore error because promise is completed", e)
+            Option(promiseReference.get) match {


why not

val p = promiseReference.get if (p == null) { ... } else { ... }

?

I was actually on the fence about this, but your comment tips me towards the == null camp since it removes a level of nesting / indentation.

SparkQA · 2014-11-15T07:50:00Z

Test build #23420 has started for PR 3259 at commit afcc8d6.

This patch merges cleanly.

SparkQA · 2014-11-15T09:17:11Z

Test build #23420 has finished for PR 3259 at commit afcc8d6.

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

AmplabJenkins · 2014-11-15T09:17:15Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/23420/
Test PASSed.

rxin · 2014-11-16T08:44:03Z

Merging in master & branch-1.2. Thanks!

…sks; use HashedWheelTimer This patch is intended to fix a subtle memory leak in ConnectionManager's ACK timeout TimerTasks: in the old code, each TimerTask held a reference to the message being sent and a cancelled TimerTask won't necessarily be garbage-collected until it's scheduled to run, so this caused huge buildups of messages that weren't garbage collected until their timeouts expired, leading to OOMs. This patch addresses this problem by capturing only the message ID in the TimerTask instead of the whole message, and by keeping a WeakReference to the promise in the TimerTask. I've also modified this code to use Netty's HashedWheelTimer, whose performance characteristics should be better for this use-case. Thanks to cristianopris for narrowing down this issue! Author: Josh Rosen <[email protected]> Closes #3259 from JoshRosen/connection-manager-timeout-bugfix and squashes the following commits: afcc8d6 [Josh Rosen] Address rxin's review feedback. 2a2e92d [Josh Rosen] Keep only WeakReference to promise in TimerTask; 0f0913b [Josh Rosen] Spelling fix: timout => timeout 3200c33 [Josh Rosen] Use Netty HashedWheelTimer f847dd4 [Josh Rosen] Don't capture entire message in ACK timeout task. (cherry picked from commit 7850e0c) Signed-off-by: Reynold Xin <[email protected]>

sarutak · 2014-11-17T18:49:00Z

We also need to fix this issue for branch-1.1 right?

rxin · 2014-11-17T19:04:30Z

I don't think this merges cleanly into branch-1.1. Can one of you submit a pull request for that branch? Thanks.

sarutak · 2014-11-17T19:41:05Z

I opend #3321 for branch-1.1.

JoshRosen added 2 commits November 13, 2014 16:13

Don't capture entire message in ACK timeout task.

f847dd4

The old code caused memory leaks.

Use Netty HashedWheelTimer

3200c33

vanzin reviewed Nov 14, 2014
View reviewed changes

sryza mentioned this pull request Nov 14, 2014

Some improvements in SortShuffleRead jerryshao/apache-spark#4

Merged

JoshRosen added 2 commits November 14, 2014 14:11

Spelling fix: timout => timeout

0f0913b

Keep only WeakReference to promise in TimerTask;

2a2e92d

Add additional documentation.

rxin reviewed Nov 15, 2014
View reviewed changes

Address rxin's review feedback.

afcc8d6

asfgit closed this in 7850e0c Nov 16, 2014

JoshRosen mentioned this pull request Nov 29, 2014

[SPARK-4342] [Core]connection ack timeout improvement, replace Timer with ScheudledExecutor... #3207

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-4393] Fix memory leak in ConnectionManager ACK timeout TimerTasks; use HashedWheelTimer #3259

[SPARK-4393] Fix memory leak in ConnectionManager ACK timeout TimerTasks; use HashedWheelTimer #3259

JoshRosen commented Nov 14, 2014

JoshRosen commented Nov 14, 2014

SparkQA commented Nov 14, 2014

vanzin Nov 14, 2014

vanzin commented Nov 14, 2014

SparkQA commented Nov 14, 2014

AmplabJenkins commented Nov 14, 2014

rxin commented Nov 14, 2014

cristianopris commented Nov 14, 2014

JoshRosen commented Nov 14, 2014

JoshRosen commented Nov 14, 2014

SparkQA commented Nov 14, 2014

SparkQA commented Nov 14, 2014

AmplabJenkins commented Nov 14, 2014

andrewor14 commented Nov 14, 2014

rxin Nov 15, 2014

JoshRosen Nov 15, 2014

SparkQA commented Nov 15, 2014

SparkQA commented Nov 15, 2014

AmplabJenkins commented Nov 15, 2014

rxin commented Nov 16, 2014

sarutak commented Nov 17, 2014

rxin commented Nov 17, 2014

sarutak commented Nov 17, 2014

[SPARK-4393] Fix memory leak in ConnectionManager ACK timeout TimerTasks; use HashedWheelTimer #3259

[SPARK-4393] Fix memory leak in ConnectionManager ACK timeout TimerTasks; use HashedWheelTimer #3259

Conversation

JoshRosen commented Nov 14, 2014

JoshRosen commented Nov 14, 2014

SparkQA commented Nov 14, 2014

vanzin Nov 14, 2014

Choose a reason for hiding this comment

vanzin commented Nov 14, 2014

SparkQA commented Nov 14, 2014

AmplabJenkins commented Nov 14, 2014

rxin commented Nov 14, 2014

cristianopris commented Nov 14, 2014

JoshRosen commented Nov 14, 2014

JoshRosen commented Nov 14, 2014

SparkQA commented Nov 14, 2014

SparkQA commented Nov 14, 2014

AmplabJenkins commented Nov 14, 2014

andrewor14 commented Nov 14, 2014

rxin Nov 15, 2014

Choose a reason for hiding this comment

JoshRosen Nov 15, 2014

Choose a reason for hiding this comment

SparkQA commented Nov 15, 2014

SparkQA commented Nov 15, 2014

AmplabJenkins commented Nov 15, 2014

rxin commented Nov 16, 2014

sarutak commented Nov 17, 2014

rxin commented Nov 17, 2014

sarutak commented Nov 17, 2014