Conversation
…queries Optimizations: - Replace HashSet/HashMap with Vec + linear scan in hot paths where N is small (parent_fragments, cycle detection, required arguments, argument equivalence, type overlap checks) - Eliminate Path Vec allocation by making Path Copy - Optimize duplicates() to skip BTreeMap allocation when no duplicates found (common case), and avoid intermediate (K,T) vec - Reuse Cache's fragment_definitions HashMap in FragmentSpreadTargetDefined instead of building a separate HashSet - Collect errors via &mut Vec in FieldSelectionMerging instead of returning intermediate Vecs Also adds a criterion benchmark suite for the validation pipeline. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
bluejay-validator/src/executable/document/rules/field_selection_merging.rs
Show resolved
Hide resolved
| // Fast path: if either type is not a composite type, spread is not applicable | ||
| if !Self::is_composite(parent_type) || !Self::is_composite(fragment_type) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
😅 kinda confusing how I wrote this because I would think spread would not be possible for non-composite parent types, I guess it kinda makes sense because we shouldn't be returning an error here because there would be a different error from a different rule (FragmentsOnCompositeTypes)
bluejay-validator/src/executable/document/rules/fragment_spread_is_possible.rs
Outdated
Show resolved
Hide resolved
bluejay-validator/src/executable/document/rules/fragment_spread_is_possible.rs
Show resolved
Hide resolved
| if self | ||
| .cache | ||
| .fragment_definition(fragment_spread.name()) | ||
| .is_none() |
| pub fn members(&self) -> &[&'a E::Selection] { | ||
| &self.members | ||
| &[] | ||
| } |
There was a problem hiding this comment.
this is just wrong now, no?
There was a problem hiding this comment.
Yeah pushed a 3rd separate which adds a leave_field hook on the visitor so that the typegen crate can track this itself and we can remove the concern from the validator entirely
swalkinshaw
force-pushed
the
validator-perf-optimizations
branch
2 times, most recently
from
53f7169 to
afcddc5
Compare
bluejay-validator/src/executable/document/rules/fragment_spread_is_possible.rs
Outdated
Show resolved
Hide resolved
bluejay-validator/src/executable/document/rules/fragment_spread_is_possible.rs
Outdated
Show resolved
Hide resolved
- Add comment explaining Interface parent type grouping in field_selection_merging - Use existing TypeDefinitionReference::is_composite() instead of custom method - Rewrite types_have_overlap as match (a, b) to avoid redundant expansion of b's types - Use Iterator::next() to avoid Vec allocation for 0-1 element iterators in duplicates() - Use itertools::combinations(2) for pairwise duplicate check Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Path is now purely a root identifier (Copy, no Vec allocation). Removed with_selection/members stubs that were broken after the Copy optimization. Added leave_field to the Visitor trait so PathsWithCustomScalarType can maintain its own field stack. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
swalkinshaw
force-pushed
the
validator-perf-optimizations
branch
from
afcddc5 to
9320f4f
Compare
| pub(crate) struct PathsWithCustomScalarType<'a, S: SchemaDefinition> { | ||
| schema_definition: &'a S, | ||
| paths: HashSet<Vec<String>>, | ||
| field_stack: Vec<String>, |
There was a problem hiding this comment.
Is it much hassle to make this Vec<&'a str>?
There was a problem hiding this comment.
Easy and defers an allocation when only inserting a path 👍
There was a problem hiding this comment.
Should we just delete this then since it doesn't do what it's name suggests?
There was a problem hiding this comment.
Hmm multiple rules do use PathRoot though. Ideally Path is removed and simplified down to just PathRoot? I'd rather defer that cleanup since it was cause a lot of (trivial) changes
2 participants
Reduce validator allocation overhead — ~28% faster on representative queries
Optimizations:
Also adds a criterion benchmark suite for the validation pipeline.
Note: this is a manually curated (with the help of Claude) and cleaned up version of an
/autoresearchrun