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

Change jsObject.value to ImmutableLinkedHashMap to mitigate hash collisions #675

Merged
merged 1 commit into from
Jan 19, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 19 additions & 11 deletions build.sbt
Original file line number Diff line number Diff line change
Expand Up @@ -234,17 +234,25 @@ lazy val `play-json` = crossProject(JVMPlatform, JSPlatform)

lazy val `play-jsonJS` = `play-json`.js

lazy val `play-jsonJVM` = `play-json`.jvm.settings(
libraryDependencies ++=
jacksons ++ {
if (isScala3.value)
specs2(scalaVersion.value).map(_.exclude("org.scala-lang.modules", "scala-xml_2.13"))
else
specs2(scalaVersion.value)
} :+ (
"ch.qos.logback" % "logback-classic" % "1.2.10" % Test
),
Test / unmanagedSourceDirectories ++= (docsP / PlayDocsKeys.scalaManualSourceDirectories).value,
lazy val `play-jsonJVM` = `play-json`.jvm
.settings(
libraryDependencies ++=
jacksons ++ {
if (isScala3.value)
specs2(scalaVersion.value).map(_.exclude("org.scala-lang.modules", "scala-xml_2.13"))
else
specs2(scalaVersion.value)
} :+ (
"ch.qos.logback" % "logback-classic" % "1.2.10" % Test
),
Test / unmanagedSourceDirectories ++= (docsP / PlayDocsKeys.scalaManualSourceDirectories).value,
)
.settings(enableJol)

def enableJol = Seq(
libraryDependencies += "org.openjdk.jol" % "jol-core" % "0.16" % Test,
Test / javaOptions += "-Djdk.attach.allowAttachSelf",
compileOrder := CompileOrder.JavaThenScala,
)

lazy val `play-json-joda` = project
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
/*
* Copyright (C) 2009-2021 Lightbend Inc. <https://www.lightbend.com>
*/

package play.api.libs.json

import org.openjdk.jol.info.GraphLayout
import org.scalatest.freespec.AnyFreeSpec
import scala.util.chaining._

class JsonMemoryFootprintSpec extends AnyFreeSpec {

"Json.parse" - {
"obj0" in assertSizes("""{}""", 32, 32)
"obj1" in assertSizes("""{"1":true}""", 168, 232)
"obj4" in assertSizes("""{"1":true,"2":true,"3":true,"4":true}""", 312, 520)

"arr0" in assertSizes("""[]""", 120, 120)
"arr1" in assertSizes("""[true]""", 120, 120)
"arr4" in assertSizes("""[true,true,true,true]""", 120, 120)

"num0" in assertSizes("""0""", 80, 80)
"num0.1" in assertSizes("""0.1""", 80, 80)
"num0.5" in assertSizes("""0.5""", 80, 80)
"numLongMax" in assertSizes(Long.MaxValue.toString, 144, 144)
"numDoubleMax" in assertSizes(Double.MaxValue.toString, 144, 144)

"true" in assertSizes("""true""", 0, 0)
"false" in assertSizes("""false""", 0, 0)
"null" in assertSizes("""null""", 0, 0)
}

"JsObject" - {
def obj(json: String) = Json.parse(json).as[JsObject]
"obj0 ++ obj0" in assertSize(obj("{}") ++ obj("{}"), 32)
"obj0 ++ obj1" in assertSize(obj("{}") ++ obj("""{"1":true}"""), 168)
"obj1 ++ obj0" in assertSize(obj("""{"1":true}""") ++ obj("""{}"""), 168)

"obj1.value" in assertSize(obj("""{"1":true}""").tap(_.value), 168)
}

"malicious" - {
// if we pack data into ~1KB of input, how much memory amplification can we achieve?
def arr1KB(elem: String, targetSize: Int = 1000): String =
Iterator.continually(elem).take(targetSize / (elem.length + 1)).mkString("[", ",", "]")
"obj0" in assertSizes(arr1KB("{}"), 12760, 12760)
"obj1" in assertSizes(arr1KB("""{"a":6}"""), 31568, 39568)
"nums" in assertSizes(arr1KB("6"), 42104, 42104)
"arr0" in assertSizes(arr1KB("[]"), 42064, 42064)
"arr1" in assertSizes(arr1KB("[6]"), 51080, 51080)
}

private def assertSizes(input: String, expected: Long, hashed: Long) = {
assertSize(Json.parse(input), expected)
withClue("After hashCode():")(
assertSize(
{
val t = Json.parse(input)
t.hashCode()
t
},
hashed
)
)
}

private def assertSize(a: => JsValue, expected: Long) = {
val layout1 = GraphLayout.parseInstance(a)
val layout2 = GraphLayout.parseInstance(a)
val distinct = layout1.subtract(layout2) // shared singletons don't count.
val clue =
s"""$a:
|${distinct.toFootprint}
|${distinct.toPrintable}""".stripMargin

withClue(clue) {
assert(distinct.totalSize() === expected)
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
/*
* Copyright (C) 2009-2021 Lightbend Inc. <https://www.lightbend.com>
*/

package play.api.libs.json

import java.util.{ LinkedHashMap => JLinkedHashMap }
import scala.collection.AbstractIterator
import scala.collection.MapFactory
import scala.collection.immutable.AbstractMap
import scala.collection.mutable

/**
* Wraps a Java LinkedHashMap as a Scala immutable.Map.
*/
private[json] class ImmutableLinkedHashMap[A, +B](underlying: JLinkedHashMap[A, B]) extends AbstractMap[A, B] {
gmethvin marked this conversation as resolved.
Show resolved Hide resolved

override def get(key: A): Option[B] = Option(underlying.get(key))

override def removed(key: A): Map[A, B] = {
val c = shallowCopy()
c.remove(key)
new ImmutableLinkedHashMap(c)
}

override def updated[V1 >: B](key: A, value: V1): Map[A, V1] = {
val c = shallowCopy[V1](size + 1)
c.put(key, value)
new ImmutableLinkedHashMap(c)
}

override def mapFactory: MapFactory[Map] = ImmutableLinkedHashMap

override def iterator: Iterator[(A, B)] = new AbstractIterator[(A, B)] {
private[this] val ui = underlying.entrySet().iterator()

override def hasNext: Boolean = ui.hasNext

override def knownSize: Int = if (underlying.isEmpty) 0 else super.knownSize

override def next(): (A, B) = {
val e = ui.next()
(e.getKey, e.getValue)
}
}

override def knownSize: Int = underlying.size()
override def size: Int = underlying.size()

private def shallowCopy[V1 >: B](sizeHint: Int = size): JLinkedHashMap[A, V1] = {
val c = new JLinkedHashMap[A, V1](sizeHint)
for ((k, v) <- this) c.put(k, v)
c
}
}

private[json] object ImmutableLinkedHashMap extends MapFactory[Map] {
private object EmptyMap extends ImmutableLinkedHashMap[Any, Nothing](new JLinkedHashMap(0))

override def empty[K, V]: Map[K, V] = EmptyMap.asInstanceOf[Map[K, V]]

override def from[K, V](it: IterableOnce[(K, V)]): Map[K, V] = (newBuilder ++= it).result()

override def newBuilder[A, B]: mutable.Builder[(A, B), Map[A, B]] = new mutable.Builder[(A, B), Map[A, B]] {
private[this] var lhm = new JLinkedHashMap[A, B](0)

override def clear(): Unit = lhm.clear()

override def sizeHint(size: Int): Unit = if (size > 0 && lhm.isEmpty) lhm = new JLinkedHashMap[A, B](size)

override def result(): Map[A, B] = {
if (lhm.isEmpty) empty
else new ImmutableLinkedHashMap(lhm)
}

override def addOne(elem: (A, B)): this.type = {
lhm.put(elem._1, elem._2)
this
}

override def addAll(xs: IterableOnce[(A, B)]): this.type = {
sizeHint(xs.knownSize)
xs.iterator.foreach(addOne)
this
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
/*
* Copyright (C) 2009-2021 Lightbend Inc. <https://www.lightbend.com>
*/

package play.api.libs.json

import java.util.{ LinkedHashMap => JLinkedHashMap }
import scala.collection.generic.ImmutableMapFactory
import scala.collection.immutable.AbstractMap
import scala.collection.immutable.Map
import scala.collection.immutable.MapLike
import scala.collection.AbstractIterator
import scala.collection.GenTraversableOnce
import scala.collection.mutable
import scala.collection.mutable.ArrayBuffer

/**
* Wraps a Java LinkedHashMap as a Scala immutable.Map.
*/
private[json] class ImmutableLinkedHashMap[A, +B](underlying: JLinkedHashMap[A, B])
extends AbstractMap[A, B]
with Map[A, B]
with MapLike[A, B, ImmutableLinkedHashMap[A, B]] {

override def get(key: A): Option[B] = Option(underlying.get(key))

override def +[V1 >: B](kv: (A, V1)): Map[A, V1] = {
val c = shallowCopy[V1]()
c.put(kv._1, kv._2)
new ImmutableLinkedHashMap(c)
}

override def ++[V1 >: B](xs: GenTraversableOnce[(A, V1)]): Map[A, V1] = {
val c = shallowCopy[V1]()
xs.foreach { case (k, v) => c.put(k, v) }
new ImmutableLinkedHashMap(c)
}

override def -(key: A) = {
val c = shallowCopy[B]()
c.remove(key)
new ImmutableLinkedHashMap[A, B](c)
}

override def iterator: Iterator[(A, B)] = new AbstractIterator[(A, B)] {
private[this] val ui = underlying.entrySet().iterator()

override def hasNext: Boolean = ui.hasNext

override def next(): (A, B) = {
val e = ui.next()
(e.getKey, e.getValue)
}
}

override def size: Int = underlying.size()

override def empty = ImmutableLinkedHashMap.empty

private def shallowCopy[V1 >: B](): JLinkedHashMap[A, V1] = {
val c = new JLinkedHashMap[A, V1](underlying.size())
for ((k, v) <- this) c.put(k, v)
c
}
}

private[json] object ImmutableLinkedHashMap extends ImmutableMapFactory[ImmutableLinkedHashMap] {
private object EmptyMap extends ImmutableLinkedHashMap[Any, Nothing](new JLinkedHashMap(0))

override def empty[A, B]: ImmutableLinkedHashMap[A, B] = EmptyMap.asInstanceOf[ImmutableLinkedHashMap[A, B]]

override def newBuilder[A, B]: mutable.Builder[(A, B), ImmutableLinkedHashMap[A, B]] = {
ArrayBuffer
.newBuilder[(A, B)]
.mapResult { buf =>
// buffering makes the size knowable resulting in a compact final hashmap
// in practice, most objects are much smaller than LHM.DEFAULT_INITIAL_CAPACITY=16.
if (buf.isEmpty) ImmutableLinkedHashMap.empty
else {
val lhm = new JLinkedHashMap[A, B](buf.size)
buf.foreach(t => lhm.put(t._1, t._2))
new ImmutableLinkedHashMap(lhm)
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -162,16 +162,17 @@ object JsPath extends JsPath(List.empty) {
}

// optimize fast path
val objectMap = JsObject.createFieldsMap()
val objectMap = ImmutableLinkedHashMap.newBuilder[String, JsValue]
objectMap.sizeHint(pathValues.size)
val isSimpleObject = pathValues.forall {
case (JsPath(KeyPathNode(key) :: Nil), value) =>
objectMap.put(key, value)
objectMap += (key -> value)
true
case _ =>
false
}
if (isSimpleObject) {
JsObject(objectMap)
JsObject(objectMap.result())
} else {
pathValues.foldLeft(JsObject.empty) { case (obj, (path, value)) =>
obj.deepMerge(buildSubPath(path, value))
Expand Down
14 changes: 6 additions & 8 deletions play-json/shared/src/main/scala/play/api/libs/json/JsValue.scala
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,7 @@ case class JsObject(
*/
lazy val value: Map[String, JsValue] = underlying match {
case m: immutable.Map[String, JsValue] => m
case m => m.toMap
case m => JsObject.createFieldsMap(m)
}

/**
Expand All @@ -157,18 +157,17 @@ case class JsObject(
/**
* Merge this object with another one. Values from other override value of the current object.
*/
def ++(other: JsObject): JsObject = JsObject(JsObject.createFieldsMap(underlying) ++= other.underlying)
def ++(other: JsObject): JsObject = JsObject(underlying ++ other.underlying)

/**
* Removes one field from the JsObject
*/
def -(otherField: String): JsObject = JsObject(JsObject.createFieldsMap(underlying) -= otherField)
def -(otherField: String): JsObject = JsObject(underlying - otherField)

/**
* Adds one field to the JsObject
*/
def +(otherField: (String, JsValue)): JsObject =
JsObject(JsObject.createFieldsMap(underlying) += otherField)
def +(otherField: (String, JsValue)): JsObject = JsObject(underlying + otherField)

/**
* merges everything in depth and doesn't stop at first level, as ++ does
Expand Down Expand Up @@ -206,9 +205,8 @@ object JsObject extends (Seq[(String, JsValue)] => JsObject) {
*
* We use this because the Java implementation better handles hash code collisions for Comparable keys.
*/
private[json] def createFieldsMap(fields: Iterable[(String, JsValue)] = Seq.empty): mutable.Map[String, JsValue] = {
import scala.collection.JavaConverters._
new java.util.LinkedHashMap[String, JsValue]().asScala ++= fields
private[json] def createFieldsMap(fields: Iterable[(String, JsValue)] = Seq.empty): immutable.Map[String, JsValue] = {
(ImmutableLinkedHashMap.newBuilder ++= fields).result()
}

/**
Expand Down
13 changes: 6 additions & 7 deletions play-json/shared/src/main/scala/play/api/libs/json/Writes.scala
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,13 @@

package play.api.libs.json

import java.util.Date
import play.api.libs.functional.ContravariantFunctor

import java.util.Date
import scala.annotation.implicitNotFound

import scala.collection._
import scala.reflect.ClassTag

import play.api.libs.functional.ContravariantFunctor

/**
* Json serializer: write an implicit to define a serializer for any type
*/
Expand Down Expand Up @@ -129,9 +127,10 @@ object OWrites extends PathWrites with ConstraintWrites {
def writeFields(fieldsMap: mutable.Map[String, JsValue], a: A): Unit

def writes(a: A): JsObject = {
val fieldsMap = JsObject.createFieldsMap()
writeFields(fieldsMap, a)
JsObject(fieldsMap)
import scala.collection.JavaConverters._
val fieldsMap = new java.util.LinkedHashMap[String, JsValue]()
writeFields(fieldsMap.asScala, a)
JsObject(new ImmutableLinkedHashMap(fieldsMap))
}
}

Expand Down