GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||||||||
GeezerVison is showing github.com — gh-154675: Improve import performance of ``tkinter`` library with lazy imports by sharktide · Pull Request #148409 · python/cpython Conversation
|
|
cc @AlexWaygood |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
@sharktide please also add a test verifying that the lazy modules are indeed not imported. See #144387 for how to do it. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
EDIT: I can’t generate a direct comparison graph currently, but expect a 20-30% performance increase.
tkinter is not a common module while enum is and that's the rationale why we wanted to improve the import time of enum. I would also not say "expect a XX" increase without convincing arguments (it could be more, it could be less, but I don't think it's relevant to shoot a random number like that).
The change to import re adds more functions that are less readable IMO. I don't think it's worth it. For traceback it may be ok but os is a cheap module so I don't think it's worth either.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
The added complexity is minimal. It's just a very basic wrapper around a variable. I was able to make it in ~5 minutes on mobile. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
It's not. It's about code readability. It's not because you did in 5 minutes that it's not complex. The problem is that we have more indirect references. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Here's a smaller diff that does it the way tokenize.py used to. It adds a fast Detailsdiff --git a/Lib/tkinter/__init__.py b/Lib/tkinter/__init__.py index 63340aafa77..f2313a5cfa9 100644 --- a/Lib/tkinter/__init__.py +++ b/Lib/tkinter/__init__.py @@ -33,6 +33,7 @@ from tkinter.constants import * import collections import enum +import functools import sys import types @@ -51,28 +52,16 @@ WRITABLE = _tkinter.WRITABLE EXCEPTION = _tkinter.EXCEPTION -_magic_re = None -_space_re = None +_MAGIC_RE_PAT = r'([\\{}])' +_SPACE_RE_PAT = r'(?a)([\s])' + + +@functools.lru_cache +def _compile(expr): + return re.compile(expr) + -def _get_magic_re(): - """ - Internal function that acts as a wrapper - for the re import to make it lazy. - """ - global _magic_re - if _magic_re is None: - _magic_re = re.compile(r'([\\{}])') - return _magic_re -def _get_space_re(): - """ - Internal function that acts as a wrapper - for the re import to make it lazy. - """ - global _space_re - if _space_re is None: - _space_re = re.compile(r'([\s])', re.ASCII) - return _space_re def _join(value): """Internal function.""" @@ -83,7 +72,7 @@ def _stringify(value): if isinstance(value, (list, tuple)): if len(value) == 1: value = _stringify(value[0]) - if _get_magic_re().search(value): + if _compile(_MAGIC_RE_PAT).search(value): value = '{%s}' % value else: value = '{%s}' % _join(value) @@ -94,14 +83,14 @@ def _stringify(value): value = str(value) if not value: value = '{}' - elif _get_magic_re().search(value): + elif _compile(_MAGIC_RE_PAT).search(value): # add '\' before special characters and spaces - value = _get_magic_re().sub(r'\\\1', value) + value = _compile(_MAGIC_RE_PAT).sub(r'\\\1', value) value = value.replace('\n', r'\n') - value = _get_space_re().sub(r'\\\1', value) + value = _compile(_SPACE_RE_PAT).sub(r'\\\1', value) if value[0] == '"': value = '\\' + value - elif value[0] == '"' or _get_space_re().search(value): + elif value[0] == '"' or _compile(_SPACE_RE_PAT).search(value): value = '{%s}' % value return value |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
tkinter library with lazy importstkinter library with lazy imports
There was an error while loading. Please reload this page.
|
I just ran some tests in a headless windows environment, and the 3.15 main branch was 23ms, this pr was 18ms, @hugovk’s idea was also 18ms. IMO it’s worth making these imports lazy, up to y’all which impl you like better. For readability, I prefer mine because it’s almost the same as the original code, save for the two wrapper functions, but they preserve the same names as before so it is easier to understand for me at least, but @hugovk’s idea is also just as good. P.S. The system I tested on has low-end specs and it’s headless (I use it for cybersecurity testing), but nevertheless those times are what I got. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
I am honestly against the change for Importing |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
This PR is stale because it has been open for 30 days with no activity. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
tkinter library with lazy importstkinter library with lazy imports
tkinter library with lazy importstkinter library with lazy imports
There was an error while loading. Please reload this page.
|
Sorry, incorrectly flagged as a duplicate. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Documentation build overview
337 files changed ·
|
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.