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

Interpreter #1394

Merged
merged 119 commits into from
Oct 5, 2018
Merged

Interpreter #1394

merged 119 commits into from
Oct 5, 2018

Conversation

rmosolgo
Copy link
Owner

@rmosolgo rmosolgo commented Apr 5, 2018

Problem:

  • GraphQL execution takes a lot of time and memory by taking fragments on abstract types and .duping them for each possible concrete type. This is a serious problem for Node queries written by Relay, see WIP - Benchmarking validation with a bigger schema and query #1172 . In fact, in many cases, this is exponential time complexity, bad!
  • GraphQL execution can't be completely prepared, since @skip is hardcoded in Rewrite, even static queries must go through that step.
  • GraphQL execution is not hackable. There are odd hooks here and there, but they're not really hackable.

Solution:

  • Stop "concrete-izing" fragments, instead, use a pure interpreter to execute queries (remove the Rewrite step)
  • Remove the rewrite step altogether (TODO: we probably need some kind of caching during query execution, eg, for coerced arguments)
  • Call execution methods on the type classes themselves, so that they can be overridden by users (🙀 😎 )

Problems with the solution:

  • Query analyzers go out the window
  • irep_nodes go out the window
  • So, some advanced tooling goes out the window.

Possible solutions to problems with the solution:

  • Replace query analysis with before-execution pass of the interpreter, allow analyzers to register methods which are called along the query tree, much like an interpretation pass

Possible problems with possible solutions to problems with the solution:

  • It would probably support the same kinds of tools, but it would require a lot of work.

TODO

  • Lazy values
  • Authorization
  • GraphQL::ExecutionError
  • SKIP
  • Tracing
  • Nil propagation
  • rewrite with cps instead of blocks? reconsider this as a perf consideration and customization option, later
    • I think passing a block counts as passing a continuation. It's callable, it's bound to the local environment, and as a bonus, it's really efficient since it's built-in to Ruby and we don't have to capture them into Procs.
    • One problem with blocks is the long call traces. Could these be cleaned up? same for the .with_* methods.
  • Find a more elegant solution than :__temp_running_interpreter? (Something that doesn't clutter user's ctx.to_h)
  • Add standard handling for LateBoundType
  • Make sure that new-style definitions are passed to introspection (TypeType, FieldType, SchemaType, ArgumentType)
  • Profile can be done after initial merge
  • Custom directives can be done after initial merge
  • rescue_from skipped the tests for this, it would be good to replace it with something.
  • Multiplex, specifically batching across multiplex context
  • Test tooling to run both new and old executes with Dummy::Schema (and Jazz::Schema ?)
  • Make every test schema run the interpreter in CI
  • Clean integration and docs for interpreter
  • Find all # TODOs and address them
  • Rename and document resolve_field_2
  • Double-check all switches on TESTING_INTERPRETER in the test suite

@rmosolgo rmosolgo mentioned this pull request Jun 20, 2018
8 tasks
@rmosolgo rmosolgo mentioned this pull request Oct 5, 2018
19 tasks
@rmosolgo rmosolgo merged commit edf4946 into 1.9-dev Oct 5, 2018
@rmosolgo rmosolgo deleted the interpreter branch October 5, 2018 19:50
@emilebosch
Copy link
Contributor

emilebosch commented Oct 6, 2018

Good work! Is there any benchmark on the performance increase/decreased memory usage?

@rmosolgo
Copy link
Owner Author

rmosolgo commented Oct 6, 2018

See #1886

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

Successfully merging this pull request may close these issues.

4 participants