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

Improved execution field merging #2019

Merged
merged 4 commits into from
Nov 25, 2023
Merged
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
65 changes: 47 additions & 18 deletions core/src/main/scala/caliban/execution/Executor.scala
Original file line number Diff line number Diff line change
Expand Up @@ -306,27 +306,43 @@ object Executor {
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
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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was gonna suggest tail recursion but this will probably iterate tighter

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm I think the compiler will rewrite the tailrec method likely to this implementation. I'll check, if it's the same I'll use tailrec

Copy link
Collaborator Author

@kyri-petrou kyri-petrou Nov 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the generated code, it looks like @tailrec uses Java labels in an infinite while loop with return statements, whereas this one uses the !remaining.isEmpty method as a condition to terminate the while loop. Not sure which one would be more performant in theory

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah we're probably getting into a grey area of compiler optimization. I think the conditionless while might be able to skip a branch if equal instruction, though for modern chips branches aren't really performance killers anymore

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](calculateMapCapacity(fields.size))
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
}

// Avoid conversions if no modification took place
if (modified) map.values().asScala.toList else field.fields
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 =
Expand Down Expand Up @@ -370,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 =
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After merging this you can use it in the other PR

Math.ceil(nMappings / 0.75d).toInt

}