-
-
Notifications
You must be signed in to change notification settings - Fork 138
validation.validate slowness #117
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
Conversation
Correction: Obviously, the variable checks removal is not safe. I just did that to inspect how expensive the recursive visitor was. |
# NoUndefinedVariablesRule, | ||
# NoUnusedVariablesRule, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be reverted
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See below, the default rules should not be changed.
KnownDirectivesRule, | ||
UniqueDirectivesPerLocationRule, | ||
KnownArgumentNamesRule, | ||
UniqueArgumentNamesRule, | ||
ValuesOfCorrectTypeRule, | ||
ProvidedRequiredArgumentsRule, | ||
VariablesInAllowedPositionRule, | ||
# VariablesInAllowedPositionRule, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be reverted
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the default validation behavior should be the same as in GraphQL.js which reflects the official specs. If you want to omit certain validation rules, see #70.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, this was more to demonstrate that these rules are quite expensive compared to the others.
I wonder if these could be separated into "essential" and "friendly". For example, I think NoUnusedVariablesRule
helps a user develop a query, but may not be needed in a production environment where there isn't a developer to provide feedback to.
e08a1f9
to
56614ce
Compare
pyproject.toml
Outdated
@@ -52,5 +52,5 @@ tox = "^3.19" | |||
target-version = ['py36', 'py37', 'py38'] | |||
|
|||
[build-system] | |||
requires = ["poetry>=1,<2"] | |||
requires = ["poetry>=1,<2", "setuptools"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was added to make pip install -e .
work; I need to research whether poetry
has something built in. Not really for landing.
c2ab270
to
6e46a09
Compare
62a58c4
to
687118c
Compare
Improve the validation performance by memoising the hashes and visitor function lookups. This improves the `test_validate_introspection_query` benchmark
687118c
to
24bcd0c
Compare
4f69c9b
to
1804bd7
Compare
0ac79de
to
3f40fc2
Compare
Small scratch script demonstrating validation.validate slowness.
You can run by installing graphql-core in a venv, then running scratch.py. Personally,
I then run it through gprof2dot for visualisation
Demonstration of #116
By memoising the various visitors, I got the runtime proportion down from ~85% to ~70%. Removing the variable checks removed another 10%. I'm not sure how safe either of these were.