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

Use graphql-jit for subscriptions #5468

Closed
1 task
mattkrick opened this issue Sep 29, 2021 · 10 comments
Closed
1 task

Use graphql-jit for subscriptions #5468

mattkrick opened this issue Sep 29, 2021 · 10 comments

Comments

@mattkrick
Copy link
Member

the latest version of graphql-jit supports subscriptions 🎉 zalando-incubator/graphql-jit#74

that means we don't have to have different caching strategies for subscribe & execute calls. Should relieve a few CPU cycles on our server

AC

  • subscriptions are compiled & cached, just like executions

Estimate: 4 hours

@mattkrick mattkrick added this to To Prioritize in Sprint Board via automation Sep 29, 2021
@jordanh jordanh moved this from To Prioritize to Backlog in Sprint Board Oct 12, 2021
@jordanh
Copy link
Contributor

jordanh commented Oct 22, 2021

Does this issue need to be a pre-req to #5333 ?

@jordanh
Copy link
Contributor

jordanh commented Oct 26, 2021

Story Points: 5
See the discussion in Sprint Poker #​​7 – New Issues

Powered by Parabol

@mattkrick
Copy link
Member Author

not a pre-req, but depending on how hard the implementation of #5333 is, it might be worthwhile to do this first.

pros of doing this first:

  • subs are handled the exact same

cons:

  • additional change in how things work

@Dschoordsch Dschoordsch self-assigned this Nov 19, 2021
@Dschoordsch Dschoordsch moved this from Backlog to In Progress in Sprint Board Nov 19, 2021
@Dschoordsch
Copy link
Contributor

At the moment graphql-jit does not expose createSourceEventStream (https://github.com/zalando-incubator/graphql-jit/blob/f522b86bb2d776f5b201734f7be9c500838b3961/src/execution.ts#L1729). It's not too much work to expose it and create an upstream PR for it.
However the use case for createSourceEventStream is quite different than for query and subscribe. In the latter we need to run all field resolvers, thus it is beneficial to compile it beforehand. For the source event stream we're only using the result of the fields subscribe function while running the resolvers in a different process. As this is the use case source event streams are made for, it is counterproductive to compile all resolvers.
A separate implementation next to compileQuery which just does compileSourceEventStream could yield a small benefit as parsing the document (not relevant for us as it's cached) and preparing the execution context could be saved.

@Dschoordsch
Copy link
Contributor

I've did some performance measurements and it appears there is no significant performance increase by using createSourceEventStream from graphql-jit.

@mattkrick
Copy link
Member Author

this is a really fun problem to think about because I've often wondered if we could remove graphql from the web/socket server completely.

what if each websocket subscribed to its own redis pubsub channel & the graphql executor handled source & response streams & then sent the final result to the websocket server?

not something we need to change anytime soon, but it's a fun problem to consider

@Dschoordsch
Copy link
Contributor

Since I didn't want my work go to waste I added some benchmark on graphql-jit side. Although I couldn't see any performance improvement when running our app, just looking at createSourceEventStream I gained between 50% and 100%. I created a PR to discuss whether or not the owners are interested in this change.

@Dschoordsch
Copy link
Contributor

@mattkrick That sounds like a fun problem indeed.
What we have now:
GQLExecutor1 publishes -> Redis -> WebSocket event source stream -> Redis -> GQLExecutor1 -> Redis -> WebSocket send

If we just move it over to the executor, we could end up with:
GQLExecutor1 publishes -> Redis -> GQLExecutor2 event source stream -> Redis -> GQLExecutor1 resolves -> Redis -> GQLExecutor2 send -> Redis -> WebSocket send

While ideally we could just:
GQLExecutor1 publishes locally -> GQLExecutor1 magic distributed event source stream involving Redis -> GQLExecutor1 resolves -> GQLExecutor1 magic distributed reply stream -> Redis -> WebSocket send

We only need to implement the 2 magic parts. We could also pack the magic into the publishing part. Add some Redis script which finds all the subscribers + variables etc. and then sends these all over prepared to GQLExecutor1.

Once we are at that stage, we could even think about refactoring some of our subscriptions to be only team or meeting dependent and not viewer dependent. This way we could resolve these subscriptions once and send the same reply to all subscribers.

@Dschoordsch Dschoordsch moved this from In Progress to Stuck in Sprint Board Dec 13, 2021
@jordanh jordanh removed the Squad: 2 label May 25, 2022
@github-actions
Copy link
Contributor

Stale issue

@github-actions github-actions bot added the stale label Nov 25, 2022
@jordanh jordanh added icebox and removed stale labels Dec 20, 2022
@jordanh
Copy link
Contributor

jordanh commented Dec 20, 2022

Iceboxing!

@jordanh jordanh closed this as completed Dec 20, 2022
Sprint Board automation moved this from Stuck to Done Dec 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Sprint Board
  
Done
Status: Done
Development

No branches or pull requests

3 participants