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

Add simple execute benchmarks for both sync and async execution #141

Merged
merged 1 commit into from
Oct 14, 2021

Conversation

jkimbo
Copy link
Member

@jkimbo jkimbo commented Oct 13, 2021

This PR adds 2 new benchmarks for to check execution both in a sync and async context.

Running these benchmarks has highlighted that the async execution in graphql-core is significantly slower than the sync execution (~70% slower). @Cito any idea why that might be? It's much higher than I would have expected.

CleanShot 2021-10-13 at 20 50 59@2x

@jkimbo jkimbo requested a review from Cito October 13, 2021 19:54
@Cito Cito merged commit 6f51c70 into main Oct 14, 2021
@Cito
Copy link
Member

Cito commented Oct 14, 2021

Hi @jkimbo, thanks for adding the benchmarks. The reason is that execution methods like execute_field(s) need to define and call async methods when resolvers are async. When everything is sync, this is not necessary. This is essentially the price we pay for the flexibility to use both sync and async resolvers, even in the same schema. Of course, if you create tests with no overhead for transport and backend access, then these additional calls will become dominant in the benchmark. In practice, I don't think that this is a problem. If you have any ideas how we can improve without changing the API and existing flexibility, let me know.

@Cito
Copy link
Member

Cito commented Oct 14, 2021

I always wanted to check whether we can make it faster by statically defining the await_... methods (i.e. making them static methods or functions), instead of defining them inside the respective methods. Not sure how much overhead the definition of the function inside the methods creates.

@jkimbo
Copy link
Member Author

jkimbo commented Oct 14, 2021

@Cito that makes sense however I created this benchmark because we are seeing significant performance issues when using async resolvers that are non trivial. For example @ThirVondukr created this benchmark that uses SQLAlchemy Async to fetch 250 records from the DB and it performs significantly worse than the equivalent sync versions: jkimbo/graphql-benchmarks#2
unknown
(the async version is labeled "SQLAlchemy + Strawberry")

I'll experiment with improving the async performance and let you know how it goes in #142

@syrusakbary
Copy link
Member

After having work on improving async performance in Quiver, some of the findings are:

  1. The way the asyncio event loop is implemented is not as performant as it could be (is not eager as in JS, it does lazy execution once you await)
  2. Using uvloop might improve a bit the performance

@Cito
Copy link
Member

Cito commented Oct 15, 2021

@syrusakbary Thanks for the feedback. Yes, the first issue you mention causes some overhead. I have also created issue #143 to discuss your second point.

@jkimbo Always good to have real-world benchmarks. It may well be that, particularly for simple queries and a local database and with connection pooling, the database access can be so fast that the Python overhead in the ORM or GraphQL-core really matters. The other question here is whether SQLAlchemy also has some overhead when using it asynchronously, so it is unclear which part of the effect is due to GraphQL-core. We need some more experiments and profiling here. I found an interesting article by Mike Bayer that touches these questions. Let's continue the discussion in #142.

@jkimbo
Copy link
Member Author

jkimbo commented Oct 15, 2021

@syrusakbary I don't understand what you mean here:

The way the asyncio event loop is implemented is not as performant as it could be (is not eager as in JS, it does lazy execution once you await)

Could you elaborate?

@Cito That is a fair point about there being overhead with async. I think it's fair to assume that there will always be some overhead with the async execution in sequential benchmarks, like we have here, since that's not really where async is beneficial. That is worth keeping in mind when we try and optimise the async call path.

@ThirVondukr
Copy link

@Cito

The other question here is whether SQLAlchemy also has some overhead when using it asynchronously, so it is unclear which part of the effect is due to GraphQL-core. We need some more experiments and profiling here.

Both GraphQL and REST(labeled as SQLAlchemy + FastAPI Rest) servers use same function to query data,
so i doubt it has anything to do with sqlalchemy.
https://github.com/jkimbo/graphql-benchmarks/blob/main/servers/fastapi-sqlalchemy/db/queries.py

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.

None yet

4 participants