FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

gh-109653: Speedup import of threading module by danielhollas · Pull Request #114509 · python/cpython · GitHub

/ cpython Public

gh-109653: Speedup import of threading module - #114509

Merged
AlexWaygood merged 3 commits into
python:mainfrom
danielhollas:import-threading-speedup
Jan 31, 2024
Merged

gh-109653: Speedup import of threading module#114509
AlexWaygood merged 3 commits into
python:mainfrom
danielhollas:import-threading-speedup

Conversation

danielhollas commented Jan 23, 2024
edited
Loading

Copy link
Copy Markdown
Contributor

Delayed import of functools speeds up the import threading by ~50% (2ms -> 1ms) in my testing.

Since the functools module is only used in the internal _register_atexit function that is called by concurrent.futures, this seems like a worthwhile win for users of threading module who do not use asyncio.

Part of #109653

CC @AlexWaygood

Delayed import of functools leads to 50% speedup
of import time.

bedevere-app Bot commented Jan 23, 2024

Copy link
Copy Markdown

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

danielhollas commented Jan 24, 2024
edited by hugovk
Loading

Copy link
Copy Markdown
Contributor Author

To be precise, compiling python with ./configure --enable-optimizations and measuring with python -Ximporttime -c "import threading", I am getting 2.44ms on main and 1.17ms on this PR.

Eclips4 added the performance Performance or resource usage label Jan 24, 2024

ajoino commented Jan 24, 2024

Copy link
Copy Markdown

Just a thought, is it even necessary to use functools.partial here? Could this not be replaced with lambda: f(*args, **kwargs), that would avoid importing functools at all. Is there something I'm missing here?

Copy link
Copy Markdown
Member

Just a thought, is it even necessary to use functools.partial here? Could this not be replaced with lambda: f(*args, **kwargs), that would avoid importing functools at all. Is there something I'm missing here?

This was my thought as well on first seeing the patch. functools.partial can be faster than a lambda function, but here I doubt it makes a significant difference. (If we wanted to check whether using a lambda here slowed things down, we'd need to do a benchmark using concurrent.futures, since the concurrent.futures module is the only public API that makes use of this private API. It might be possible to write such a benchmark, but it also might be difficult -- not sure.)

Copy link
Copy Markdown
Contributor Author

functools.partial can be faster than a lambda function, but here I doubt it makes a significant difference. (If we wanted to check whether using a lambda here slowed things down, we'd need to do a benchmark using concurrent.futures, since the concurrent.futures module is the only public API that makes use of this private API. It might be possible to write such a benchmark, but it also might be difficult -- not sure.)

Looking at the code, the threading.register_atexit() is only ever called during concurrent.futures import, so I would assume any performance difference here would be marginal?

Copy link
Copy Markdown
Member

Looking at the code, the threading.register_atexit() is only ever called during concurrent.futures import, so I would assume any performance difference here would be marginal?

Oh, great point 😄

In that case, let's just go with a lambda here -- it seems simpler :)

Comment thread Lib/threading.py Outdated
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>

AlexWaygood left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM, thanks! I'd love to check with a core dev more familiar with subinterpreters before merging, though (since this feature was specifically added to help with subinterpreter support).

@ericsnowcurrently, there's no reason why switching to a lambda rather than functools.partial could be problematic for subinterpreter support, is there?

Copy link
Copy Markdown
Contributor Author

@AlexWaygood thanks!

@ericsnowcurrently, there's no reason why switching to a lambda rather than functools.partial could be problematic for subinterpreter support, is there?

Just a note, if this was a problem, we could still get away with it by simply not doing either: the function is (at least currently) being called without any extra *args or **args arguments so we could make _register_atexit less general and simply pass the callback function directly to _threading_atexits list.

danielhollas changed the title gh-109653: Speedup import of threading module gh-109653: Speedup import of threading module Jan 24, 2024

Copy link
Copy Markdown
Member

I can't see a way in which this would cause problems — I'll go ahead and merge, since it's been a few days :)

Thanks @danielhollas!

AlexWaygood merged commit 5e390a0 into python:main Jan 31, 2024
danielhollas deleted the import-threading-speedup branch January 31, 2024 10:59

Copy link
Copy Markdown
Member

@ericsnowcurrently, there's no reason why switching to a lambda rather than functools.partial could be problematic for subinterpreter support, is there?

I'm not aware of any such reason.

Comment thread Lib/threading.py
aisk pushed a commit to aisk/cpython that referenced this pull request Feb 11, 2024
Avoiding an import of functools leads to 50% speedup of import time.

Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
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

Labels

performance Performance or resource usage

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants


Back | FazBrowse Home | New Git URL