RFC: Make schema serialisable and cachable

[unclecheese]

Pull Requests

• WIP: Schema caching #202

Overview

The performance limitations of this module are well documented and long-lived. Attempts to solve the issue have shown limited success. While the recent work to cache the resolver computations has been merged and provides notable benefits, this will never be a scalable offering until we can find a way around building the schema dynamically on every request.

The most obvious and least complex way to do this is to cache the schema and invalidate it when it changes. Those are both hard, because, respectively:

• The schema is built using a plethora of anonymous functions and other runtime context
• The schema is designed to take on implicit changes as DataObjects change their definitions.

Suggested approach

This can be done with three reasonably-sized changes that would not break any APIs.

1: Remove all anonymous functions from the schema

An inventory of all the anonymous functions being used in type and query creators shows that most of them are paranoid countermeasures against Manager race conditions. Due to the declarative nature of the schema, the manager state is not always ready to define a query, and therefore, wrapping everything in a closure is the easiest safeguard. Most of these functions do not appear to be necessary if we implement lazy loading correctly.

Lazy load types properly

A recent addition to the graphql-php API was the typeLoader property, which allows users to define a global accessor for all types:

new Schema([
  'typeLoader' => function ($name) use ($someState) {
    return $someState[$name];
  }
]);

The only constraint is that type instances must be singletons.

Eliminate lazy loading of fields

Most of the closures around field definitions are protections against attempting to access type instances too early. A lazy loaded type layer would eliminate the need for most of these workarounds.

Encourage concrete resolvers

Right now, nearly all resolvers that ship with the module are defined at runtime. The benefits of moving these to class definitions would be twofold:

• The fields that use them would be serializable
• They would become injectable.

Before:

class Read extends ListQueryScaffolder
{
  protected function createResolver()
  {
    return function () { ... };
  }
}

After:

class Read extends ListQueryScaffolder
{
  $dependencies = [
    'Resolver' => ResolverInterface::class . '.read',
  ];
}

Note that it is already possible to do this with the scaffolder. You can simply point the resolver at an instance, or a FQCN of a ResolverInterface.

Attempts to serialise a field using a closure as a resolver will throw.

2. Make type and field creators serialisable

In the case of the scaffolders, we're already halfway there. All scaffolders implement the ConfigurationApplier interface, which means they can hydrate themselves from a known array structure. Creating the inverse of applyConfig() -- something like dumpConfig() would suffice, provided reciprocity between the two.

dumpConfig() would be a good place to throw if the scaffolder is using anonymous functions as resolvers.

class MyScaffolder implements ConfigurationApplier, ConfigurationExporter, Serializable
{
  public function serialize()
  {
     return serialize($this->dumpConfig());
  }
  
  public function unserialize($data)
  {
     return $this->applyConfig(unserialize($data));
  }
}

For tidiness, a new interface, ConfigurationConsumer could extend both ConfigurationApplier and ConfigurationExporter without breaking anything.

3. Cache and invalidate

Caching the Schema object itself probably has diminishing returns relative to the benefit of caching the Manager instance, especially considering typeLoader would be incompatible with most caching strategies.

Example:

$manager = $this->getCache()->get('Manager');
return new Schema([
  'types' => $manager->getTypes(),
  'queries' => $manager->getQueries(),
  'mutations' => $manager->getMutations(),
  'typeLoader' => function ($name) use ($manager) {
    return $manager->getType($name);
  }
]);

Invalidating the cache hinges primarily on config. A hash of the entire config would suffice as a cache key for most graphql implementations.

