Fix slowdown on Python 3#290
Merged
Merged
Conversation
Don't parse command line arguments each time `Options()` is called. The metaclass syntax was changed in Python 3 (see [1]) and it is tricky to add a metaclass in a Python version agnostic way. Still possible, but the code would be hard to understand. So I get rid of the Singleton metaclass and use __new__() to implement this logic. Aside of the Option class, we had another singleton: Colorer. However it is instanciated once (this instance is exposed as color_stdout) and it does not matter, whether it is a singleton or not. A brief testing on tarantool's test suite (with --long --force) gives me ~8 minutes after the fix instead of ~11 minutes before the fix. However it worth to note that we have a lot of tests, which are marked as fragile and so run without parallelization. Otherwise the difference would be lower, I guess. [1]: https://www.python.org/dev/peps/pep-3115/ Fixes #279
Totktonada
force-pushed
the
Totktonada/fix-slowdown-on-python-3
branch
from
fbc68c7 to
344d404
Compare
Contributor
Author
Good idea. Done. |
ligurio
approved these changes
Apr 6, 2021
Member
There was a problem hiding this comment.
Thanks for the patch!
Change declares performance speedup, but there are no any numbers attached to check it. So I decided to measure overall time required to run tests for sql test suite (because initially performance degradation has been discovered with tests in that suite) and all tests with and without patch.
time ../../test/test-run.py --builddir=/home/sergeyb/sources/MRG/tarantool/build --vardir=/home/sergeyb/sources/MRG/tarantool/build/test/var --suite sql
| iteration | wo patch | with patch |
|---|---|---|
| 1 | 51,909s | 16,175s |
| 2 | 51,338s | 16,295s |
| 3 | 57,364s | 16,358s |
| 4 | 51,278s | 16,267s |
| 5 | 50,738s | 16,564s |
time ../../test/test-run.py --builddir=/home/sergeyb/sources/MRG/tarantool/build --vardir=/home/sergeyb/sources/MRG/tarantool/build/test/var
| iteration | wo patch | with patch |
|---|---|---|
| 1 | 9m35,228s | 7m23,025s |
| 2 | 9m32,369s | 7m3,614s |
LGTM
avtikhon
approved these changes
Apr 6, 2021
Contributor
avtikhon
left a comment
avtikhon
left a comment
There was a problem hiding this comment.
Timings checked on self-hosted and Github Actions hosts, patch LGTM.
Contributor
Author
|
Thank you all for reviews, testing and measurements! |
Totktonada
added a commit
to tarantool/tarantool
that referenced
this pull request
Apr 7, 2021
List of changes in test-run: * readme: add memcached to users ([PR #285][1]) * test: add integration tests ([PR #273][2]) * test: enable in continuous integration ([PR #273][2]) * Fix slowdown on Python 3 ([PR #290][3]) Updated .luacheckrc to exclude newly added 'core = tarantool' tests (ones that check test-run itself). [1]: tarantool/test-run#285 [2]: tarantool/test-run#273 [3]: tarantool/test-run#290
Totktonada
added a commit
to tarantool/tarantool
that referenced
this pull request
Apr 7, 2021
List of changes in test-run: * readme: add memcached to users ([PR #285][1]) * test: add integration tests ([PR #273][2]) * test: enable in continuous integration ([PR #273][2]) * Fix slowdown on Python 3 ([PR #290][3]) Updated .luacheckrc to exclude newly added 'core = tarantool' tests (ones that check test-run itself). [1]: tarantool/test-run#285 [2]: tarantool/test-run#273 [3]: tarantool/test-run#290 (cherry picked from commit 8b3ec7b)
Totktonada
added a commit
to tarantool/tarantool
that referenced
this pull request
Apr 7, 2021
List of changes in test-run: * readme: add memcached to users ([PR #285][1]) * test: add integration tests ([PR #273][2]) * test: enable in continuous integration ([PR #273][2]) * Fix slowdown on Python 3 ([PR #290][3]) Updated .luacheckrc to exclude newly added 'core = tarantool' tests (ones that check test-run itself). [1]: tarantool/test-run#285 [2]: tarantool/test-run#273 [3]: tarantool/test-run#290 (cherry picked from commit 8b3ec7b)
Totktonada
added a commit
to tarantool/tarantool
that referenced
this pull request
Apr 7, 2021
List of changes in test-run: * readme: add memcached to users ([PR #285][1]) * test: add integration tests ([PR #273][2]) * test: enable in continuous integration ([PR #273][2]) * Fix slowdown on Python 3 ([PR #290][3]) Updated .luacheckrc to exclude newly added 'core = tarantool' tests (ones that check test-run itself). [1]: tarantool/test-run#285 [2]: tarantool/test-run#273 [3]: tarantool/test-run#290 (cherry picked from commit 8b3ec7b) (The .luacheckrc change was excluded from the backport to 1.10: there is no .luacheckrc here.)
Contributor
Author
|
Updated the test-run submodule in tarantool in 2.8.0-199-g8b3ec7b92, 2.7.1-182-gafc84a5d2, 2.6.2-178-geedd55789, 1.10.9-110-gef7938933. |
1 task
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Don't parse command line arguments each time
Options()is called.The metaclass syntax was changed in Python 3 (see 1) and it is tricky
to add a metaclass in a Python version agnostic way. Still possible, but
the code would be hard to understand. So I get rid of the Singleton
metaclass and use __new__() to implement this logic.
Aside of the Option class, we had another singleton: Colorer. However it
is instanciated once (this instance is exposed as color_stdout) and it
does not matter, whether it is a singleton or not.
A brief testing on tarantool's test suite (with --long --force) gives me
~8 minutes after the fix instead of ~11 minutes before the fix. However
it worth to note that we have a lot of tests, which are marked as
fragile and so run without parallelization. Otherwise the difference
would be lower, I guess.
Fixes #279