Optimize FieldsWillMerge validation rule#5591
Open
swalkinshaw wants to merge 7 commits intomasterfrom
Open
Conversation
Plain class initialization is faster than Struct.new with YJIT. Also adds memoized return_type and unwrapped_return_type accessors to avoid repeated definition.type and unwrap calls in find_conflict and find_conflicts_between_sub_selection_sets. Skips return_types_conflict? when both field definitions are the same object. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Inline the setting_errors block to eliminate closure allocation per on_field/on_operation_definition call. Cache context.max_errors, context.fragments, and context.query.types as instance variables to reduce method dispatch overhead in hot paths. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Use object identity (equal?) as fast path before falling back to the expensive AST node #== comparison. Most conflict checks involve the same node object, making the identity check sufficient. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Cache type-pair mutual exclusivity checks to avoid repeated possible_types lookups for the same pair. Use object identity (equal?) instead of == for type comparison, and Array#intersect? instead of (& other).empty? for the set overlap check. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ture Replace the exponential recursive fragment cross-comparison algorithm with a linear field-flattening approach based on: https://tech.new-work.se/graphql-overlapping-fields-can-be-merged-fast-ea6e92e0a01 Instead of comparing fields-vs-fragments and fragments-vs-fragments separately (O(n*m*k) where m=fragments, k=nesting depth), flatten all fragment spreads into a single response-key map and compare within it. Key optimizations in the new algorithm: - Deduplicate fields by signature (name + definition + arguments) to avoid redundant comparisons — fields_merge benchmark: 3.1s to ~1.8ms - selections_may_conflict? fast path skips validation when all direct children are unaliased unique-named fields with no fragments - Cache collect_fields results per (node, return_type) to avoid re-expanding the same sub-selections across comparison contexts - Track compared sub-selection node pairs to prevent infinite recursion - Store single Field directly in response_keys hash, only wrap in array on collision (saves ~1200 array allocations) - Lazy visited_fragments allocation (nil until first fragment spread) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
rmosolgo
reviewed
Mar 26, 2026
Collaborator
Author
|
Okay I got Claude to do a few passes at expanding test coverage referencing https://github.com/graphql/graphql-js/tree/16.x.x/src/validation/__tests__ too. 36 added test cases They discovered one regression/issue which has been fixed |
swalkinshaw
force-pushed
the
optimize-fields-will-merge
branch
from
8b2c261 to
3dc282b
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Extracted from #5578
This is the main optimization since the
FieldsWillMergerule basically always dominates static validation time so improving the speed of this rule impacts the entire pipeline. Each optimization is broken down into a separate commit.The original algorithm compared fields across three phases (fields-within, fields-vs-fragments, fragments-vs-fragments) with recursive fragment cross-comparison that could be exponential. This rewrites it to: