Skip to content

Commit

Permalink
Reuse buffers in pickler
Browse files Browse the repository at this point in the history
Each `Pickle` object previously contained an `index` and `entries`,
which are only used to populate the raw pickle bytes. Profiles show
this as a significant source of allocations in some runs, and since
the arrays backing these collections are held until jvm, the memory
is likely to survive for several garbage collections and be tenured
into an older space.

Since the `bytes` array is the actual result of the pickling, this
commit moves the other large (mutable!) collections to the pickler
phase factory itself, and arranges that they be cleared before use
and replaced at the end of each pickler phase (so as not to retain
the memory for longer than needed). As a sanity check, each pickle
has a field `active` which is used to assert that accesses to both
mutable fields is safe -- it's set to false when they're cleared.

The new override of `clear` in `AnyRefMap` is safe, since scala
itself currently generates calls to `AnyRefMap.clear` when asked to:

```
[nix-shell:/code/scala/sandbox]$ scala -version
Scala code runner version 2.12.9 -- Copyright 2002-2019, LAMP/EPFL and Lightbend, Inc.

[nix-shell:/code/scala/sandbox]$ cat Test.scala
object Test extends App {
  val map = new collection.mutable.AnyRefMap[String, Int]()
  map("a") = 0
  map.clear()
}

[nix-shell:/code/scala/sandbox]$ scalac Test.scala -d .

[nix-shell:/code/scala/sandbox]$ javap -c Test\$.class | grep '\.clear'
      28: invokevirtual #87                 // Method scala/collection/mutable/AnyRefMap.clear:()V
```
  • Loading branch information
hrhino committed Oct 15, 2019
1 parent a56590a commit cf477a0
Show file tree
Hide file tree
Showing 3 changed files with 106 additions and 36 deletions.
92 changes: 59 additions & 33 deletions src/compiler/scala/tools/nsc/symtab/classfile/Pickler.scala
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ package classfile

import java.lang.Float.floatToIntBits
import java.lang.Double.doubleToLongBits
import java.util.Arrays.fill

