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 pytest-benchmark and port the benchmarks from GraphQL.js #55

Merged
merged 8 commits into from
Sep 10, 2019

Conversation

ktosiek
Copy link
Contributor

@ktosiek ktosiek commented Sep 4, 2019

This is a start for working on #3 - currently only one benchmark is ported from GraphQL.js, but when this code is in an acceptable shape I'll try porting the rest.

Benchmarks can be run with pytest --benchmark-enable -k benchmark.
By default benchmarks work like simple tests (they only run once), so they shouldn't slow things down too much.

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 this contribution. I always wanted to add the benchmarks - being pushed by others helps a lot. Overall, this looks very good. See below for a few minor suggestions for changes.

[tool:pytest]
# Only run benchmarks as tests.
# To actually run the benchmarks, use --benchmark-enable on the command line.
addopts = --benchmark-disable
Copy link
Member

Choose a reason for hiding this comment

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

We could also use benchmark-skip to completely skip them, but I agree this is better. They are probably a bit redundant, but not too slow if they run only as test.

@ktosiek
Copy link
Contributor Author

ktosiek commented Sep 8, 2019

I've ported the rest of the benchmarks.

Impact on the test suite, as measured with time: (Warning: not current, see comment below)

Command Python 3.6 Python 3.7
pytest 0m11,881s 0m7,113s
pytest --benchmark-skip 0m7,001s 0m4,311s
pytest --benchmark-enable 0m28,314s 0m17,205s

So even running the benchmarks as tests adds 3-4s to the whole test suite. Main offenders are:

1.13s call     tests/benchmarks/test_introspection_from_schema.py::test_executing_introspection_query
0.67s call     tests/benchmarks/test_validate_gql.py::test_validating_introspection_query
0.64s call     tests/benchmarks/test_validate_sdl.py::test_validating_big_schema_sdl

@ktosiek
Copy link
Contributor Author

ktosiek commented Sep 8, 2019

Update: there was an optimisation in GraphQL.js test setup that I've missed when porting, here are the current times:

Command Python 3.6 Python 3.7
pytest 0m10,336s 0m6,125s
pytest --benchmark-skip 0m6,969s 0m4,252s
pytest --benchmark-enable 0m26,654s 0m16,411s

So even running the benchmarks as tests adds 2-3s to the whole test suite. Main offenders are:

0.69s call     tests/benchmarks/test_introspection_from_schema.py::test_executing_introspection_query
0.65s call     tests/benchmarks/test_validate_sdl.py::test_validating_big_schema_sdl

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.

Great job, thanks. See two small issues below. And maybe the test function names should match with the names used in GraphQL.js (specified as const name =....

From src/validation/__tests__/validateGQL-benchmark.js
From src/validation/__tests__/validateSDL-benchmark.js
From src/utilities/__tests__/buildASTSchema-benchmark.js
From src/utilities/__tests__/buildClientSchema-benchmark.js
GraphQL.js does that too, and it shaves ~0.9s off the execution time:
1.13s -> 0.69s for test_introspection_from_schema.py
0.67s -> 0.21s for test_validate_gql.py
@ktosiek
Copy link
Contributor Author

ktosiek commented Sep 8, 2019

I've renamed the test functions.
I see there's a conflict in poetry.lock - should I handle that? Are there any poetry-specific tools for handling merges?

@Cito
Copy link
Member

Cito commented Sep 8, 2019

Thanks. Don't worry, I can resolve that conflict. Will be very busy the next 2 days and look at this later.

@Cito Cito merged commit 32a2e6b into graphql-python:master Sep 10, 2019
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.

2 participants