-
Notifications
You must be signed in to change notification settings - Fork 221
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
Optimize OverlappingFieldsCanBeMerged #296
Comments
Thanks for reporting the issue! Also, the example query would be quite useful. I think the main issue is with the I will investigate the root cause. |
Thanks a lot for your prompt reply. I would love to work through this issue with you. Let me see if I can get a better stacktrace. |
This would be awesome! 👍 As a first step, I think it would be really helpful to verify that mentioned validation is indeed the bottleneck. Can you please exclude it in your tests: QueryReducerExecutor.reduceQueryWithoutVariables(
...
queryReducers = ...,
queryValidator =
new RuleBasedQueryValidator(
QueryValidator.allRules
.filterNot(_.isInstanceOf[OverlappingFieldsCanBeMerged])))
// or
Executor.execute(
...
queryReducers = ...,
queryValidator =
new RuleBasedQueryValidator(
QueryValidator.allRules
.filterNot(_.isInstanceOf[OverlappingFieldsCanBeMerged]))) If we can verify that it cases the slowdown, then we can focus on |
remove the |
Great, this means that we can isolate the issue and focus the effort on optimising the If you would like to investigate the issue as well, here are some useful resources:
|
Now that I think about it, recent bugfix introduced a change that reduces usage of the cache. i described this issue here: graphql/graphql-js#780 (review) This might be a promising path to investigate. For example, we can revert the bugfix and see how it performs without it. This commit introduced the bugfix: 9866a7b#diff-9ce8420e3a37d01d401ac1bc89e17e6e |
The validation rules are normally completely self-contained. So you can just copy/paste this old version of the validation under a different name in your test and add it in the |
QueryReducer.rejectComplexQueries
unable to reject really large query
Thanks. I'll try to find some time to debug this. |
Notice this problem can introduce serious problem. When doing |
Indeed, this is a concern. Assuming that parsing and So far I thought about following possible solutions that generally implemented outside of GraphQL execution:
I would be very interested to hear you opinion and ideas on possible solutions and countermeasures for this issue. |
For now, we added a simple HTTP filter to filter out request that is above certain threshold. |
@OlegIlyenko using old version of the validation file and got the result above. The situation is still bad but I got part of the result back and then got bunch of HTTP timeout failures. That said, it seems the bug fix does not matter much here. |
I see, thanks for investigating this possibility! I guess we need to look at the original algorithm and try to optimise it. |
Thanks. I wouldn't worry much now since we have put an HTTP filter at the very front. |
We also experienced this issue at XING and invested some time to fix it. Here is the PR: #458 and here is the description of the algorithm. |
The faster version is now the default one starting from https://github.com/sangria-graphql/sangria/releases/tag/v3.0.0 |
QueryReducer.rejectComplexQueries
become stuck with a 412KBytes GraphQL query selecting ~150000 fields. Did a profiling and stacktrace shows we are doing tremendous amount of work inOverlappingFieldsCanBeMerged
An example of the query is at here
We are using:
A screenshot of part of the stacktrace (sorry I cannot provide complete stacktrace).
The text was updated successfully, but these errors were encountered: