Skip to content
This repository has been archived by the owner on Mar 20, 2023. It is now read-only.

Performance improvement caching validation #474

Closed
jamesmoriarty opened this issue Feb 26, 2019 · 4 comments
Closed

Performance improvement caching validation #474

jamesmoriarty opened this issue Feb 26, 2019 · 4 comments

Comments

@jamesmoriarty
Copy link
Contributor

jamesmoriarty commented Feb 26, 2019

I'm currently using GraphQL to transform large documents and I've noticed a significant amount of time in validate. https://github.com/graphql/express-graphql/blob/master/src/index.js#L254

validate(schema, documentAST, validationRules);

The time spent on the first request:

parse: 21.042ms
hash: 3.727ms
validate: 965.500ms <
execute: 331.606ms

If I cache the result of validate. The time spent on the second request:

parse: 49.794ms
hash: 5.105ms
validate: 5.203ms <
execute: 336.634ms

As far as I can tell the schema is static, the query is the same with different arguments, and the validation rules are the same.

The change looks something like:

hash = hashFn(schema, documentAST, validationRules)

if (!cache[hash]) {
   cache[hash] = validate(schema, documentAST, validationRules);
}

const validationErrors = cache[hash];

As it's currently stateless, I'm trying to understand the trade-offs caching the result. I would appreciate any input from the contributors / maintainers. Basically, is this a terrible idea?

@IvanGoncharov
Copy link
Member

@jamesmoriarty GraphQL validate functionality was designed with a pretty similar case in mind:
https://facebook.github.io/graphql/June2018/#sec-Validation

Typically validation is performed in the context of a request immediately before execution, however a GraphQL service may execute a request without explicitly validating it if that exact same request is known to have been validated before. For example: the request may be validated during development, provided it does not later change, or a service may validate a request once and memoize the result to avoid validating the same request again in the future. Any client‐side or development‐time tool should report validation errors and not allow the formulation or execution of requests known to be invalid at that given point in time.

Moreover you can optimise it a little bit more by hashing query string before parsing it to AST.
AFAIK Facebook engineer went one step forward and hash queries during development so client sents only hash. Here is a talk with more details: https://youtu.be/G_zipR8Y8Ks

@jamesmoriarty
Copy link
Contributor Author

jamesmoriarty commented Feb 26, 2019

Thanks @IvanGoncharov. I really appreciate the response.

Is this a feature the maintainers would be interested in merging? If so is there any guidance on how the maintainers would like it implemented?

app.use('/graphql', graphqlHTTP({
  schema: MyGraphQLSchema,
  graphiql: true,
  validationCache: false <default> | <cache object e.g. {} or LRU>
}));

cc/ @wincent @zpao @kassens

@IvanGoncharov
Copy link
Member

Fixed in master.
@jamesmoriarty Thanks for PR 👍

@kaushik143
Copy link

@IvanGoncharov Is the proposal for validationCache accepted? I looking to do a similar kind of optimization for improving GraphQL performance

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants