Multiple critical performance improvments. #1314
Conversation
There was a problem hiding this comment.
While I understand that these changes make things faster, there's also a few things that cause these changes not to be mergeable as-is and I think larger changes are required.
We don't know when a schema comes from the cache and when it's loaded from disk for the first time. The current change disables validation in all cases, but it's important that the schema is at least validated at some point.
Your change always removes location data, but there are cases when location data might be very useful, for example when developing the schema. Additionally there may be production cases (like profiling) which might want location data too. So always disabling it is not an option. We'll have to identify when it's appropriate to disable, or create the right configuration for end users.
I'm not entirely sure what your change for the schema extension is trying to do, but it appears as if we're rebuilding the schema entirely, rather than only building the extension on top of an existing schema? This feels like it might also need a rethink (or a better explanation of what's happening).
EDIT: Ah the original issue has more explanation of what's going on :)
| @@ -119,13 +120,14 @@ public function getSchema(ResolverRegistryInterface $registry) { | |||
| $extensions = $this->getExtensions(); | |||
| $resolver = [$registry, 'resolveType']; | |||
| $document = $this->getSchemaDocument($extensions); | |||
| $options = ['assumeValid' => TRUE]; | |||
There was a problem hiding this comment.
we should only do this in prod, so we should check empty($this->inDevelopment)
| * @throws \GraphQL\Error\Error | ||
| * @throws \GraphQL\Error\SyntaxError | ||
| */ | ||
| public function getExtensionSchemaAst($schema, $extendSchema) { |
There was a problem hiding this comment.
this is confusing now, we already cache an AST in getExtensionDocument(). here we cache something again, why?
| } | ||
|
|
||
| $schema = SchemaExtender::extend($schema, $extendSchema); | ||
| $schema_string = SchemaPrinter::doPrint($schema); |
There was a problem hiding this comment.
this is clever, I like the idea! We construct a final AST with all extensions and then cache that 👍
|
This was done, closing. |
Multiple performance improvements:
noLocationto avoid recursions