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

fix performance degradation on handling conflict fields #212

Merged
merged 5 commits into from
Feb 14, 2024

Conversation

mrtc0
Copy link
Contributor

@mrtc0 mrtc0 commented Feb 1, 2024

When a crafted query is received, it significantly exhausts computing resources like the CPU, which negatively impacts response time.
For example, adding the following benchmark takes about 7 ~ 8 seconds in my environment (M1 Mac / 32GB).

from graphql import build_schema, parse, validate
from graphql.utilities import get_introspection_query

from ..fixtures import big_schema_sdl  # noqa: F401

def test_validate_conflict_fields_query(benchmark, big_schema_sdl):  # noqa: F811
    schema = build_schema(big_schema_sdl, assume_valid=True)
    fields = "__typename " * 1000
    query = parse('query {{ {fields} }}'.format(fields=fields))
    result = benchmark(lambda: validate(schema, query))
    assert result == []
$ time poetry run pytest tests/benchmarks/test_validate_gql_conflict_fields.py
Executed in    8.23 secs    fish           external
   usr time    7.60 secs    0.06 millis    7.60 secs
   sys time    0.22 secs    1.01 millis    0.21 secs

This is a similar problem in graphql-js and has been fixed.
This PR has been ported to make the same changes.

This change will greatly improve performance:

$ time poetry run pytest tests/benchmarks/test_validate_gql_conflict_fields.py
Executed in    1.93 secs    fish           external
   usr time    1.47 secs    0.07 millis    1.47 secs
   sys time    0.18 secs    1.22 millis    0.18 secs

@mrtc0 mrtc0 requested a review from Cito as a code owner February 1, 2024 08:34
@Cito
Copy link
Member

Cito commented Feb 1, 2024

Thanks. Will look into this as quickly as possible.

@mrtc0
Copy link
Contributor Author

mrtc0 commented Feb 9, 2024

@Cito Hi, Any update would be appreciated.

@Cito
Copy link
Member

Cito commented Feb 9, 2024

It's on my list for this weekend. Sorry, I can manage this project only in my spare time.

Copy link
Member

@Cito Cito left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for submitting this PR, very well done!

The same_arguments function can be simplified a bit by returning early if one of the values is false, instead of building the list and then checking all results.

The test for OverlappingFieldsCanBeMergedRule from graphql.js should be added as well, this will probably also solve the problem that line 611 is not covered.

It would be also good to make the benchmark test more similar in name and content to the GraphQL.js one - I always try to replicate things as closely as possible, since this makes maintenance much easier.

Let me know if you would like to make these changes, otherwise I will do it immediately after merging this PR.

@mrtc0
Copy link
Contributor Author

mrtc0 commented Feb 13, 2024

@Cito Thank you for the review. I've made changes to the code based on your advice.

@mrtc0 mrtc0 requested a review from Cito February 13, 2024 03:22
Copy link
Member

@Cito Cito left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot, looks good. Will fix the formatting issues after merging.

@Cito Cito merged commit e8035a8 into graphql-python:main Feb 14, 2024
8 of 9 checks passed
@Cito
Copy link
Member

Cito commented Feb 14, 2024

A release with this fix will be available soon.

@datur
Copy link

datur commented Jun 4, 2024

Hey @Cito 👋 do you know when you might be putting up a release that includes this fix? If there's anything I can do to help it along let me know

@Cito
Copy link
Member

Cito commented Jun 4, 2024

@datur It's already contained in v3.3.0a5. It's still labelled as an alpha release because the corresponding GraphQL.js version 17 is also still labelled as alpha.

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.

3 participants