import scala.io.Codec
import scala.reflect.internal.pickling.{PickleBuffer, PickleFormat}
Expand All @@ -28,7 +29,7 @@ import scala.reflect.io.PlainFile
/**
* Serialize a top-level module and/or class.
*
* @see EntryTags.scala for symbol table attribute format.
* @see [[PickleFormat]] for symbol table attribute format.
*
* @author Martin Odersky
* @version 1.0
Expand Down Expand Up @@ -57,21 +58,21 @@ abstract class Pickler extends SubComponent {
val sym = tree.symbol
def shouldPickle(sym: Symbol) = currentRun.compiles(sym) && !currentRun.symData.contains(sym)
if (shouldPickle(sym)) {
val pickle = new Pickle(sym)
def reserveDeclEntries(sym: Symbol): Unit = {
pickle.reserveEntry(sym)
if (sym.isClass) sym.info.decls.foreach(reserveDeclEntries)
else if (sym.isModule) reserveDeclEntries(sym.moduleClass)
val pickle = initPickle(sym) { pickle =>
def reserveDeclEntries(sym: Symbol): Unit = {
pickle.reserveEntry(sym)
if (sym.isClass) sym.info.decls.foreach(reserveDeclEntries)
else if (sym.isModule) reserveDeclEntries(sym.moduleClass)
}

val companion = sym.companionSymbol.filter(_.owner == sym.owner) // exclude companionship between package- and package object-owned symbols.
val syms = sym :: (if (shouldPickle(companion)) companion :: Nil else Nil)
syms.foreach(reserveDeclEntries)
syms.foreach { sym =>
pickle.putSymbol(sym)
currentRun.symData(sym) = pickle
}
}

val companion = sym.companionSymbol.filter(_.owner == sym.owner) // exclude companionship between package- and package object-owned symbols.
val syms = sym :: (if (shouldPickle(companion)) companion :: Nil else Nil)
syms.foreach(reserveDeclEntries)
syms.foreach { sym =>
pickle.putSymbol(sym)
currentRun.symData(sym) = pickle
}
pickle.writeArray()
writeSigFile(sym, pickle)
currentRun registerPickle sym
}
Expand Down Expand Up @@ -102,7 +103,11 @@ abstract class Pickler extends SubComponent {

override def run(): Unit = {
try super.run()
finally closeSigWriter()
finally {
closeSigWriter()
_index = null
_entries = null
}
}

private def writeSigFile(sym: Symbol, pickle: PickleBuffer): Unit = {
Expand All @@ -124,14 +129,30 @@ abstract class Pickler extends SubComponent {
override protected def shouldSkipThisPhaseForJava: Boolean = !settings.YpickleJava.value
}

private class Pickle(root: Symbol) extends PickleBuffer(new Array[Byte](4096), -1, 0) {
type Index = mutable.AnyRefMap[AnyRef, Int] // a map from objects (symbols, types, names, ...) to indices into Entries
type Entries = Array[AnyRef]

final val InitEntriesSize = 256
private[this] var _index: Index = _
private[this] var _entries: Entries = _

final def initPickle(root: Symbol)(f: Pickle => Unit): Pickle = {
if (_index eq null) { _index = new Index(InitEntriesSize) }
if (_entries eq null) { _entries = new Entries(InitEntriesSize) }
val pickle = new Pickle(root, _index, _entries)
try f(pickle) finally { pickle.close(); _index.clear(); fill(_entries, null) }
pickle
}

class Pickle private[Pickler](root: Symbol, private var index: Index, private var entries: Entries)
extends PickleBuffer(new Array[Byte](4096), -1, 0) {
private val rootName = root.name.toTermName
private val rootOwner = root.owner
private var entries = new Array[AnyRef](256)
private var ep = 0
private val index = new mutable.AnyRefMap[AnyRef, Int]
private lazy val nonClassRoot = findSymbol(root.ownersIterator)(!_.isClass)

def close(): Unit = { writeArray(); index = null; entries = null }

private def isRootSym(sym: Symbol) =
sym.name.toTermName == rootName && sym.owner == rootOwner

Expand Down Expand Up @@ -177,19 +198,22 @@ abstract class Pickler extends SubComponent {
*
* @return true iff entry is new.
*/
private def putEntry(entry: AnyRef): Boolean = index.get(entry) match {
case Some(i) =>
reserved.remove(i)
case None =>
if (ep == entries.length) {
val entries1 = new Array[AnyRef](ep * 2)
System.arraycopy(entries, 0, entries1, 0, ep)
entries = entries1
}
entries(ep) = entry
index(entry) = ep
ep = ep + 1
true
private def putEntry(entry: AnyRef): Boolean = {
assert(index ne null, this)
index.get(entry) match {
case Some(i) =>
reserved.remove(i)
case None =>
if (ep == entries.length) {
val entries1 = new Array[AnyRef](ep * 2)
System.arraycopy(entries, 0, entries1, 0, ep)
entries = entries1
}
entries(ep) = entry
index(entry) = ep
ep = ep + 1
true
}
}

private def deskolemizeTypeSymbols(ref: AnyRef): AnyRef = ref match {
Expand Down Expand Up @@ -390,6 +414,7 @@ abstract class Pickler extends SubComponent {
/** Write a reference to object, i.e., the object's number in the map index.
*/
private def writeRef(ref: AnyRef) {
assert(index ne null, this)
writeNat(index(deskolemizeTypeSymbols(ref)))
}
private def writeRefs(refs: List[AnyRef]): Unit = refs foreach writeRef
Expand Down Expand Up @@ -577,8 +602,9 @@ abstract class Pickler extends SubComponent {
}

/** Write byte array */
def writeArray() {
private def writeArray() {
assert(writeIndex == 0)
assert(index ne null, this)
writeNat(MajorVersion)
writeNat(MinorVersion)
writeNat(ep)
Expand Down
9 changes: 9 additions & 0 deletions src/library/scala/collection/mutable/AnyRefMap.scala
Original file line number Diff line number Diff line change
Expand Up @@ -426,6 +426,15 @@ extends AbstractMap[K, V]
this
}

override def clear(): Unit = {
import java.util.Arrays.fill
fill(_keys, null)
fill(_values, null)
fill(_hashes, 0)
_size = 0
_vacant = 0
}

}

object AnyRefMap {
Expand Down
41 changes: 38 additions & 3 deletions test/junit/scala/collection/mutable/AnyRefMapTest.scala
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,8 @@ package scala.collection.mutable
import org.junit.runner.RunWith
import org.junit.runners.JUnit4
import org.junit.Test
import org.junit.Assert.assertTrue
import org.junit.Assert._


/* Test for scala/bug#10540 */
@RunWith(classOf[JUnit4])
class AnyRefMapTest {
@Test
Expand All @@ -20,4 +18,41 @@ class AnyRefMapTest {
assertTrue(AnyRefMap(sameHashCode -> 1) contains sameHashCode)
assertTrue(sameHashCode.hashCode == badHashCode) // Make sure test works
}
@Test
def testClear: Unit = {
val map = new AnyRefMap[String, String]()
map("greeting") = "hi"
map("farewell") = "bye"
assertEquals(2, map.size)
map.clear()
assertEquals(0, map.size)
map("greeting") = "howdy"
assertEquals(1, map.size)
map("farewell") = "auf Wiedersehen"
map("good day") = "labdien"
assertEquals(3, map.size)
}
@Test
def testClearMemoryReuse: Unit = { // otherwise there's no point to the override
val map = new AnyRefMap[String, Int]
def getField[T <: AnyRef](name: String): T =
reflect.ensureAccessible(map.getClass.getDeclaredField("scala$collection$mutable$AnyRefMap$$" + name)).get(map).asInstanceOf[T]
def hashesSz = getField[Array[Int]]("_hashes").length
def keysSz = getField[Array[AnyRef]]("_keys").length
def valuesSz = getField[Array[AnyRef]]("_values").length
def assertArraysSize(sz: Int) = {
assertEquals(sz, hashesSz)
assertEquals(sz, keysSz)
assertEquals(sz, valuesSz)
}
for (i <- (1 to 1000000)) map(i.toString) = i
assertEquals(1000000, map.size)
assertArraysSize(1 << 21)
map.clear()
assertEquals(0, map.size)
assertArraysSize(1 << 21)
for (i <- (-10000 to 10000)) map(i.toString) = i
assertEquals(20001, map.size)
assertArraysSize(1 << 21)
}
}

0 comments on commit cf477a0

Please sign in to comment.