-
-
Notifications
You must be signed in to change notification settings - Fork 137
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
fix performance degradation on handling conflict fields #212
Conversation
Thanks. Will look into this as quickly as possible. |
@Cito Hi, Any update would be appreciated. |
It's on my list for this weekend. Sorry, I can manage this project only in my spare time. |
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.
Thanks a lot for submitting this PR, very well done!
The same_arguments
function can be simplified a bit by returning early if one of the values is false, instead of building the list and then checking all results.
The test for OverlappingFieldsCanBeMergedRule from graphql.js should be added as well, this will probably also solve the problem that line 611 is not covered.
It would be also good to make the benchmark test more similar in name and content to the GraphQL.js one - I always try to replicate things as closely as possible, since this makes maintenance much easier.
Let me know if you would like to make these changes, otherwise I will do it immediately after merging this PR.
@Cito Thank you for the review. I've made changes to the code based on your advice. |
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.
Thanks a lot, looks good. Will fix the formatting issues after merging.
A release with this fix will be available soon. |
Hey @Cito 👋 do you know when you might be putting up a release that includes this fix? If there's anything I can do to help it along let me know |
@datur It's already contained in v3.3.0a5. It's still labelled as an alpha release because the corresponding GraphQL.js version 17 is also still labelled as alpha. |
When a crafted query is received, it significantly exhausts computing resources like the CPU, which negatively impacts response time.
For example, adding the following benchmark takes about 7 ~ 8 seconds in my environment (M1 Mac / 32GB).
This is a similar problem in graphql-js and has been fixed.
This PR has been ported to make the same changes.
This change will greatly improve performance: