From 5fffc5415169f2f5dca9de6e2a9d6af08b5db994 Mon Sep 17 00:00:00 2001 From: Lawrence Lin Date: Tue, 6 Oct 2026 14:25:13 -0500 Subject: [PATCH] Keep the field order in Json.newBuilder JsObjectBuilder accumulated into Map.newBuilder, so above four fields the result was backed by a Scala HashMap and lost the insertion order that JsObject.apply and Json.parse keep. Accumulate into ImmutableLinkedHashMap.newBuilder instead: the result keeps the order and takes the same memory as JsObject(fields), with no second copy. That builder handed its LinkedHashMap to the result and kept mutating it, which was fine for its internal one-shot uses but not for a public builder that can be reused. Copy the map on the first change after result(), as Scala's own map builders do, and report knownSize. Also add JsObjectBuilder.append(key, value), which adds a field without the caller allocating a tuple. The builder stays private[json]; exposing it is left for later. --- .../libs/json/JsonMemoryFootprintSpec.scala | 11 +++++ .../libs/json/ImmutableLinkedHashMap.scala | 22 ++++++++- .../play/api/libs/json/JsObjectBuilder.scala | 10 ++-- .../play/api/libs/json/JsObjectBuilder.scala | 10 ++-- .../play/api/libs/json/JsonSharedSpec.scala | 48 +++++++++++++++++++ 5 files changed, 91 insertions(+), 10 deletions(-) diff --git a/play-json/jvm/src/test/scala-3/play/api/libs/json/JsonMemoryFootprintSpec.scala b/play-json/jvm/src/test/scala-3/play/api/libs/json/JsonMemoryFootprintSpec.scala index c9166b7a6..f5c259b52 100644 --- a/play-json/jvm/src/test/scala-3/play/api/libs/json/JsonMemoryFootprintSpec.scala +++ b/play-json/jvm/src/test/scala-3/play/api/libs/json/JsonMemoryFootprintSpec.scala @@ -55,6 +55,17 @@ class JsonMemoryFootprintSpec extends AnyFreeSpec { } } + "Json.newBuilder" - { + "takes the same memory as JsObject(fields)" in { + 1.to(20).foreach { n => + val fields = 0.until(n).map(i => s"$i" -> JsTrue) + val built = footprint(fields.foldLeft(Json.newBuilder)((b, f) => b += (f._1 -> f._2)).result()) + val oneGo = footprint(JsObject(fields)) + withClue(s"$n fields:")(assert(built > 0 && built === oneGo)) + } + } + } + // By-name: each layout is diffed against a second instance, so shared singletons don't count. private def footprint(a: => JsValue): Long = GraphLayout.parseInstance(a).subtract(GraphLayout.parseInstance(a)).totalSize() diff --git a/play-json/shared/src/main/scala-2.13+/play/api/libs/json/ImmutableLinkedHashMap.scala b/play-json/shared/src/main/scala-2.13+/play/api/libs/json/ImmutableLinkedHashMap.scala index 78477326d..a04ac2e60 100644 --- a/play-json/shared/src/main/scala-2.13+/play/api/libs/json/ImmutableLinkedHashMap.scala +++ b/play-json/shared/src/main/scala-2.13+/play/api/libs/json/ImmutableLinkedHashMap.scala @@ -79,16 +79,34 @@ private[json] object ImmutableLinkedHashMap extends MapFactory[Map] { override def newBuilder[A, B]: mutable.Builder[(A, B), Map[A, B]] = new mutable.Builder[(A, B), Map[A, B]] { private var lhm = new JLinkedHashMap[A, B](0) - override def clear(): Unit = lhm.clear() + // Whether lhm backs a map returned by result(), so it must be copied before any change. + private var aliased = false + + override def clear(): Unit = { + if (aliased) { + lhm = new JLinkedHashMap[A, B](0) + aliased = false + } else lhm.clear() + } override def sizeHint(size: Int): Unit = if (size > 0 && lhm.isEmpty) lhm = new JLinkedHashMap[A, B](size) + override def knownSize: Int = lhm.size() + override def result(): Map[A, B] = { if (lhm.isEmpty) empty - else new ImmutableLinkedHashMap(lhm) + else { + aliased = true + new ImmutableLinkedHashMap(lhm) + } } override def addOne(elem: (A, B)): this.type = { + if (aliased) { + lhm = new JLinkedHashMap[A, B](lhm) + aliased = false + } + lhm.put(elem._1, elem._2) this } diff --git a/play-json/shared/src/main/scala-2.13+/play/api/libs/json/JsObjectBuilder.scala b/play-json/shared/src/main/scala-2.13+/play/api/libs/json/JsObjectBuilder.scala index a6e984372..769500efc 100644 --- a/play-json/shared/src/main/scala-2.13+/play/api/libs/json/JsObjectBuilder.scala +++ b/play-json/shared/src/main/scala-2.13+/play/api/libs/json/JsObjectBuilder.scala @@ -8,12 +8,14 @@ import scala.collection.mutable.{ Builder => MBuilder } private[json] final class JsObjectBuilder extends MBuilder[(String, Json.JsValueWrapper), JsObject] { - private val fs = Map.newBuilder[String, JsValue] + // Keeps the fields in insertion order, as JsObject.apply does. + private val fs = ImmutableLinkedHashMap.newBuilder[String, JsValue] - def addOne(elem: (String, Json.JsValueWrapper)): this.type = { - val (name, wrapped) = elem + def addOne(elem: (String, Json.JsValueWrapper)): this.type = append(elem._1, elem._2) - fs += (name -> Json.unwrap(wrapped)) + // Adds a field without the caller allocating a tuple. + def append(key: String, value: Json.JsValueWrapper): this.type = { + fs += (key -> Json.unwrap(value)) this } diff --git a/play-json/shared/src/main/scala-2.13-/play/api/libs/json/JsObjectBuilder.scala b/play-json/shared/src/main/scala-2.13-/play/api/libs/json/JsObjectBuilder.scala index 70c41d616..342273e09 100644 --- a/play-json/shared/src/main/scala-2.13-/play/api/libs/json/JsObjectBuilder.scala +++ b/play-json/shared/src/main/scala-2.13-/play/api/libs/json/JsObjectBuilder.scala @@ -8,12 +8,14 @@ import scala.collection.mutable.{ Builder => MBuilder } private[json] final class JsObjectBuilder extends MBuilder[(String, Json.JsValueWrapper), JsObject] { - private val fs = Map.newBuilder[String, JsValue] + // Keeps the fields in insertion order, as JsObject.apply does. + private val fs = ImmutableLinkedHashMap.newBuilder[String, JsValue] - def +=(elem: (String, Json.JsValueWrapper)): this.type = { - val (name, wrapped) = elem + def +=(elem: (String, Json.JsValueWrapper)): this.type = append(elem._1, elem._2) - fs += (name -> Json.unwrap(wrapped)) + // Adds a field without the caller allocating a tuple. + def append(key: String, value: Json.JsValueWrapper): this.type = { + fs += (key -> Json.unwrap(value)) this } diff --git a/play-json/shared/src/test/scala/play/api/libs/json/JsonSharedSpec.scala b/play-json/shared/src/test/scala/play/api/libs/json/JsonSharedSpec.scala index 62cb66be9..05ee3a04a 100644 --- a/play-json/shared/src/test/scala/play/api/libs/json/JsonSharedSpec.scala +++ b/play-json/shared/src/test/scala/play/api/libs/json/JsonSharedSpec.scala @@ -174,6 +174,54 @@ class JsonSharedSpec ) } + "create object using builder, keeping the field order" in { + // more fields than the specialised small immutable Maps hold + val fields = 1.to(20).map(i => s"f$i" -> JsNumber(i)) + val builder = Json.newBuilder + + fields.foreach { case (k, v) => builder += (k -> v) } + builder += ("f3" -> 30) + + val expected = JsObject(fields.updated(2, "f3" -> JsNumber(30))) + + Json.stringify(builder.result()).mustEqual(Json.stringify(expected)) + } + + "not change a built object when the builder is reused" in { + val builder = Json.newBuilder + + builder += ("a" -> 1) + builder += ("b" -> 2) + + val first = builder.result() + + builder += ("c" -> 3) + + val second = builder.result() + + builder.clear() + builder += ("d" -> 4) + + Json.stringify(first).mustEqual("""{"a":1,"b":2}""") + Json.stringify(second).mustEqual("""{"a":1,"b":2,"c":3}""") + Json.stringify(builder.result()).mustEqual("""{"d":4}""") + } + + "create object using builder append, mixed with +=" in { + val builder = new JsObjectBuilder() + + builder.append("a", 1) + builder += ("b" -> "two") + builder.append("c", Json.arr(3)) + + val first = builder.result() + + builder.append("a", 10) + + Json.stringify(first).mustEqual("""{"a":1,"b":"two","c":[3]}""") + Json.stringify(builder.result()).mustEqual("""{"a":10,"b":"two","c":[3]}""") + } + "convert to a byte array containing the UTF-8 representation" in json { js => val json = js.parse(""" |{