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

Memory leak in TypeTag cache #11767

Open
zsxwing opened this issue Oct 9, 2019 · 7 comments

Comments

@zsxwing
Copy link

commented Oct 9, 2019

We found a memory leak after upgrading from Scala 2.12.8 to Scala 2.12.10. After investigation,
we realized that the root cause is scala/scala#8112 and confirmed disabling this cache can stop the memory leak.

After digging into codes, we think there are two issues here :

  • It caches TypeTagImpl using the typeCreator's class. However, the class of typeCreator can be the same when there are multiple JavaMirrors. Hence, the cache may return a TypeTagImpl that ties to an incorrect JavaMirror (It's cached by a previous JavaMirror).
  • TypeTagImpl has a strong reference to typeTagCache (TypeTagImpl -> JavaMirror -> typeTagCache -> java.lang.ClassValue.Identity). As java.lang.ClassValue.Identity is the key type and TypeTagImpl is the value type of WeakHashMap (inside ClassValue), the current pattern is exactly WeakHashMap's doc asks to not do: Thus care should be taken to ensure that value objects do not strongly refer to their own keys, either directly or indirectly, since that will prevent the keys from being discarded.
    Right now, as long as typeCreator's class and its ClassValueMap(WeakHashMap) is not GCed, TypeTagImpl and JavaMirror will not be GCed even if there is no other place using them. As typeCreator's class can be loaded by the root class loader (e.g., it's inside a jar on the classpath), if we cache a TypeTagImpl with such TypeCreator, TypeTagImpl and JavaMirror will never be GCed.

Thanks @rednaxelafx for helping me debug this issue.

@zsxwing

This comment has been minimized.

Copy link
Author

commented Oct 9, 2019

@rednaxelafx

This comment has been minimized.

Copy link

commented Oct 9, 2019

To give a bit more context: Spark embeds a Scala REPL for ease of development / use. In some settings, a Spark driver may host multiple Scala REPLs that are dynamically created and destroyed.

When we tear down a Scala REPL from a Spark driver, we'd expect everything specific to that REPL (including the dynamically generated classes from the REPL) to be GC-able. But due to the typeTagCache's use of ClassValue potentially leaking memory due to the value referencing the key (directly or indirectly), the classes loaded by such REPLs may stay loaded forever and thus trigger OOMEs.

P.S. I'd like to ask a question about the context of the original typeTagCache feature: was it a pure performance improvement in Scala 2.12.9, or was it compensating for some other newly introduced overhead?
i.e. should we expect a performance level at least on par with 2.12.8 when we disable the typeTagCache through the conf, or should we expect performance regressions, e.g. in Scala reflection operations?

Thanks!

@retronym retronym self-assigned this Oct 9, 2019
@retronym retronym added the blocker label Oct 9, 2019
@retronym retronym added this to the 2.12.11 milestone Oct 9, 2019
@retronym

This comment has been minimized.

Copy link
Member

commented Oct 9, 2019

Thanks for the analysis.

Hence, the cache may return a TypeTagImpl that ties to an incorrect JavaMirror

Note that each JavaMirror has its own typeTagCache which I believe should avoid that problem.

was it a pure performance improvement in Scala 2.12.9, or was it compensating for some other newly introduced overhead?

It was intended as an improvement only to reduce allocations. I'll look for an alternative implementation to avoid this leak (e.g. a static field in the synthetized TypeCreator could contain a weak reference to the most recently created (JavaMirror, TypeTag).)

@zsxwing

This comment has been minimized.

Copy link
Author

commented Oct 10, 2019

Note that each JavaMirror has its own typeTagCache which I believe should avoid that problem.

Yeah, you are right. I missed that piece.

@retronym

This comment has been minimized.

Copy link
Member

commented Oct 11, 2019

I'm having a little trouble replicating this in a unit test.

Here's what I have:

import scala.reflect.runtime.universe
import scala.reflect.runtime.universe._
import scala.reflect.runtime.currentMirror

object Test {
  def main(args: Array[String]): Unit = {
    for (i <- 1 to 1024) {
      println(i)
      val settings = new scala.tools.nsc.Settings
      settings.Xnojline.value = true
      settings.usejavacp.value = true
      val intp = new scala.tools.nsc.interpreter.IMain(settings)

      try {
        intp.interpret("object C { val data = new Array[Byte](16 * 1024 * 1024) }; class C")
        intp.interpret("C.data; scala.reflect.runtime.universe.typeOf[C.type]; scala.reflect.classTag[java.util.List[String]]")
      } finally {
        intp.close()
      }
    }
  }
}

Running that with scala -J-XX:+TraceClassUnloading shows:

...
[Unloading class $line1.$eval 0x00000007c07c4a18]
[Unloading class $line1.$read 0x00000007c07c4828]
[Unloading class $line1.$read$$iw$$iw$C$ 0x00000007c07c4230]
[Unloading class $line2.$read$$iw$$iw$$typecreator1$1 0x00000007c07c4028]
[Unloading class $line2.$read$$iw$$iw$ 0x00000007c07c3e00]
[Unloading class $line2.$eval$ 0x00000007c07c3c08]
[Unloading class $line2.$eval 0x00000007c07c3a18]
[Unloading class $line2.$read 0x00000007c07c3828]
[Unloading class $line1.$read$$iw$$iw$ 0x00000007c07c6600]
[Unloading class $line1.$eval$ 0x00000007c07c6408]
[Unloading class $line1.$eval 0x00000007c07c6218]
[Unloading class $line1.$read 0x00000007c07c6028]
[Unloading class $line1.$read$$iw$$iw$C$ 0x00000007c07c5a30]
[Unloading class $line2.$read$$iw$$iw$$typecreator1$1 0x00000007c07c5828]
[Unloading class $line2.$read$$iw$$iw$ 0x00000007c07c5600]
[Unloading class $line2.$eval$ 0x00000007c07c5408]
[Unloading class $line2.$eval 0x00000007c07c5218]
[Unloading class $line2.$read 0x00000007c07c5028]
[Unloading class $line1.$read$$iw$$iw$ 0x00000007c07ce600]
[Unloading class $line1.$eval$ 0x00000007c07ce408]
[Unloading class $line1.$eval 0x00000007c07ce218]
[Unloading class $line1.$read 0x00000007c07ce028]
@zsxwing

This comment has been minimized.

Copy link
Author

commented Oct 14, 2019

Here is a reproducer:

package test

import scala.reflect.runtime.universe
import scala.reflect.runtime.universe._

class Test {

  def repo(): String = {
    val tag = implicitly[TypeTag[Array[Byte]]]
    val mirror = universe.runtimeMirror(Thread.currentThread().getContextClassLoader)
    tag.in(mirror).tpe.dealias.toString
  }
}

object Test extends Test {

  def main(args: Array[String]): Unit = {
    for (i <- 1 to 1000) {
      println(i)
      val settings = new scala.tools.nsc.Settings
      settings.Xnojline.value = true
      settings.usejavacp.value = true
      // Set this to avoid the child repl loading classes to its own class loader.
      settings.embeddedDefaults(this.getClass.getClassLoader)
      val intp = new scala.tools.nsc.interpreter.IMain(settings)

      try {
        intp.interpret("object C { val data = new Array[Byte](16 * 1024 * 1024) }")
        intp.interpret("C.data; test.Test.repo()")
      } finally {
        intp.close()
      }
    }
  }
}

I compile the above codes into a jar and use CLASSPATH=... scala test.Test to run it.

@retronym

This comment has been minimized.

Copy link
Member

commented Oct 15, 2019

@zsxwing What do you think of this solution: scala/scala#8470?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.