From 1e357087791be916dbd18e6832c1cddcdb61cb1e Mon Sep 17 00:00:00 2001 From: Josh Rosen Date: Fri, 3 Jan 2025 11:01:22 -0800 Subject: [PATCH 01/14] Optimize key lookup / visibility checks for super-free objects. --- sjsonnet/src/sjsonnet/Val.scala | 41 ++++++++++++++++++++++++++------- 1 file changed, 33 insertions(+), 8 deletions(-) diff --git a/sjsonnet/src/sjsonnet/Val.scala b/sjsonnet/src/sjsonnet/Val.scala index 9e02311d..2b423418 100644 --- a/sjsonnet/src/sjsonnet/Val.scala +++ b/sjsonnet/src/sjsonnet/Val.scala @@ -221,18 +221,43 @@ object Val{ allKeys } - @inline def hasKeys = !getAllKeys.isEmpty + @inline def hasKeys: Boolean = { + val m = if (static || `super` != null) getAllKeys else value0 + !m.isEmpty + } - @inline def containsKey(k: String): Boolean = getAllKeys.containsKey(k) + @inline def containsKey(k: String): Boolean = { + val m = if (static || `super` != null) getAllKeys else value0 + m.containsKey(k) + } - @inline def containsVisibleKey(k: String): Boolean = getAllKeys.get(k) == java.lang.Boolean.FALSE + @inline def containsVisibleKey(k: String): Boolean = { + if (static || `super` != null) { + getAllKeys.get(k) == java.lang.Boolean.FALSE + } else { + val m = value0.get(k) + m != null && (m.visibility != Visibility.Hidden) + } + } - lazy val allKeyNames: Array[String] = getAllKeys.keySet().toArray(new Array[String](getAllKeys.size())) + lazy val allKeyNames: Array[String] = { + val m = if (static || `super` != null) getAllKeys else value0 + m.keySet().toArray(new Array[String](m.size())) + } - lazy val visibleKeyNames: Array[String] = if(static) allKeyNames else { - val buf = mutable.ArrayBuilder.make[String] - getAllKeys.forEach((k, b) => if(b == java.lang.Boolean.FALSE) buf += k) - buf.result() + lazy val visibleKeyNames: Array[String] = { + if (static) { + allKeyNames + } else { + val buf = new mutable.ArrayBuilder.ofRef[String] + if (`super` == null) { + buf.sizeHint(Math.min(value0.size(), 16)) + value0.forEach((k, m) => if (m.visibility != Visibility.Hidden) buf += k) + } else { + getAllKeys.forEach((k, b) => if (b == java.lang.Boolean.FALSE) buf += k) + } + buf.result() + } } def value(k: String, From 5e531ef9d1368a2109b0884fac99dea2ac8d414c Mon Sep 17 00:00:00 2001 From: Josh Rosen Date: Fri, 3 Jan 2025 11:25:57 -0800 Subject: [PATCH 02/14] Throw errors on more cases we should never hit --- sjsonnet/src/sjsonnet/Evaluator.scala | 1 + sjsonnet/src/sjsonnet/Val.scala | 3 ++- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/sjsonnet/src/sjsonnet/Evaluator.scala b/sjsonnet/src/sjsonnet/Evaluator.scala index 48213291..106b374c 100644 --- a/sjsonnet/src/sjsonnet/Evaluator.scala +++ b/sjsonnet/src/sjsonnet/Evaluator.scala @@ -604,6 +604,7 @@ class Evaluator(resolver: CachedResolver, builder.put(k, v) } case _ => + Error.fail("This case should never be hit", objPos) } cachedObj = new Val.Obj(objPos, builder, false, if(asserts != null) assertions else null, sup) cachedObj diff --git a/sjsonnet/src/sjsonnet/Val.scala b/sjsonnet/src/sjsonnet/Val.scala index 2b423418..23bc3117 100644 --- a/sjsonnet/src/sjsonnet/Val.scala +++ b/sjsonnet/src/sjsonnet/Val.scala @@ -382,7 +382,8 @@ object Val{ allKeys.put(uniqueKey, false) keys(idx) = uniqueKey idx += 1 - case _ => + case other => + throw new Error(s"Unexpected non-literal field in static object: ${other} of class ${other.getClass}") } val fieldSet = new StaticObjectFieldSet(keys) new Val.Obj(pos, null, true, null, null, cache, internedKeyMaps.getOrElseUpdate(fieldSet, allKeys)) From 9fa504ab4a0d126f5acc108d2d34a33079e7ac34 Mon Sep 17 00:00:00 2001 From: Josh Rosen Date: Fri, 3 Jan 2025 12:40:13 -0800 Subject: [PATCH 03/14] Manual unroll of for comprehensions in mergePatch --- sjsonnet/src/sjsonnet/Std.scala | 56 ++++++++++++++++++++++----------- 1 file changed, 38 insertions(+), 18 deletions(-) diff --git a/sjsonnet/src/sjsonnet/Std.scala b/sjsonnet/src/sjsonnet/Std.scala index 58774e49..c4548687 100644 --- a/sjsonnet/src/sjsonnet/Std.scala +++ b/sjsonnet/src/sjsonnet/Std.scala @@ -927,31 +927,51 @@ class Std(private val additionalNativeFunctions: Map[String, Val.Builtin] = Map. } def recPair(l: Val, r: Val): Val = (l, r) match{ case (l: Val.Obj, r: Val.Obj) => - val kvs = for { - k <- (l.visibleKeyNames ++ r.visibleKeyNames).distinct - lValue = if (l.containsVisibleKey(k)) Option(l.valueRaw(k, l, pos)(ev)) else None - rValue = if (r.containsVisibleKey(k)) Option(r.valueRaw(k, r, pos)(ev)) else None - if !rValue.exists(_.isInstanceOf[Val.Null]) - } yield (lValue, rValue) match{ - case (Some(lChild), None) => k -> createMember{lChild} - case (Some(lChild: Val.Obj), Some(rChild: Val.Obj)) => k -> createMember{recPair(lChild, rChild)} - case (_, Some(rChild)) => k -> createMember{recSingle(rChild)} - case (None, None) => Error.fail("std.mergePatch: This should never happen") + val keys: Array[String] = (l.visibleKeyNames ++ r.visibleKeyNames).distinct + val kvs: Array[(String, Val.Obj.Member)] = new Array[(String, Val.Obj.Member)](keys.length) + var kvsIdx = 0 + var i = 0 + while (i < keys.length) { + val key = keys(i) + val lValue = if (l.containsVisibleKey(key)) l.valueRaw(key, l, pos)(ev) else null + val rValue = if (r.containsVisibleKey(key)) r.valueRaw(key, r, pos)(ev) else null + if (rValue == null || !rValue.isInstanceOf[Val.Null]) { + if (lValue != null && rValue == null) { + kvs(kvsIdx) = (key, new Val.Obj.ConstMember(false, Visibility.Normal, lValue)) + } else if (lValue.isInstanceOf[Val.Obj] && rValue.isInstanceOf[Val.Obj]) { + kvs(kvsIdx) = (key, createMember(recPair(lValue, rValue))) + } else if (rValue != null) { + kvs(kvsIdx) = (key, createMember(recSingle(rValue))) + } else { + Error.fail("std.mergePatch: This should never happen") + } + kvsIdx += 1 + } + i += 1 } - Val.Obj.mk(mergePosition, kvs:_*) + val trimmedKvs = if (kvsIdx == i) kvs else kvs.slice(0, kvsIdx) + Val.Obj.mk(mergePosition, trimmedKvs:_*) case (_, _) => recSingle(r) } def recSingle(v: Val): Val = v match{ case obj: Val.Obj => - val kvs = for{ - k <- obj.visibleKeyNames - value = obj.value(k, pos, obj)(ev) - if !value.isInstanceOf[Val.Null] - } yield (k, createMember{recSingle(value)}) - - Val.Obj.mk(obj.pos, kvs:_*) + val keys: Array[String] = obj.visibleKeyNames + val kvs: Array[(String, Val.Obj.Member)] = new Array[(String, Val.Obj.Member)](keys.length) + var kvsIdx = 0 + var i = 0 + while (i < keys.length) { + val key = keys(i) + val value = obj.value(key, pos, obj)(ev) + if (!value.isInstanceOf[Val.Null]) { + kvs(kvsIdx) = (key, createMember(recSingle(value))) + kvsIdx += 1 + } + i += 1 + } + val trimmedKvs = if (kvsIdx == i) kvs else kvs.slice(0, kvsIdx) + Val.Obj.mk(obj.pos, trimmedKvs:_*) case _ => v } From a19b92eccd3b905db60ccf14e83accf3ae887ba4 Mon Sep 17 00:00:00 2001 From: Josh Rosen Date: Fri, 22 Nov 2024 16:47:56 -0800 Subject: [PATCH 04/14] Factor existing pre-sizing code into helper function. --- sjsonnet/src/sjsonnet/Util.scala | 11 ++++++++++- sjsonnet/src/sjsonnet/Val.scala | 7 +------ 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/sjsonnet/src/sjsonnet/Util.scala b/sjsonnet/src/sjsonnet/Util.scala index c6e87885..91efaa4c 100644 --- a/sjsonnet/src/sjsonnet/Util.scala +++ b/sjsonnet/src/sjsonnet/Util.scala @@ -52,4 +52,13 @@ object Util{ s"<$s>" } } -} \ No newline at end of file + + def preSizedJavaLinkedHashMap[K, V](expectedElems: Int): java.util.LinkedHashMap[K, V] = { + // Set the initial capacity to the number of elems divided by the default load factor + 1 + // this ensures that we can fill up the map to the total number of fields without resizing. + // From JavaDoc - true for both Scala & Java HashMaps + val hashMapDefaultLoadFactor = 0.75f + val capacity = (expectedElems / hashMapDefaultLoadFactor).toInt + 1 + new java.util.LinkedHashMap[K, V](capacity, hashMapDefaultLoadFactor) + } +} diff --git a/sjsonnet/src/sjsonnet/Val.scala b/sjsonnet/src/sjsonnet/Val.scala index 23bc3117..1b746a23 100644 --- a/sjsonnet/src/sjsonnet/Val.scala +++ b/sjsonnet/src/sjsonnet/Val.scala @@ -366,13 +366,8 @@ object Val{ fields: Array[Expr.Member.Field], internedKeyMaps: mutable.HashMap[StaticObjectFieldSet, java.util.LinkedHashMap[String, java.lang.Boolean]], internedStrings: mutable.HashMap[String, String]): Obj = { - // Set the initial capacity to the number of fields divided by the default load factor + 1 - - // this ensures that we can fill up the map to the total number of fields without resizing. - // From JavaDoc - true for both Scala & Java HashMaps - val hashMapDefaultLoadFactor = 0.75f - val capacity = (fields.length / hashMapDefaultLoadFactor).toInt + 1 val cache = mutable.HashMap.empty[Any, Val] - val allKeys = new util.LinkedHashMap[String, java.lang.Boolean](capacity, hashMapDefaultLoadFactor) + val allKeys = Util.preSizedJavaLinkedHashMap[String, java.lang.Boolean](fields.length) val keys = new Array[String](fields.length) var idx = 0 fields.foreach { From 33145298d181fa9ad7a659651589866e6f85caf7 Mon Sep 17 00:00:00 2001 From: Josh Rosen Date: Fri, 22 Nov 2024 17:26:25 -0800 Subject: [PATCH 05/14] Presize more call sites. --- sjsonnet/src/sjsonnet/Evaluator.scala | 2 +- sjsonnet/src/sjsonnet/Std.scala | 2 +- sjsonnet/src/sjsonnet/Util.scala | 6 ++++++ sjsonnet/src/sjsonnet/Val.scala | 6 +++--- 4 files changed, 11 insertions(+), 5 deletions(-) diff --git a/sjsonnet/src/sjsonnet/Evaluator.scala b/sjsonnet/src/sjsonnet/Evaluator.scala index 106b374c..62079a61 100644 --- a/sjsonnet/src/sjsonnet/Evaluator.scala +++ b/sjsonnet/src/sjsonnet/Evaluator.scala @@ -579,7 +579,7 @@ class Evaluator(resolver: CachedResolver, newScope } - val builder = new java.util.LinkedHashMap[String, Val.Obj.Member] + val builder = Util.preSizedJavaLinkedHashMap[String, Val.Obj.Member](fields.length) fields.foreach { case Member.Field(offset, fieldName, plus, null, sep, rhs) => val k = visitFieldName(fieldName, offset) diff --git a/sjsonnet/src/sjsonnet/Std.scala b/sjsonnet/src/sjsonnet/Std.scala index c4548687..3f399d84 100644 --- a/sjsonnet/src/sjsonnet/Std.scala +++ b/sjsonnet/src/sjsonnet/Std.scala @@ -381,7 +381,7 @@ class Std(private val additionalNativeFunctions: Map[String, Val.Builtin] = Map. val func = _func.asFunc val obj = _obj.asObj val allKeys = obj.allKeyNames - val m = new util.LinkedHashMap[String, Val.Obj.Member]() + val m = Util.preSizedJavaLinkedHashMap[String, Val.Obj.Member](allKeys.length) var i = 0 while(i < allKeys.length) { val k = allKeys(i) diff --git a/sjsonnet/src/sjsonnet/Util.scala b/sjsonnet/src/sjsonnet/Util.scala index 91efaa4c..87237d95 100644 --- a/sjsonnet/src/sjsonnet/Util.scala +++ b/sjsonnet/src/sjsonnet/Util.scala @@ -61,4 +61,10 @@ object Util{ val capacity = (expectedElems / hashMapDefaultLoadFactor).toInt + 1 new java.util.LinkedHashMap[K, V](capacity, hashMapDefaultLoadFactor) } + + def preSizedScalaMutableHashMap[K, V](expectedElems: Int): scala.collection.mutable.HashMap[K, V] = { + val hashMapDefaultLoadFactor = 0.75f + val capacity = (expectedElems / hashMapDefaultLoadFactor).toInt + 1 + new scala.collection.mutable.HashMap[K, V](capacity, hashMapDefaultLoadFactor) + } } diff --git a/sjsonnet/src/sjsonnet/Val.scala b/sjsonnet/src/sjsonnet/Val.scala index 1b746a23..49ab7600 100644 --- a/sjsonnet/src/sjsonnet/Val.scala +++ b/sjsonnet/src/sjsonnet/Val.scala @@ -154,7 +154,7 @@ object Val{ } def mk(pos: Position, members: (String, Obj.Member)*): Obj = { - val m = new util.LinkedHashMap[String, Obj.Member]() + val m = Util.preSizedJavaLinkedHashMap[String, Obj.Member](members.length) for((k, v) <- members) m.put(k, v) new Obj(pos, m, false, null, null) } @@ -173,7 +173,7 @@ object Val{ private[this] def getValue0: util.LinkedHashMap[String, Obj.Member] = { if(value0 == null) { - val value0 = new java.util.LinkedHashMap[String, Val.Obj.Member] + val value0 = Util.preSizedJavaLinkedHashMap[String, Val.Obj.Member](allKeys.size()) allKeys.forEach { (k, _) => value0.put(k, new Val.Obj.ConstMember(false, Visibility.Normal, valueCache(k))) } @@ -366,7 +366,7 @@ object Val{ fields: Array[Expr.Member.Field], internedKeyMaps: mutable.HashMap[StaticObjectFieldSet, java.util.LinkedHashMap[String, java.lang.Boolean]], internedStrings: mutable.HashMap[String, String]): Obj = { - val cache = mutable.HashMap.empty[Any, Val] + val cache = Util.preSizedScalaMutableHashMap[Any, Val](fields.length) val allKeys = Util.preSizedJavaLinkedHashMap[String, java.lang.Boolean](fields.length) val keys = new Array[String](fields.length) var idx = 0 From e6ecc8e7b58314bc35f32c99e6dc84ef33ed9878 Mon Sep 17 00:00:00 2001 From: Josh Rosen Date: Fri, 22 Nov 2024 18:09:07 -0800 Subject: [PATCH 06/14] Pre-size value caches for objects without super objects. --- sjsonnet/src/sjsonnet/Evaluator.scala | 14 ++++++++++++-- sjsonnet/src/sjsonnet/Std.scala | 3 ++- sjsonnet/src/sjsonnet/Val.scala | 12 ++++++++++++ 3 files changed, 26 insertions(+), 3 deletions(-) diff --git a/sjsonnet/src/sjsonnet/Evaluator.scala b/sjsonnet/src/sjsonnet/Evaluator.scala index 62079a61..91a75b8b 100644 --- a/sjsonnet/src/sjsonnet/Evaluator.scala +++ b/sjsonnet/src/sjsonnet/Evaluator.scala @@ -606,7 +606,12 @@ class Evaluator(resolver: CachedResolver, case _ => Error.fail("This case should never be hit", objPos) } - cachedObj = new Val.Obj(objPos, builder, false, if(asserts != null) assertions else null, sup) + val valueCache = if (sup == null) { + Val.Obj.getEmptyValueCacheForObjWithoutSuper(fields.length) + } else { + mutable.HashMap.empty[Any, Val] + } + cachedObj = new Val.Obj(objPos, builder, false, if(asserts != null) assertions else null, sup, valueCache) cachedObj } @@ -637,7 +642,12 @@ class Evaluator(resolver: CachedResolver, case _ => } } - new Val.Obj(e.pos, builder, false, null, sup) + val valueCache = if (sup == null) { + Val.Obj.getEmptyValueCacheForObjWithoutSuper(builder.size()) + } else { + mutable.HashMap.empty[Any, Val] + } + new Val.Obj(e.pos, builder, false, null, sup, valueCache) } newSelf diff --git a/sjsonnet/src/sjsonnet/Std.scala b/sjsonnet/src/sjsonnet/Std.scala index 3f399d84..6652bdc9 100644 --- a/sjsonnet/src/sjsonnet/Std.scala +++ b/sjsonnet/src/sjsonnet/Std.scala @@ -392,7 +392,8 @@ class Std(private val additionalNativeFunctions: Map[String, Val.Builtin] = Map. m.put(k, v) i += 1 } - new Val.Obj(pos, m, false, null, null) + val valueCache = Val.Obj.getEmptyValueCacheForObjWithoutSuper(allKeys.length) + new Val.Obj(pos, m, false, null, null, valueCache) } } diff --git a/sjsonnet/src/sjsonnet/Val.scala b/sjsonnet/src/sjsonnet/Val.scala index 49ab7600..e05881e8 100644 --- a/sjsonnet/src/sjsonnet/Val.scala +++ b/sjsonnet/src/sjsonnet/Val.scala @@ -144,6 +144,18 @@ object Val{ object Obj{ + def getEmptyValueCacheForObjWithoutSuper(numFields: Int): mutable.HashMap[Any, Val] = { + // Helper for saving space in valueCache for objects without a super object. + // For objects with no super, we (cheaply) know the exact number of fields and + // therefore can upper bound the number of fields that _might_ be computed. + // We only want to pre-size if it yields a smaller initial map size than the default. + if (numFields >= 12) { + scala.collection.mutable.HashMap.empty[Any, Val] + } else { + Util.preSizedScalaMutableHashMap[Any, Val](numFields) + } + } + abstract class Member(val add: Boolean, val visibility: Visibility, val cached: Boolean = true) { def invoke(self: Obj, sup: Obj, fs: FileScope, ev: EvalScope): Val } From 21704f2cc2664c97047744d156e287eea80baab9 Mon Sep 17 00:00:00 2001 From: Josh Rosen Date: Fri, 3 Jan 2025 12:55:27 -0800 Subject: [PATCH 07/14] Make valueCache into a Java HashMap. --- sjsonnet/src/sjsonnet/Evaluator.scala | 4 ++-- sjsonnet/src/sjsonnet/Util.scala | 4 ++-- sjsonnet/src/sjsonnet/Val.scala | 29 ++++++++++++++------------ sjsonnet/src/sjsonnet/ValVisitor.scala | 2 +- 4 files changed, 21 insertions(+), 18 deletions(-) diff --git a/sjsonnet/src/sjsonnet/Evaluator.scala b/sjsonnet/src/sjsonnet/Evaluator.scala index 91a75b8b..765705e8 100644 --- a/sjsonnet/src/sjsonnet/Evaluator.scala +++ b/sjsonnet/src/sjsonnet/Evaluator.scala @@ -609,7 +609,7 @@ class Evaluator(resolver: CachedResolver, val valueCache = if (sup == null) { Val.Obj.getEmptyValueCacheForObjWithoutSuper(fields.length) } else { - mutable.HashMap.empty[Any, Val] + new java.util.HashMap[Any, Val]() } cachedObj = new Val.Obj(objPos, builder, false, if(asserts != null) assertions else null, sup, valueCache) cachedObj @@ -645,7 +645,7 @@ class Evaluator(resolver: CachedResolver, val valueCache = if (sup == null) { Val.Obj.getEmptyValueCacheForObjWithoutSuper(builder.size()) } else { - mutable.HashMap.empty[Any, Val] + new java.util.HashMap[Any, Val]() } new Val.Obj(e.pos, builder, false, null, sup, valueCache) } diff --git a/sjsonnet/src/sjsonnet/Util.scala b/sjsonnet/src/sjsonnet/Util.scala index 87237d95..7fe73827 100644 --- a/sjsonnet/src/sjsonnet/Util.scala +++ b/sjsonnet/src/sjsonnet/Util.scala @@ -62,9 +62,9 @@ object Util{ new java.util.LinkedHashMap[K, V](capacity, hashMapDefaultLoadFactor) } - def preSizedScalaMutableHashMap[K, V](expectedElems: Int): scala.collection.mutable.HashMap[K, V] = { + def preSizedJavaHashMap[K, V](expectedElems: Int): java.util.HashMap[K, V] = { val hashMapDefaultLoadFactor = 0.75f val capacity = (expectedElems / hashMapDefaultLoadFactor).toInt + 1 - new scala.collection.mutable.HashMap[K, V](capacity, hashMapDefaultLoadFactor) + new java.util.HashMap[K, V](capacity, hashMapDefaultLoadFactor) } } diff --git a/sjsonnet/src/sjsonnet/Val.scala b/sjsonnet/src/sjsonnet/Val.scala index e05881e8..a6df28da 100644 --- a/sjsonnet/src/sjsonnet/Val.scala +++ b/sjsonnet/src/sjsonnet/Val.scala @@ -144,15 +144,15 @@ object Val{ object Obj{ - def getEmptyValueCacheForObjWithoutSuper(numFields: Int): mutable.HashMap[Any, Val] = { + def getEmptyValueCacheForObjWithoutSuper(numFields: Int): util.HashMap[Any, Val] = { // Helper for saving space in valueCache for objects without a super object. // For objects with no super, we (cheaply) know the exact number of fields and // therefore can upper bound the number of fields that _might_ be computed. // We only want to pre-size if it yields a smaller initial map size than the default. if (numFields >= 12) { - scala.collection.mutable.HashMap.empty[Any, Val] + new util.HashMap[Any, Val]() } else { - Util.preSizedScalaMutableHashMap[Any, Val](numFields) + Util.preSizedJavaHashMap[Any, Val](numFields) } } @@ -177,7 +177,7 @@ object Val{ static: Boolean, triggerAsserts: Val.Obj => Unit, `super`: Obj, - valueCache: mutable.HashMap[Any, Val] = mutable.HashMap.empty[Any, Val], + valueCache: util.HashMap[Any, Val] = new util.HashMap[Any, Val](), private[this] var allKeys: util.LinkedHashMap[String, java.lang.Boolean] = null) extends Literal with Expr.ObjBody { var asserting: Boolean = false @@ -187,7 +187,7 @@ object Val{ if(value0 == null) { val value0 = Util.preSizedJavaLinkedHashMap[String, Val.Obj.Member](allKeys.size()) allKeys.forEach { (k, _) => - value0.put(k, new Val.Obj.ConstMember(false, Visibility.Normal, valueCache(k))) + value0.put(k, new Val.Obj.ConstMember(false, Visibility.Normal, valueCache.get(k))) } // Only assign to field after initialization is complete to allow unsynchronized multi-threaded use: this.value0 = value0 @@ -277,18 +277,21 @@ object Val{ self: Obj = this) (implicit evaluator: EvalScope): Val = { if(static) { - valueCache.getOrElse(k, null) match { + valueCache.get(k) match { case null => Error.fail("Field does not exist: " + k, pos) case x => x } } else { val cacheKey = if(self eq this) k else (k, self) - valueCache.getOrElse(cacheKey, { + val cachedValue = valueCache.get(cacheKey) + if (cachedValue != null) { + cachedValue + } else { valueRaw(k, self, pos, valueCache, cacheKey) match { case null => Error.fail("Field does not exist: " + k, pos) case x => x } - }) + } } } @@ -323,12 +326,12 @@ object Val{ def valueRaw(k: String, self: Obj, pos: Position, - addTo: mutable.HashMap[Any, Val] = null, + addTo: util.HashMap[Any, Val] = null, addKey: Any = null) (implicit evaluator: EvalScope): Val = { if(static) { - val v = valueCache.getOrElse(k, null) - if(addTo != null && v != null) addTo(addKey) = v + val v = valueCache.get(k) + if(addTo != null && v != null) addTo.put(addKey, v) v } else { val s = this.`super` @@ -343,7 +346,7 @@ object Val{ case supValue => mergeMember(supValue, vv, pos) } } else vv - if(addTo != null && m.cached) addTo(addKey) = v + if(addTo != null && m.cached) addTo.put(addKey, v) v } } @@ -378,7 +381,7 @@ object Val{ fields: Array[Expr.Member.Field], internedKeyMaps: mutable.HashMap[StaticObjectFieldSet, java.util.LinkedHashMap[String, java.lang.Boolean]], internedStrings: mutable.HashMap[String, String]): Obj = { - val cache = Util.preSizedScalaMutableHashMap[Any, Val](fields.length) + val cache = Util.preSizedJavaHashMap[Any, Val](fields.length) val allKeys = Util.preSizedJavaLinkedHashMap[String, java.lang.Boolean](fields.length) val keys = new Array[String](fields.length) var idx = 0 diff --git a/sjsonnet/src/sjsonnet/ValVisitor.scala b/sjsonnet/src/sjsonnet/ValVisitor.scala index 441acabe..85901c00 100644 --- a/sjsonnet/src/sjsonnet/ValVisitor.scala +++ b/sjsonnet/src/sjsonnet/ValVisitor.scala @@ -17,7 +17,7 @@ class ValVisitor(pos: Position) extends JsVisitor[Val, Val] { self => } def visitObject(length: Int, index: Int): ObjVisitor[Val, Val] = new ObjVisitor[Val, Val] { - val cache = mutable.HashMap.empty[Any, Val] + val cache = new java.util.HashMap[Any, Val]() val allKeys = new util.LinkedHashMap[String, java.lang.Boolean] var key: String = null def subVisitor: Visitor[_, _] = self From 3ff6f085541d79a29d8950a0a19d5a8df4e4c0f6 Mon Sep 17 00:00:00 2001 From: Josh Rosen Date: Fri, 3 Jan 2025 16:11:34 -0800 Subject: [PATCH 08/14] Optimized distinctKeys for small rhs (typical case) --- sjsonnet/src/sjsonnet/Std.scala | 40 ++++++++++++++++++++++++++++++++- 1 file changed, 39 insertions(+), 1 deletion(-) diff --git a/sjsonnet/src/sjsonnet/Std.scala b/sjsonnet/src/sjsonnet/Std.scala index 6652bdc9..20e0c032 100644 --- a/sjsonnet/src/sjsonnet/Std.scala +++ b/sjsonnet/src/sjsonnet/Std.scala @@ -928,7 +928,7 @@ class Std(private val additionalNativeFunctions: Map[String, Val.Builtin] = Map. } def recPair(l: Val, r: Val): Val = (l, r) match{ case (l: Val.Obj, r: Val.Obj) => - val keys: Array[String] = (l.visibleKeyNames ++ r.visibleKeyNames).distinct + val keys: Array[String] = distinctKeys(l.visibleKeyNames, r.visibleKeyNames) val kvs: Array[(String, Val.Obj.Member)] = new Array[(String, Val.Obj.Member)](keys.length) var kvsIdx = 0 var i = 0 @@ -976,6 +976,44 @@ class Std(private val additionalNativeFunctions: Map[String, Val.Builtin] = Map. case _ => v } + def distinctKeys(lKeys: Array[String], rKeys: Array[String]): Array[String] = { + if (rKeys.length <= 8) { + val rKeysCopy = new Array[String](rKeys.length) + rKeys.copyToArray(rKeysCopy) + var i = 0 + var numNewRKeys = rKeysCopy.length + while (i < lKeys.length) { + val lKey = lKeys(i) + var j = 0 + while (j < rKeysCopy.length) { + if (lKey == rKeysCopy(j)) { + rKeysCopy(j) = null + numNewRKeys -= 1 + } + j += 1 + } + i += 1 + } + if (numNewRKeys == 0) { + lKeys + } else { + val outArray = new Array[String](lKeys.length + numNewRKeys) + System.arraycopy(lKeys, 0, outArray, 0, lKeys.length) + var outIdx = lKeys.length + var j = 0 + while (j < rKeysCopy.length) { + if (rKeysCopy(j) != null) { + outArray(outIdx) = rKeysCopy(j) + outIdx += 1 + } + j += 1 + } + outArray + } + } else { + (lKeys ++ rKeys).distinct + } + } recPair(target, patch) }, builtin("sqrt", "x"){ (pos, ev, x: Double) => From 017b80cfc8d0eb5b9bb6977d6a3e3f99d139be58 Mon Sep 17 00:00:00 2001 From: Josh Rosen Date: Fri, 3 Jan 2025 16:13:56 -0800 Subject: [PATCH 09/14] Optimize for common case where most keys are visible --- sjsonnet/src/sjsonnet/Val.scala | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sjsonnet/src/sjsonnet/Val.scala b/sjsonnet/src/sjsonnet/Val.scala index a6df28da..879e4c29 100644 --- a/sjsonnet/src/sjsonnet/Val.scala +++ b/sjsonnet/src/sjsonnet/Val.scala @@ -263,7 +263,7 @@ object Val{ } else { val buf = new mutable.ArrayBuilder.ofRef[String] if (`super` == null) { - buf.sizeHint(Math.min(value0.size(), 16)) + buf.sizeHint(value0.size()) value0.forEach((k, m) => if (m.visibility != Visibility.Hidden) buf += k) } else { getAllKeys.forEach((k, b) => if (b == java.lang.Boolean.FALSE) buf += k) From 8054868618627a45536b227cd6608ccae8ae58f1 Mon Sep 17 00:00:00 2001 From: Josh Rosen Date: Fri, 3 Jan 2025 16:24:21 -0800 Subject: [PATCH 10/14] Avoid some array copies in Obj.mk in hot paths. --- sjsonnet/src/sjsonnet/Std.scala | 12 ++++++------ sjsonnet/src/sjsonnet/Val.scala | 11 +++++++++++ 2 files changed, 17 insertions(+), 6 deletions(-) diff --git a/sjsonnet/src/sjsonnet/Std.scala b/sjsonnet/src/sjsonnet/Std.scala index 20e0c032..838142e8 100644 --- a/sjsonnet/src/sjsonnet/Std.scala +++ b/sjsonnet/src/sjsonnet/Std.scala @@ -952,7 +952,7 @@ class Std(private val additionalNativeFunctions: Map[String, Val.Builtin] = Map. } val trimmedKvs = if (kvsIdx == i) kvs else kvs.slice(0, kvsIdx) - Val.Obj.mk(mergePosition, trimmedKvs:_*) + Val.Obj.mk(mergePosition, trimmedKvs) case (_, _) => recSingle(r) } @@ -972,7 +972,7 @@ class Std(private val additionalNativeFunctions: Map[String, Val.Builtin] = Map. i += 1 } val trimmedKvs = if (kvsIdx == i) kvs else kvs.slice(0, kvsIdx) - Val.Obj.mk(obj.pos, trimmedKvs:_*) + Val.Obj.mk(obj.pos, trimmedKvs) case _ => v } @@ -1476,12 +1476,12 @@ class Std(private val additionalNativeFunctions: Map[String, Val.Builtin] = Map. } def rec(x: Val): Val = x match{ case o: Val.Obj => - val bindings = for{ + val bindings: Array[(String, Val.Obj.Member)] = for{ k <- o.visibleKeyNames v = rec(o.value(k, pos.fileScope.noOffsetPos)(ev)) if filter(v) }yield (k, new Val.Obj.ConstMember(false, Visibility.Normal, v)) - Val.Obj.mk(pos, bindings: _*) + Val.Obj.mk(pos, bindings) case a: Val.Arr => new Val.Arr(pos, a.asStrictArray.map(rec).filter(filter).map(identity)) case _ => x @@ -1572,12 +1572,12 @@ class Std(private val additionalNativeFunctions: Map[String, Val.Builtin] = Map. ))) }, builtin("objectRemoveKey", "obj", "key") { (pos, ev, o: Val.Obj, key: String) => - val bindings = for{ + val bindings: Array[(String, Val.Obj.Member)] = for{ k <- o.visibleKeyNames v = o.value(k, pos.fileScope.noOffsetPos)(ev) if k != key }yield (k, new Val.Obj.ConstMember(false, Visibility.Normal, v)) - Val.Obj.mk(pos, bindings: _*) + Val.Obj.mk(pos, bindings) }, builtin(MinArray), builtin(MaxArray), diff --git a/sjsonnet/src/sjsonnet/Val.scala b/sjsonnet/src/sjsonnet/Val.scala index 879e4c29..3a48a134 100644 --- a/sjsonnet/src/sjsonnet/Val.scala +++ b/sjsonnet/src/sjsonnet/Val.scala @@ -170,6 +170,17 @@ object Val{ for((k, v) <- members) m.put(k, v) new Obj(pos, m, false, null, null) } + + def mk(pos: Position, members: Array[(String, Obj.Member)]): Obj = { + val m = Util.preSizedJavaLinkedHashMap[String, Obj.Member](members.length) + var i = 0 + while (i < members.length) { + val e = members(i) + m.put(e._1, e._2) + i += 1 + } + new Obj(pos, m, false, null, null) + } } final class Obj(val pos: Position, From bf84312aa4ed6bdeb43d3befae2aa43b04fdad52 Mon Sep 17 00:00:00 2001 From: Josh Rosen Date: Fri, 3 Jan 2025 17:37:05 -0800 Subject: [PATCH 11/14] PrettyNamed implicits should be singletons / constants --- sjsonnet/src/sjsonnet/Val.scala | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/sjsonnet/src/sjsonnet/Val.scala b/sjsonnet/src/sjsonnet/Val.scala index 3a48a134..e3543691 100644 --- a/sjsonnet/src/sjsonnet/Val.scala +++ b/sjsonnet/src/sjsonnet/Val.scala @@ -74,13 +74,13 @@ sealed abstract class Val extends Lazy { class PrettyNamed[T](val s: String) object PrettyNamed{ - implicit def strName: PrettyNamed[Val.Str] = new PrettyNamed("string") - implicit def numName: PrettyNamed[Val.Num] = new PrettyNamed("number") - implicit def arrName: PrettyNamed[Val.Arr] = new PrettyNamed("array") - implicit def boolName: PrettyNamed[Val.Bool] = new PrettyNamed("boolean") - implicit def objName: PrettyNamed[Val.Obj] = new PrettyNamed("object") - implicit def funName: PrettyNamed[Val.Func] = new PrettyNamed("function") - implicit def nullName: PrettyNamed[Val.Null] = new PrettyNamed("null") + implicit val strName: PrettyNamed[Val.Str] = new PrettyNamed("string") + implicit val numName: PrettyNamed[Val.Num] = new PrettyNamed("number") + implicit val arrName: PrettyNamed[Val.Arr] = new PrettyNamed("array") + implicit val boolName: PrettyNamed[Val.Bool] = new PrettyNamed("boolean") + implicit val objName: PrettyNamed[Val.Obj] = new PrettyNamed("object") + implicit val funName: PrettyNamed[Val.Func] = new PrettyNamed("function") + implicit val nullName: PrettyNamed[Val.Null] = new PrettyNamed("null") } object Val{ From 49b3a5287e05d07f3507d25f5b8c4ed0c4c201f2 Mon Sep 17 00:00:00 2001 From: Josh Rosen Date: Tue, 7 Jan 2025 12:39:11 -0800 Subject: [PATCH 12/14] Add more comments. --- sjsonnet/src/sjsonnet/Expr.scala | 4 +-- sjsonnet/src/sjsonnet/Std.scala | 10 ++++++++ sjsonnet/src/sjsonnet/Val.scala | 43 +++++++++++++++++++++++++++++--- 3 files changed, 52 insertions(+), 5 deletions(-) diff --git a/sjsonnet/src/sjsonnet/Expr.scala b/sjsonnet/src/sjsonnet/Expr.scala index 8216142c..14643887 100644 --- a/sjsonnet/src/sjsonnet/Expr.scala +++ b/sjsonnet/src/sjsonnet/Expr.scala @@ -58,7 +58,7 @@ object Expr{ } case class Field(pos: Position, fieldName: FieldName, - plus: Boolean, + plus: Boolean, // see https://jsonnet.org/ref/language.html#nested-field-inheritance args: Params, sep: Visibility, rhs: Expr) extends Member { @@ -175,7 +175,7 @@ object Expr{ preLocals: Array[Bind], key: Expr, value: Expr, - plus: Boolean, + plus: Boolean, // see https://jsonnet.org/ref/language.html#nested-field-inheritance postLocals: Array[Bind], first: ForSpec, rest: List[CompSpec]) extends ObjBody { diff --git a/sjsonnet/src/sjsonnet/Std.scala b/sjsonnet/src/sjsonnet/Std.scala index 838142e8..41b6c46e 100644 --- a/sjsonnet/src/sjsonnet/Std.scala +++ b/sjsonnet/src/sjsonnet/Std.scala @@ -977,6 +977,13 @@ class Std(private val additionalNativeFunctions: Map[String, Val.Builtin] = Map. case _ => v } def distinctKeys(lKeys: Array[String], rKeys: Array[String]): Array[String] = { + // Fast path for small RHS size (the common case when merging a small + // patch into a large target object), avoiding the cost of constructing + // and probing a hash set: instead, perform a nested loop where the LHS + // is scanned and matching RHS entries are marked as null to be skipped. + // Via local microbenchmarks simulating a "worst-case" (RHS keys all new), + // the threshold of `8` was empirically determined to be a good tradeoff + // between allocation + hashing costs vs. nested loop array scans. if (rKeys.length <= 8) { val rKeysCopy = new Array[String](rKeys.length) rKeys.copyToArray(rKeysCopy) @@ -986,6 +993,7 @@ class Std(private val additionalNativeFunctions: Map[String, Val.Builtin] = Map. val lKey = lKeys(i) var j = 0 while (j < rKeysCopy.length) { + // This LHS key is in the RHS, so mark it to be skipped in output: if (lKey == rKeysCopy(j)) { rKeysCopy(j) = null numNewRKeys -= 1 @@ -994,6 +1002,7 @@ class Std(private val additionalNativeFunctions: Map[String, Val.Builtin] = Map. } i += 1 } + // Combine lKeys with non-null elements of rKeysCopy: if (numNewRKeys == 0) { lKeys } else { @@ -1011,6 +1020,7 @@ class Std(private val additionalNativeFunctions: Map[String, Val.Builtin] = Map. outArray } } else { + // Fallback: Use hash-based deduplication for large RHS arrays: (lKeys ++ rKeys).distinct } } diff --git a/sjsonnet/src/sjsonnet/Val.scala b/sjsonnet/src/sjsonnet/Val.scala index e3543691..1383db64 100644 --- a/sjsonnet/src/sjsonnet/Val.scala +++ b/sjsonnet/src/sjsonnet/Val.scala @@ -144,10 +144,12 @@ object Val{ object Obj{ + /** + * Helper for saving space in valueCache for objects without a super object. + * For objects with no super, we (cheaply) know the exact number of fields and + * therefore can upper bound the number of fields that _might_ be computed. + */ def getEmptyValueCacheForObjWithoutSuper(numFields: Int): util.HashMap[Any, Val] = { - // Helper for saving space in valueCache for objects without a super object. - // For objects with no super, we (cheaply) know the exact number of fields and - // therefore can upper bound the number of fields that _might_ be computed. // We only want to pre-size if it yields a smaller initial map size than the default. if (numFields >= 12) { new util.HashMap[Any, Val]() @@ -156,6 +158,11 @@ object Val{ } } + /** + * @param add whether this field was defined the "+:", "+::" or "+:::" separators, corresponding + * to the "nested field inheritance" language feature; see + * https://jsonnet.org/ref/language.html#nested-field-inheritance + */ abstract class Member(val add: Boolean, val visibility: Visibility, val cached: Boolean = true) { def invoke(self: Obj, sup: Obj, fs: FileScope, ev: EvalScope): Val } @@ -183,6 +190,27 @@ object Val{ } } + /** + * Represents json/jsonnet objects. + * + * Obj implements special optimizations for "static objects", which are objects without + * `super` where all fields are visible and constant. Static objects can be created + * during parsing or in [[StaticOptimizer]]. + * + * @param value0 maps fields to their Member definitions. This is initially null for + * static objects and is non-null for non-static objects. + * @param static true if this object is static, false otherwise. + * @param triggerAsserts callback to evaluate assertions defined in the object. + * @param `super` the super object, or null if there is no super object. + * @param valueCache a cache for computed values. For static objects, this is pre-populated + * with all fields. For non-static objects, this is lazily populated as + * fields are accessed. + * @param allKeys a map of all keys in the object (including keys inherited from `super`), + * where the boolean value is true if the key is hidden and false otherwise. + * For static objects, this is pre-populated and the mapping may be interned + * and shared across instances. For non-static objects, it is dynamically + * computed only if the object has a `super` + */ final class Obj(val pos: Position, private[this] var value0: util.LinkedHashMap[String, Obj.Member], static: Boolean, @@ -195,6 +223,9 @@ object Val{ def getSuper = `super` private[this] def getValue0: util.LinkedHashMap[String, Obj.Member] = { + // value0 is always defined for non-static objects, so if we're computing it here + // then that implies that the object is static and therefore valueCache should be + // pre-populated and all members should be visible and constant. if(value0 == null) { val value0 = Util.preSizedJavaLinkedHashMap[String, Val.Obj.Member](allKeys.size()) allKeys.forEach { (k, _) => @@ -274,6 +305,8 @@ object Val{ } else { val buf = new mutable.ArrayBuilder.ofRef[String] if (`super` == null) { + // This size hint is based on an optimistic assumption that most fields are visible, + // avoiding re-sizing or trimming the buffer in the common case: buf.sizeHint(value0.size()) value0.forEach((k, m) => if (m.visibility != Visibility.Hidden) buf += k) } else { @@ -309,6 +342,10 @@ object Val{ private def renderString(v: Val)(implicit evaluator: EvalScope): String = evaluator.materialize(v).transform(new Renderer()).toString + /** + * Merge two values for "nested field inheritance"; see + * https://jsonnet.org/ref/language.html#nested-field-inheritance for background. + */ def mergeMember(l: Val, r: Val, pos: Position) From 4cbbe864a62cdf877458c3703168a3cfecaaf564 Mon Sep 17 00:00:00 2001 From: Josh Rosen Date: Tue, 7 Jan 2025 13:18:32 -0800 Subject: [PATCH 13/14] More comments for mergePatch --- sjsonnet/src/sjsonnet/Std.scala | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/sjsonnet/src/sjsonnet/Std.scala b/sjsonnet/src/sjsonnet/Std.scala index 41b6c46e..5e6d8b08 100644 --- a/sjsonnet/src/sjsonnet/Std.scala +++ b/sjsonnet/src/sjsonnet/Std.scala @@ -923,7 +923,7 @@ class Std(private val additionalNativeFunctions: Map[String, Val.Builtin] = Map. builtin(Range), builtin("mergePatch", "target", "patch"){ (pos, ev, target: Val, patch: Val) => val mergePosition = pos - def createMember(v: => Val) = new Val.Obj.Member(false, Visibility.Normal) { + def createLazyMember(v: => Val) = new Val.Obj.Member(false, Visibility.Normal) { def invoke(self: Val.Obj, sup: Val.Obj, fs: FileScope, ev: EvalScope): Val = v } def recPair(l: Val, r: Val): Val = (l, r) match{ @@ -936,13 +936,16 @@ class Std(private val additionalNativeFunctions: Map[String, Val.Builtin] = Map. val key = keys(i) val lValue = if (l.containsVisibleKey(key)) l.valueRaw(key, l, pos)(ev) else null val rValue = if (r.containsVisibleKey(key)) r.valueRaw(key, r, pos)(ev) else null - if (rValue == null || !rValue.isInstanceOf[Val.Null]) { + if (!rValue.isInstanceOf[Val.Null]) { // if we are not removing the key if (lValue != null && rValue == null) { + // Preserve the LHS/target value: kvs(kvsIdx) = (key, new Val.Obj.ConstMember(false, Visibility.Normal, lValue)) } else if (lValue.isInstanceOf[Val.Obj] && rValue.isInstanceOf[Val.Obj]) { - kvs(kvsIdx) = (key, createMember(recPair(lValue, rValue))) + // Recursively merge objects: + kvs(kvsIdx) = (key, createLazyMember(recPair(lValue, rValue))) } else if (rValue != null) { - kvs(kvsIdx) = (key, createMember(recSingle(rValue))) + // Use the RHS/patch value and recursively remove Null or hidden fields: + kvs(kvsIdx) = (key, createLazyMember(recSingle(rValue))) } else { Error.fail("std.mergePatch: This should never happen") } @@ -966,7 +969,7 @@ class Std(private val additionalNativeFunctions: Map[String, Val.Builtin] = Map. val key = keys(i) val value = obj.value(key, pos, obj)(ev) if (!value.isInstanceOf[Val.Null]) { - kvs(kvsIdx) = (key, createMember(recSingle(value))) + kvs(kvsIdx) = (key, createLazyMember(recSingle(value))) kvsIdx += 1 } i += 1 From 655345057056c2ca64a592512c274d46f78cb86c Mon Sep 17 00:00:00 2001 From: Josh Rosen Date: Tue, 7 Jan 2025 13:46:10 -0800 Subject: [PATCH 14/14] comment reword --- sjsonnet/src/sjsonnet/Val.scala | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/sjsonnet/src/sjsonnet/Val.scala b/sjsonnet/src/sjsonnet/Val.scala index 1383db64..33400201 100644 --- a/sjsonnet/src/sjsonnet/Val.scala +++ b/sjsonnet/src/sjsonnet/Val.scala @@ -194,8 +194,8 @@ object Val{ * Represents json/jsonnet objects. * * Obj implements special optimizations for "static objects", which are objects without - * `super` where all fields are visible and constant. Static objects can be created - * during parsing or in [[StaticOptimizer]]. + * `super` where all fields are constant and have default visibility. Static objects can + * be created during parsing or in [[StaticOptimizer]]. * * @param value0 maps fields to their Member definitions. This is initially null for * static objects and is non-null for non-static objects.