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

Cache for parsing the schema #137

Merged
merged 3 commits into from
Apr 19, 2018

Conversation

koddsson
Copy link
Contributor

@koddsson koddsson commented Apr 19, 2018

👋 Hey hey!

When working with large GraphQL schemas (like GitHubs v4 API schema) we've noticed that eslint could run a bit slower. We narrowed it down to eslint-plugin-graphql adding ~500ms per file to the run time of eslint.

By adding a simple cache object for the schema results of parseOptions we can bring that time down pretty signifigantly.

Here's a example eslint run in one of our projects at GitHub:

Before this patch:

⚡ time TIMING=1 ./node_modules/.bin/eslint .
Rule                            | Time (ms) | Relative
:-------------------------------|----------:|--------:
prettier/prettier               |  5956.319 |    57.1%
graphql/no-deprecated-fields    |  1427.804 |    13.7%
import/namespace                |   810.107 |     7.8%
no-unused-vars                  |   274.724 |     2.6%
import/no-deprecated            |   148.324 |     1.4%
react/no-direct-mutation-state  |   124.988 |     1.2%
react/no-unused-prop-types      |   117.736 |     1.1%
react/prefer-stateless-function |   115.713 |     1.1%
import/named                    |    86.341 |     0.8%
react/no-deprecated             |    77.588 |     0.7%
TIMING=1 ./node_modules/.bin/eslint .  174.30s user 4.23s system 97% cpu 3:02.83 total

After this patch:

⚡ time TIMING=1 ./node_modules/.bin/eslint .
cache miss
Rule                                | Time (ms) | Relative
:-----------------------------------|----------:|--------:
prettier/prettier                   |  2788.298 |    65.9%
import/namespace                    |   415.556 |     9.8%
no-unused-vars                      |    68.644 |     1.6%
import/no-deprecated                |    60.269 |     1.4%
react/no-unused-prop-types          |    52.729 |     1.2%
graphql/no-deprecated-fields        |    52.299 |     1.2%
import/named                        |    48.913 |     1.2%
react/no-direct-mutation-state      |    45.669 |     1.1%
react/void-dom-elements-no-children |    45.446 |     1.1%
react/prefer-stateless-function     |    38.467 |     0.9%
TIMING=1 ./node_modules/.bin/eslint .  10.07s user 0.43s system 125% cpu 8.356 total

(There seems to be some prettier/prettier weirdness going on here as well)

I wasn't quite sure on how to test this but am open for suggestions if you've got any.

TODO:

  • Make sure all of the significant new logic is covered by tests
  • Rebase your changes on master so that they can be merged easily
  • Make sure all tests pass
  • Update CHANGELOG.md with your change
  • If this was a change that affects the external API, update the README

Pull Request Labels

  • feature
  • blocking
  • docs

/label performance

@ghost ghost added the performance label Apr 19, 2018
@jnwng jnwng merged commit 7e248ce into apollographql:master Apr 19, 2018
@jnwng
Copy link
Contributor

jnwng commented Apr 19, 2018

simple enough — thank you for providing benchmarks to make this easier to merge! live in [email protected]

@patsissons
Copy link

looks like this has broken support for graphql-config with multiple schemas defined, as the cache hits are short circuiting schema selection. I'll write up a proper issue and create a PR to fix this shortly.

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

Successfully merging this pull request may close these issues.

3 participants