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

fix(schema): Performance improvements for better AST caching #1379

Merged
merged 3 commits into from
Dec 20, 2023

Conversation

klausi
Copy link
Contributor

@klausi klausi commented Nov 12, 2023

Fixes #1312, reworked from #1314 .

Copy link
Contributor

@Kingdutch Kingdutch left a comment

Choose a reason for hiding this comment

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

I think in general this is a step forwards from where we are. With 417c308 I'd like to go a bit further that includes a few differences:

  • Preservation of the source locations allows for better debugging
  • Moving the separation of the base schema and the schema extension document into a single function provides more power to extending modules such as graphql_compose and graphql_core_schema which usually don't have a separation between a base schema or extensions. People who don't overwrite schema generation will usually use the default file loading methods anyway.
  • Schema printing itself is quite slow for the uncached case, an in-memory AST transformation is faster

Unfortunately the referenced commit is blocked by the need for a utility in graphql-php which is slightly smarter than what's included now.

My only request for this PR is to mark the two new methods buildSchema and getFullSchemaDocument as private. That way we can still jump to my proposed commit in 4.x without having to break everyone's code. If we keep them as protected then people might overwrite them and I no longer see how I'd get from this PR to the code in my commit without risking the breakage of users of the GraphQL module.

@klausi
Copy link
Contributor Author

klausi commented Nov 29, 2023

Agreed, pushed a commit to make them private.

@klausi klausi requested a review from Kingdutch November 29, 2023 15:35
@klausi klausi merged commit eb467d6 into drupal-graphql:8.x-4.x Dec 20, 2023
6 checks passed
@klausi klausi deleted the perf-build-schema branch December 20, 2023 12:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Performance Issues with larger schemas
2 participants