-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Current main is up to 50% slower than previous major version #4008
Comments
СС @yaacovCR |
Are you comparing latest main to https://www.npmjs.com/package/graphql/v/17.0.0-alpha.3 or to v16.x? |
If you are available to walk me through your benchmark and how you have excluded collectfields/buildfieldplan => as far as I can tell that’s mostly what changed from alpha 3 |
Our GraphQL executor is based on graphql-js, latest update that added defer/stream was based on alpha.3 that had no regressions. When I ported the latest code that was added in main it became significantly slower. I then measure the latest main and it was the same regression in graphql-js. Here is our benchmark suite (execution is the only important one for this): |
That’s very helpful. I hope to have time to look into this. |
These are my results using your benchmarks for graphql-js only on my machine, not scientific, but seems to show the problem is between alpha-2 and alpha-3. I ran the benchmarks x2 each for main, alpha-3, alpha-2.... More investigation to follow.... I am also curious how you pinpointed that the problem is outside main Query parsing Query parsing alpha3 Query parsing Query parsing alpha2 Query parsing Query parsing |
Just to point out the initially obvious, the major change between the alphas was the introduction of deduplicated incremental delivery. |
looks like #4026 improves performance on alpha3 in the direction of alpha2 (note that today my machine is overall smaller, compare to below runs for alpha.2 and alpha.3 rather than the above ops/sec). definitely might be further room for improvement there! (would be great if we could get benchmarking into the PR checks/workflow) graphql@17.0.0-alpha.3.canary.pr.4026.5922420b3b235970ee230497190e28c8290c8f16 (#4026) alpha.3 alpha2 |
further improved performance with latest canary #4026 (comment) -- but still not as good as alpha.2 |
Hi @freiksenet and @vladar! Are you able to test again with your benchmarks using the latest canary release from #4026 (comment) ...on my machine this latest canary essentially pulls even with alpha.2 It is also at the limit of my ideas about how to improve further. Thanks so much for opening this issue! |
Closing, feel free to reopen as necessary. |
Based on our benchmarks, latest implementation is up to 50% slower than the one in last 17 alpha. The slowness is based on extra work done after
collectFields
-buildFieldPlan
. I think it's quite a major regression that should be addressed before it's published as a release.The text was updated successfully, but these errors were encountered: