Performance improvements #659
Conversation
| @@ -23,16 +23,20 @@ import java.util.concurrent.atomic.AtomicBoolean | |||
| import java.util.concurrent.atomic.AtomicInteger | |||
|
|
|||
| class NadelIncrementalResultSupport internal constructor( | |||
| private val accumulator: NadelIncrementalResultAccumulator, | |||
| lazyAccumulator: Lazy<NadelIncrementalResultAccumulator>, | |||
There was a problem hiding this comment.
This is a costly object, we should put it behind lazy because most requests aren't using @defer
Either way it should only be instantiated once we actually need it, so that the request can proceed quicker.
There was a problem hiding this comment.
Just for the record there is very little "performance" to be had by avoiding a single object allocation to later.
But I can see that private val queryPathToExecutions = operation.walkTopDown() would be expensive so this makes sense.
| @@ -28,10 +28,6 @@ data class NadelExecutionContext internal constructor( | |||
| ) { | |||
| private val serviceContexts = ConcurrentHashMap<String, CompletableFuture<Any?>>() | |||
|
|
|||
| @Deprecated("Use incrementalSupport instead", ReplaceWith("incrementalResultSupport")) | |||
| internal val deferSupport: NadelIncrementalResultSupport | |||
| get() = incrementalResultSupport | |||
| } | ||
|
|
||
| return map | ||
| } |
There was a problem hiding this comment.
Moved these from below.
| @@ -16,7 +16,7 @@ import graphql.schema.GraphQLSchema | |||
| */ | |||
| data class NadelOverallExecutionBlueprint( | |||
| val engineSchema: GraphQLSchema, | |||
| val fieldInstructions: Map<FieldCoordinates, List<NadelFieldInstruction>>, | |||
| val fieldInstructions: NadelFieldMap<List<NadelFieldInstruction>>, | |||
There was a problem hiding this comment.
This is a new construct that lets us query by get(typeName: String, fieldName: String) so we don't have to construct a FieldCoordinates just to perform a lookup.
| @@ -60,60 +61,64 @@ internal class NadelExecutionPlanFactory( | |||
| rootField: ExecutableNormalizedField, | |||
| serviceHydrationDetails: ServiceExecutionHydrationDetails?, | |||
| ): NadelExecutionPlan { | |||
| val executionSteps = mutableListOf<AnyNadelExecutionPlanStep>() | |||
There was a problem hiding this comment.
We used to do .groupBy { it.field } at the end but that is pretty wasteful.
So here we construct the Map directly.
| val noForeignTypes = overallField.objectTypeNames | ||
| .all { objectTypeName -> | ||
| objectTypeName in typeNamesOwnedByService | ||
| || objectTypeName in underlyingTypeNamesOwnedByService | ||
| } |
There was a problem hiding this comment.
This first checks if we have any foreign types before running .filter to actually transform it.
Most of the time we should only have types for the service, so we shouldn't need to invoke .filter and create a new list with everything copied over.
| // concat 1 giant set and then check | ||
| return objectTypeName in typeNamesOwnedByService | ||
| || objectTypeName in underlyingTypeNamesOwnedByService | ||
| || (executionContext.hints.sharedTypeRenames(service) && executionBlueprint.getUnderlyingTypeName(objectTypeName) in underlyingTypeNamesOwnedByService) |
There was a problem hiding this comment.
This was inlined above
| @@ -57,7 +57,7 @@ class NadelQueryTransformer private constructor( | |||
|
|
|||
| private data class TransformContext( | |||
| val artificialFields: MutableList<ExecutableNormalizedField> = mutableListOf(), | |||
| val overallToUnderlyingFields: MutableMap<ExecutableNormalizedField, List<ExecutableNormalizedField>> = mutableMapOf(), | |||
| val overallToUnderlyingFields: MutableMap<ExecutableNormalizedField, MutableList<ExecutableNormalizedField>> = mutableMapOf(), | |||
There was a problem hiding this comment.
Mutate list should be faster instead of copying list with new element
| var fieldFromPreviousTransform: ExecutableNormalizedField = field | ||
| var aggregatedTransformResult: NadelTransformFieldResult? = null | ||
| var newField: ExecutableNormalizedField = field | ||
| val artificialFields = mutableListOf<ExecutableNormalizedField>() |
There was a problem hiding this comment.
Add artificial fields to this mutable list intead of creating a new list every time
| } | ||
|
|
||
| private fun getUnderlyingTypeNames(objectTypeNames: Collection<String>): List<String> { | ||
| return if (executionContext.hints.sharedTypeRenames(service)) { |
There was a problem hiding this comment.
Shared type renames are not required with the new blueprint
gnawf
force-pushed
the
performance-improvements
branch
from
38f1321 to
1e2f9f5
Compare
No description provided.