if ($this->getCache()->get($sha) {
  // warm cache
} else {
  // regenerate schema
}

However, TypeCreator and ScaffoldingProvider instances mutate the schema procedurally. Therefore, a more accurate cache key may be something like:

md5($configSha . $contentsOfAllScaffoldingProviders . $contentsOfAllTypeCreators);

Still, though, there is a chance that some code-level change to a dataobject could alter its effect on the schema -- for instance, a get_extra_config() change.

Hashing all dataobjects and extensions would be too aggressive and still imperfect. With that taken into consideration, schema changes would require a ?flush.

Further thought

• Investigate possible impact on with asset-admin and versioned-admin
• CLI based task to generate the cache outside of the request cycle, e.g. on deployment
• Update documentation to steer developers away from practices that defeat caching. We want to be performant by default, slow by opt-in.
• Given the amount of reliance we'll have on data structures, it may be time to use proper validation of the configuration, in lieu of all the imperative checks we're doing now. See symfony's validator

References

• RFC Serverside graphql query caching #164
• Cache GraphQL schema #130
• http://webonyx.github.io/graphql-php/type-system/schema/#lazy-loading-of-types

[chillu]

Encourage concrete resolvers

Looking at actual implementations like DataObjectScaffolderExtension in versioned, would we be talking one resolver per field here? They're one-liners, so that's pretty clumsy.

• Option A: Look at serialising anonymous functions which don't close over other values, e.g. with https://github.com/jeremeamia/super_closure. I'm not even sure we'd want this in core, since those approaches naturally will have to use eval() and that's commonly flagged by security audits.
• Option B: Allow static definition of (singleton) method calls on resolver classes, rather than implementing a single resolve() method.

So $this->manager->getType(<type>) would still be the equivalent of $registry->get(<type>) in the lazy loading types example, right?

Caching the Schema object itself probably has diminishing returns relative to the benefit of caching the Manager instance

Agree, hopefully most of the legwork will be in all the config parsing/merging, ORM introspection and scaffolding, rather than constructing the Schema object.

$manager = $this->getCache()->get('Manager');
return new Schema(

Detail niggle: I'd prefer if we kept Manager->schema() rather than externalising this logic into the object creating Manager (which I think is Controller).

Update documentation to steer developers away from practices that defeat caching. We want to be performant by default, slow by opt-in.

Yeah you'd need to update all the setResolver(function()...) examples in the docs, those would all be considered bad practice now, right? I'm wondering if we should even continue to provide this API. Unless we can create a partially cached schema (instead of throwing on serialisation), one inlined resolver would defeat ALL caching benefits. Which might be acceptable for a small, low concurrency custom schema, but will make the entire CMS slower when applied to the CMS schema.

[unclecheese]

would we be talking one resolver per field here?

No, certainly not. For the resolvers that are simply accessing properties, I think something like the static definition of resolver methods would make sense, but it needs thinking through.

So $this->manager->getType() would still be the equivalent of $registry->get()

Yes, depending on the performance gains. If caching the manager doesn't make enough of an impact, we might have to look at an alternative solution using something more primitive, perhaps, like a simple array. But I really think the manager is the lion's share of the work here.

Detail niggle: I'd prefer if we kept Manager->schema() rather than externalising this logic

Yes, no plans on putting Schema into the controller. That doesn't make sense to me, either. It's a concern of manager. Controller should only understand the concept of query.

I'm wondering if we should even continue to provide this API

It's good for rapid prototyping. You can spin up a schema in some very simple procedural code. I'm actually leaning more toward keeping it in the docs as the introduction "look how easy it is" section rather than overwhelming the reader with writing a new single-method class definition.

[unclecheese]

Benchmarks

There's been previous research on this topic, but for good measure, I wanted to get some numbers using the latest version using multi-schema.

I've done some testing on lightly populated and densely populated schemas with varying numbers of types. We clearly have a lot to gain by implementing caching.

Test conditions

• Macbook Pro 2016
• Standard SS vagrant box
• PHP 7.1
• 512M memory

Test query

query readOnePage(ID: 1) {
  Title
}

Test results

Number of types	All fields	All fields, operations	All fields, operations, nested queries
10	2.6s	3.5s	4.0s
50	8.5s	10.75s	12.5s
100	17s	23s	27s
200	39s	48s	60s

Explanation

• All fields : The dataobject (or page) type has all of its db and has_one fields exposed on the type
• All fields and operations: The dataobject has all its fields and all CRUD operations (as well as versioned, if applicable) enabled.
• All fields, operations, and nested queries: All fields, CRUD operations, and any has_many, many_many relationships exposed as nested queries.

[chillu]

Discussed a bit more offline with Aaron:

• We should be able to implement a global field resolver which is aware of Object->obj() in our data model to resolve fields (Author field calls $myObj->obj('Author')). This should capture most simple resolver use cases
• Custom resolver logic should be implemented through resolver classes (with a single resolve() method) by default since that gets you type hinting through PHP interfaces
• By allowing callables through array syntax, we allow serialisation of the manager (not easily possible with callables through anonymous functions). This can be a middle ground to create one resolver class per type (e.g. Author => ['MyMemberResolver', 'resolveAuthor']).
• Caching should focus on dev ergonomics vs. production performance tradeoffs: It should allow for explicit cache invalidation through a task as part of production deployments. But it should also minimise mental overhead for developers during development, specifically by a semi-smart integration into ?flush=1. This is only possible if it doesn't significantly increase flush times, otherwise we'll need to resort to a separate cache invalidation task for developers. Given that eventually, every field in a DataObject will be represented in GraphQL for the CMS to interact with it, these will be high use tasks - and worth optimising.

[unclecheese]

On further thought, ?flush doesn't seem like a great fit for explicit cache invalidation, as the benchmarks show that a moderately populated schema builds in about 30s.

Something like dev/schema/my-schema-name feels more appropriate and would be considered another post-deployment build step, like dev/build.

[robbieaverill]

Would generating the cache for the schema be a required step in order to use the site? If so, I think it should be bundled into either ?flush or dev/build, otherwise people will forget. If it's not and is an opt in enhancement that you can choose, then sure =)

[unclecheese]

OK, I have a rough POC of this working. It took on a much different direction than outlined above, but it looks promising.

High level

• Upgrade to graphql-php: 0.12, which affords us several new APIs that make this a whole lot easier, including typeLoader.
• Serialisation is tricky because graphql-php needs types to be guaranteed singletons. You need referential equality across the schema. Every unserialize call gets you a new reference. That made for some really ugly workarounds to ensure the types came back with the same references, and that work was quite expensive, costing ~800ms with 100+ dataobjects and all operations.
• Make all primitives serialisable by enforcing no closures. More use of callable array syntax, and new ResolverFactory class that allows lazy loading of context-dependent resolvers.
• Use code generation to dump schema in to an executable PHP class of lazy-loaded types.
• You have to run dev/schema/<schema-key> every time you change your schema

New Benchmarks

The results for all tests above are ~200ms in a local set up (Vagrant with NFS), and 90ms on an SSP virtual stack.

Ugliness

• Schema builds take ~15 seconds for 100 dataobjects (1,000 types)
• GraphiQL introspection query is still painfully slow. It circumvents all the typeLoader stuff. Will need a new query middleware for caching the introspection response.
• Subclasses for all primitives, e.g. SerialisableObjectType, SerialisableFieldDefinition, etc, which overload the visibility of private members. Quite a bit of code duplication. These subclasses may not be necessary if we turn the code generation into an agnostic service.
• Probably lots of other things. It's a POC.

Pull request

Forthcoming.

[unclecheese]

This has taken yet another left turn. Serialisation, while effective at reducing schema build time, was not performant enough for the big stage. The new implementation uses code generation to export your schema into a monolithic class where types are registered for the schema to lazy load.

Lots to discuss, review, and argue about. Please have your say.

#202

[lekoala]

so i noticed one of my data object with a couple of gridfield was very slow (cf screenshot)... could this be related to this? i've tried the GridFieldLazyLoader but it doesn't seem to help much (actually, it may even be slower because it's slipping the work across multiple requests that end up being slow that one request)

[ScopeyNZ]

Thanks @lekoala . This is a known issue: silverstripe/silverstripe-admin#700 . Those SearchForm requests aren't actually graphql - they're just usual REST calls.

[lekoala]

Ah great sorry :) so it's only the "types" call that is relevant to this issue?

It's really sad to this all these calls being made it makes that whole back end much slower than it should. It's certainly one common complaints from my customers.

[ScopeyNZ]

only the "types" call that is relevant to this issue

Yes. There's some configuration that can be turned on to cache this response to a file that's served from your assets folder: SilverStripe\GraphQL\Controller::$cache_types_in_filesystem. It's off by default because it can be finicky to get right depending on your infrastructure.

The types call is required and won't change. As SS moves towards becoming more of a single page app then this will only happen once while you're using the CMS 😉

Also note that sessions in PHP are blocking by default which is probably why each request takes longer than the previous (assuming they're started at the same time). The actual execution time is probably going to be the time shown minus the previous request.

[lekoala]

@ScopeyNZ Ok I look into the SilverStripe\GraphQL\Controller::$cache_types_in_filesystem it seems easy enough to enable without too many downsides a on standard setup. When can it be "finicky"?

You are quite right about the sessions. Do you have any idea how to close session early for stateless controller endpoints by default in the cms? That would indeed help a lot !

[chillu]

This has been merged into GraphQL v4 now (although in quite a different implementation). Rather than serialising the schema, we generate the code for it.