Repository navigation
Conversation
ValuesResolverOneOfValidation walks the full input object tree of every argument on every argument coercion and every variable coercion, and allocates a stream pipeline plus a collected list per input object field, solely to find @OneOf types and enforce their single-key constraint. On schemas that use no @OneOf types this walk can never do anything but still runs for every input argument of every field of every request. Memoize per GraphQLInputObjectType whether its transitively reachable input types include a @OneOf type, and skip the walk when none is reachable. The check is cycle-safe for self-referencing input types. When a @OneOf type is reachable the existing walk runs unchanged. Synthetic execution benchmark (5.5k fetched fields/op, 501 fields/op carrying nested input-object arguments, ThreadMXBean allocated bytes): -3.6% bytes/op with default instrumentation, -1.6% bytes/op with 8 chained instrumentations. Generated by Josh-Bot 9000
| * Per input object type: true if the type itself or any transitively reachable input object type is a | ||
| * @oneOf type. Input types are immutable once built, so this answer is stable for a given type instance. | ||
| */ | ||
| private static final Map<GraphQLInputObjectType, Boolean> CONTAINS_ONE_OF = new ConcurrentHashMap<>(); |
There was a problem hiding this comment.
While the idea here has merit I think the static is the wrong place to cache the object types.
A graphql server MAY use many different schemas over its life time. However map is cached JVM wide. So its every growing.
I think this question (does an input type have the @OneOf directive would be better cached inside the schema
schema.inputTypeContainsOneOf( inputType)
This could then be computed at schema build time and cached in a map that does not change.
That way if the schema goes away - the knowledge of the types goes with it.
As it stands today if the schema goes away, the static map here conitnues to have a reference to the object type and hence the schema.
So the schema can never be garbage collected and schemas are very large objects
There was a problem hiding this comment.
Ahh just saw
If that is a concern I can switch to weak keys or compute the flag at schema build time instead — happy to take guidance on the preferred form.
There was a problem hiding this comment.
I understand the concerns @bbakerman, I'm still running through some performance tests on my side before I move this out of draft at which point I'll also address the concerns about a static cache
bbakerman
left a comment
There was a problem hiding this comment.
This needs to cache to be inside the schema object - not in a JVM static
Test ReportTest Results
Code Coverage (Java 25)
Changed Class Coverage (1 class)
ValuesResolverOneOfValidation — method details
|
Problem
ValuesResolverOneOfValidation.validateOneOfInputTypesruns on every argument coercion (ValuesResolver.getArgumentValuesImpl) and every variable coercion (ValuesResolverConversion). It recursively walks the full input object tree of every argument value — allocating a stream pipeline and a collectedListper input object field — solely to find@oneOftypes and enforce their single-key constraint.On schemas that use no
@oneOftypes at all this walk can never do anything, but it still runs for every input argument of every field of every request. In allocation profiles of high-throughput services this walk shows up as a measurable share of total allocated bytes.Change
Memoize, per
GraphQLInputObjectType, whether the type itself or any transitively reachable input object type is a@oneOftype, and skip the walk when the answer is no:@oneOftype is reachable, the existing walk runs unchanged.ValuesResolverOneOfValidationis@Internal), no new dependencies.Benchmark
Synthetic execution benchmark: SDL schema with nested lists, ~5.5k fetched fields/op, 501 fields/op carrying nested input-object arguments (
input SubFilter { x: Int, y: Int, c: SubFilter }style). Bytes measured viaThreadMXBean.getThreadAllocatedBytes, 300 ops after 200 warmup, 3 runs each, ParallelGC:A consistent ~104 KB/op reduction across all runs; wall time unchanged within noise. The absolute win scales with the size and depth of input argument trees in the schema.
Tests
Two new Spock tests in
ValuesResolverTest:oneOf reachable through a self-referencing input type is still validated— the guard does not over-skip: a@oneOfviolation nested inside a recursive input type is still rejected, on both the literal and the variable coercion paths.self-referencing input types without oneOf execute without infinite recursion— the reachability computation terminates on cyclic input types and the values execute normally.Verification:
./gradlew test— 5863 tests, BUILD SUCCESSFUL (includes the fullgraphql.execution.*package and the existing oneOf validation tests).Notes for reviewers
CONTAINS_ONE_OFkeys onGraphQLInputObjectTypeidentity. Schemas are long-lived singletons in the intended usage, but an application that rebuilds schemas per request would grow this map. If that is a concern I can switch to weak keys or compute the flag at schema build time instead — happy to take guidance on the preferred form.Assert.assertTrue(inputValue instanceof Map)no longer runs on skipped subtrees; coerced input object values are maps by construction.Generated by Josh-Bot 9000