From fa95ccbbe6369076e587c30b389018e0bb378db7 Mon Sep 17 00:00:00 2001 From: Kyri Petrou Date: Thu, 23 Nov 2023 18:23:01 +1100 Subject: [PATCH 1/4] Avoid unnecessary field merging during execution --- .../scala/caliban/execution/Executor.scala | 44 +++++++++---------- 1 file changed, 22 insertions(+), 22 deletions(-) diff --git a/core/src/main/scala/caliban/execution/Executor.scala b/core/src/main/scala/caliban/execution/Executor.scala index 87a3b13409..c6db8ccfe7 100644 --- a/core/src/main/scala/caliban/execution/Executor.scala +++ b/core/src/main/scala/caliban/execution/Executor.scala @@ -305,29 +305,29 @@ object Executor { private[caliban] def fail(error: CalibanError): UIO[GraphQLResponse[CalibanError]] = ZIO.succeed(GraphQLResponse(NullValue, List(error))) - private[caliban] def mergeFields(field: Field, typeName: String): List[Field] = { - val map = new java.util.LinkedHashMap[String, Field]() - var modified = false - - field.fields.foreach { field => - if (field._condition.forall(_.contains(typeName))) { - map.compute( - field.aliasedName, - (_, f) => - if (f == null) field - else { - modified = true - f.copy(fields = f.fields ::: field.fields) - } - ) - } else { - modified = true - } - } + private[caliban] def mergeFields(field: Field, typeName: String): List[Field] = + field.fields match { + // Shortcut if all the fields have the same condition, which means we don't need to dedup + // as that's been handled in Field.apply + case fields @ head :: tail if tail.forall(_._condition == head._condition) => + if (head._condition.forall(_.contains(typeName))) fields + else Nil + case Nil => Nil + case fields => + val map = new java.util.LinkedHashMap[String, Field]() + fields.foreach { field => + if (field._condition.forall(_.contains(typeName))) { + map.compute( + field.aliasedName, + (_, f) => + if (f == null) field + else f.copy(fields = f.fields ::: field.fields) + ) + } + } - // Avoid conversions if no modification took place - if (modified) map.values().asScala.toList else field.fields - } + map.values().asScala.toList + } private def fieldInfo(field: Field, path: List[Either[String, Int]], fieldDirectives: List[Directive]): FieldInfo = FieldInfo(field.aliasedName, field, path, fieldDirectives, field.parentType) From 3e7ec5ad5777ddc881ca3e904ef70f6f3bf18d0b Mon Sep 17 00:00:00 2001 From: Kyri Petrou Date: Thu, 23 Nov 2023 18:27:11 +1100 Subject: [PATCH 2/4] Cleanup --- core/src/main/scala/caliban/execution/Executor.scala | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/core/src/main/scala/caliban/execution/Executor.scala b/core/src/main/scala/caliban/execution/Executor.scala index c6db8ccfe7..2a7476e427 100644 --- a/core/src/main/scala/caliban/execution/Executor.scala +++ b/core/src/main/scala/caliban/execution/Executor.scala @@ -309,11 +309,11 @@ object Executor { field.fields match { // Shortcut if all the fields have the same condition, which means we don't need to dedup // as that's been handled in Field.apply - case fields @ head :: tail if tail.forall(_._condition == head._condition) => - if (head._condition.forall(_.contains(typeName))) fields + case head :: tail if tail.forall(_._condition == head._condition) => + if (head._condition.forall(_.contains(typeName))) field.fields else Nil - case Nil => Nil - case fields => + case Nil => Nil + case fields => val map = new java.util.LinkedHashMap[String, Field]() fields.foreach { field => if (field._condition.forall(_.contains(typeName))) { From 547efa203532e42dd1bb489ad506aef5898ef649 Mon Sep 17 00:00:00 2001 From: Kyri Petrou Date: Fri, 24 Nov 2023 08:38:58 +1100 Subject: [PATCH 3/4] PR comments --- .../scala/caliban/execution/Executor.scala | 56 ++++++++++++------- 1 file changed, 36 insertions(+), 20 deletions(-) diff --git a/core/src/main/scala/caliban/execution/Executor.scala b/core/src/main/scala/caliban/execution/Executor.scala index 2a7476e427..b9a7db5eea 100644 --- a/core/src/main/scala/caliban/execution/Executor.scala +++ b/core/src/main/scala/caliban/execution/Executor.scala @@ -305,29 +305,45 @@ object Executor { private[caliban] def fail(error: CalibanError): UIO[GraphQLResponse[CalibanError]] = ZIO.succeed(GraphQLResponse(NullValue, List(error))) - private[caliban] def mergeFields(field: Field, typeName: String): List[Field] = - field.fields match { - // Shortcut if all the fields have the same condition, which means we don't need to dedup - // as that's been handled in Field.apply - case head :: tail if tail.forall(_._condition == head._condition) => - if (head._condition.forall(_.contains(typeName))) field.fields - else Nil - case Nil => Nil - case fields => - val map = new java.util.LinkedHashMap[String, Field]() - fields.foreach { field => - if (field._condition.forall(_.contains(typeName))) { - map.compute( - field.aliasedName, - (_, f) => - if (f == null) field - else f.copy(fields = f.fields ::: field.fields) - ) - } + private[caliban] def mergeFields(field: Field, typeName: String): List[Field] = { + def haveSameCondition(head: Field, tail: List[Field]): Boolean = { + val condition = head._condition + var remaining = tail + while (!remaining.isEmpty) { + if (remaining.head._condition != condition) return false + remaining = remaining.tail + } + true + } + + def matchesTypename(f: Field): Boolean = + f._condition.isEmpty || f._condition.get.contains(typeName) + + def mergeFields(fields: List[Field]) = { + val map = new java.util.LinkedHashMap[String, Field]((fields.size / 0.75d).toInt + 1) + var remaining = fields + while (!remaining.isEmpty) { + val h = remaining.head + if (matchesTypename(h)) { + map.compute( + h.aliasedName, + (_, f) => + if (f eq null) h + else f.copy(fields = f.fields ::: h.fields) + ) } + remaining = remaining.tail + } + map.values().asScala.toList + } - map.values().asScala.toList + field.fields match { + // Shortcut if all the fields have the same condition, which means we don't need to merge as that's been handled in Field.apply + case h :: t if haveSameCondition(h, t) => if (matchesTypename(h)) field.fields else Nil + case Nil => Nil + case fields => mergeFields(fields) } + } private def fieldInfo(field: Field, path: List[Either[String, Int]], fieldDirectives: List[Directive]): FieldInfo = FieldInfo(field.aliasedName, field, path, fieldDirectives, field.parentType) From c6201feda9b4c3c488b2091149f96cc1b9e096cb Mon Sep 17 00:00:00 2001 From: Kyri Petrou Date: Fri, 24 Nov 2023 17:46:14 +1100 Subject: [PATCH 4/4] Extract Map size calculation into separate method --- .../main/scala/caliban/execution/Executor.scala | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/core/src/main/scala/caliban/execution/Executor.scala b/core/src/main/scala/caliban/execution/Executor.scala index b9a7db5eea..10c86d61d7 100644 --- a/core/src/main/scala/caliban/execution/Executor.scala +++ b/core/src/main/scala/caliban/execution/Executor.scala @@ -320,7 +320,7 @@ object Executor { f._condition.isEmpty || f._condition.get.contains(typeName) def mergeFields(fields: List[Field]) = { - val map = new java.util.LinkedHashMap[String, Field]((fields.size / 0.75d).toInt + 1) + val map = new java.util.LinkedHashMap[String, Field](calculateMapCapacity(fields.size)) var remaining = fields while (!remaining.isEmpty) { val h = remaining.head @@ -386,4 +386,17 @@ object Executor { (l.result(), r.result()) } } + + /** + * The behaviour of mutable Maps (both Java and Scala) is to resize once the number of entries exceeds + * the capacity * loadFactor (default of 0.75d) threshold in order to prevent hash collisions. + * + * This method is a helper method to estimate the initial map size depending on the number of elements the Map is + * expected to hold + * + * NOTE: This method is the same as java.util.HashMap.calculateHashMapCapacity on JDK19+ + */ + private def calculateMapCapacity(nMappings: Int): Int = + Math.ceil(nMappings / 0.75d).toInt + }