Skip to content
Snippets Groups Projects
Commit 94b377f9 authored by Nathan Kronenfeld's avatar Nathan Kronenfeld Committed by Josh Rosen
Browse files

[SPARK-4772] Clear local copies of accumulators as soon as we're done with them

Accumulators keep thread-local copies of themselves.  These copies were only cleared at the beginning of a task.  This meant that (a) the memory they used was tied up until the next task ran on that thread, and (b) if a thread died, the memory it had used for accumulators was locked up forever on that worker.

This PR clears the thread-local copies of accumulators at the end of each task, in the tasks finally block, to make sure they are cleaned up between tasks.  It also stores them in a ThreadLocal object, so that if, for some reason, the thread dies, any memory they are using at the time should be freed up.

Author: Nathan Kronenfeld <nkronenfeld@oculusinfo.com>

Closes #3570 from nkronenfeld/Accumulator-Improvements and squashes the following commits:

a581f3f [Nathan Kronenfeld] Change Accumulators to private[spark] instead of adding mima exclude to get around false positive in mima tests
b6c2180 [Nathan Kronenfeld] Include MiMa exclude as per build error instructions - this version incompatibility should be irrelevent, as it will only surface if a master is talking to a worker running a different version of spark.
537baad [Nathan Kronenfeld] Fuller refactoring as intended, incorporating JR's suggestions for ThreadLocal localAccums, and keeping clear(), but also calling it in tasks' finally block, rather than just at the beginning of the task.
39a82f2 [Nathan Kronenfeld] Clear local copies of accumulators as soon as we're done with them
parent f79c1cfc
No related branches found
No related tags found
No related merge requests found
...@@ -19,6 +19,7 @@ package org.apache.spark ...@@ -19,6 +19,7 @@ package org.apache.spark
import java.io.{ObjectInputStream, Serializable} import java.io.{ObjectInputStream, Serializable}
import java.util.concurrent.atomic.AtomicLong import java.util.concurrent.atomic.AtomicLong
import java.lang.ThreadLocal
import scala.collection.generic.Growable import scala.collection.generic.Growable
import scala.collection.mutable.Map import scala.collection.mutable.Map
...@@ -278,10 +279,12 @@ object AccumulatorParam { ...@@ -278,10 +279,12 @@ object AccumulatorParam {
// TODO: The multi-thread support in accumulators is kind of lame; check // TODO: The multi-thread support in accumulators is kind of lame; check
// if there's a more intuitive way of doing it right // if there's a more intuitive way of doing it right
private object Accumulators { private[spark] object Accumulators {
// TODO: Use soft references? => need to make readObject work properly then // TODO: Use soft references? => need to make readObject work properly then
val originals = Map[Long, Accumulable[_, _]]() val originals = Map[Long, Accumulable[_, _]]()
val localAccums = Map[Thread, Map[Long, Accumulable[_, _]]]() val localAccums = new ThreadLocal[Map[Long, Accumulable[_, _]]]() {
override protected def initialValue() = Map[Long, Accumulable[_, _]]()
}
var lastId: Long = 0 var lastId: Long = 0
def newId(): Long = synchronized { def newId(): Long = synchronized {
...@@ -293,22 +296,21 @@ private object Accumulators { ...@@ -293,22 +296,21 @@ private object Accumulators {
if (original) { if (original) {
originals(a.id) = a originals(a.id) = a
} else { } else {
val accums = localAccums.getOrElseUpdate(Thread.currentThread, Map()) localAccums.get()(a.id) = a
accums(a.id) = a
} }
} }
// Clear the local (non-original) accumulators for the current thread // Clear the local (non-original) accumulators for the current thread
def clear() { def clear() {
synchronized { synchronized {
localAccums.remove(Thread.currentThread) localAccums.get.clear
} }
} }
// Get the values of the local accumulators for the current thread (by ID) // Get the values of the local accumulators for the current thread (by ID)
def values: Map[Long, Any] = synchronized { def values: Map[Long, Any] = synchronized {
val ret = Map[Long, Any]() val ret = Map[Long, Any]()
for ((id, accum) <- localAccums.getOrElse(Thread.currentThread, Map())) { for ((id, accum) <- localAccums.get) {
ret(id) = accum.localValue ret(id) = accum.localValue
} }
return ret return ret
......
...@@ -172,7 +172,6 @@ private[spark] class Executor( ...@@ -172,7 +172,6 @@ private[spark] class Executor(
val startGCTime = gcTime val startGCTime = gcTime
try { try {
Accumulators.clear()
val (taskFiles, taskJars, taskBytes) = Task.deserializeWithDependencies(serializedTask) val (taskFiles, taskJars, taskBytes) = Task.deserializeWithDependencies(serializedTask)
updateDependencies(taskFiles, taskJars) updateDependencies(taskFiles, taskJars)
task = ser.deserialize[Task[Any]](taskBytes, Thread.currentThread.getContextClassLoader) task = ser.deserialize[Task[Any]](taskBytes, Thread.currentThread.getContextClassLoader)
...@@ -278,6 +277,8 @@ private[spark] class Executor( ...@@ -278,6 +277,8 @@ private[spark] class Executor(
env.shuffleMemoryManager.releaseMemoryForThisThread() env.shuffleMemoryManager.releaseMemoryForThisThread()
// Release memory used by this thread for unrolling blocks // Release memory used by this thread for unrolling blocks
env.blockManager.memoryStore.releaseUnrollMemoryForThisThread() env.blockManager.memoryStore.releaseUnrollMemoryForThisThread()
// Release memory used by this thread for accumulators
Accumulators.clear()
runningTasks.remove(taskId) runningTasks.remove(taskId)
} }
} }
......
0% Loading or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment