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

perf improvements for fragment cycles validation #2786

Merged

Conversation

jbellenger
Copy link
Contributor

@jbellenger jbellenger commented Apr 8, 2022

We've observed that some of our real-world queries are taking seconds to validate.
When profiling these queries, it looks like the validation time is dominated by the NoFragmentCycles rule.

This PR reworks NoFragmentCycles to not traverse into fragments that have already been visited anywhere in a fragments transitive dependencies. This is similar behavior to the graphql-js implementation and adheres to 5.5.2.2 Fragment spreads must not form cycles. This PR should result in no external behavioral changes.

This change improves the validation time for one of our fragment-heavy queries by over 100x.
I've added an anonymized version of our schema and query as new resources, which are measured below as a new "manyFragments" benchmark:

Before:

Benchmark                         Mode  Cnt     Score    Error  Units
ValidatorBenchmark.largeSchema1   avgt   40     0.070 ±  0.002  ms/op
ValidatorBenchmark.largeSchema4   avgt   40    24.214 ±  0.168  ms/op
ValidatorBenchmark.manyFragments  avgt   40  1255.602 ± 11.135  ms/op

After:

Benchmark                         Mode  Cnt   Score   Error  Units
ValidatorBenchmark.largeSchema1   avgt   40   0.069 ± 0.001  ms/op
ValidatorBenchmark.largeSchema4   avgt   40  25.289 ± 0.299  ms/op
ValidatorBenchmark.manyFragments  avgt   40  10.979 ± 0.151  ms/op

Side note:
I had some issues using Anonymizer with built-in directives. It seemed to generate assertion errors when @include was applied to a selection set, like:

fragment Foo on Bar {
  field @include(if: true) {
     x
  }
}

I didn't have the context to understand what Anonymizer expected in this case so I didn't dig into it too much, though I'm happy to privately share a query+schema that triggered this if that helps.

@@ -81,31 +81,6 @@ class NoFragmentCyclesTest extends Specification {
errorCollector.getErrors().isEmpty()
}


def "circular fragments"() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this test is a duplicate of "no co-recursive spreads in floating fragments"

errorCollector.containsValidationError(ValidationErrorType.FragmentCycle)
}

def 'no spreading itself directly'() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this test is a duplicate of "no self-spreading in floating fragments"

James Bellenger added 2 commits April 8, 2022 15:34
don't recheck fragments that have been checked anywhere in the fragments
tree, not just the immediate ancestors
@andimarek andimarek added this to the 19.0 milestone Jul 26, 2022
# Conflicts:
#	src/main/java/graphql/validation/rules/NoFragmentCycles.java
#	src/test/groovy/graphql/validation/rules/NoFragmentCyclesTest.groovy
#	src/test/java/benchmark/ValidatorBenchmark.java
@dondonz
Copy link
Member

dondonz commented Jul 26, 2022

FYI @jbellenger I updated your branch and resolved merge conflicts to get this ready for release. Thanks so much!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants