gh-156777: Optimize import overhead in the logging package - #156778
brittanyrey wants to merge 9 commits into
Conversation
Lift the function-level imports in logging/handlers.py and logging/config.py
to module scope as lazy imports, and mark other cold-path imports lazy.
Imports reachable from interpreter finalization stay eager and are now
commented as such: at teardown sys.modules has been cleared, so resolving a
lazy import raises ImportError('sys.meta_path is None').
The Windows-only (_winapi, winreg) and optional third-party
(win32evtlogutil) imports stay function-level on purpose: as module-level
lazy imports they would break inspect.getmembers() and pydoc on non-Windows.
import logging.config -28.3%
import logging.handlers -17.8%
import logging unchanged
Call-time benchmarks unchanged (all within noise).
Co-authored-by: Pieter Eendebak <pieter.eendebak@gmail.com>
Sort each import block into the isort/Ruff groups requested in review: plain imports, from-imports, lazy imports, lazy from-imports. In logging/config.py, replace ``lazy import logging.handlers`` with ``lazy from logging import handlers``. The former binds the name ``logging`` lazily, so it shadows the eager ``import logging`` above it and any use of ``logging.<anything>`` in the module pulls in logging.handlers. Aliased to ``logging_handlers`` because ``handlers`` is already a local variable in _install_handlers() and _configure_queue_handler(). Now logging.handlers is imported only when a handler is configured through the ``class`` key; previously dictConfig(), fileConfig() and stopListening() all reified it. Co-authored-by: Pieter Eendebak <pieter.eendebak@gmail.com> Co-authored-by: Hugo van Kemenade <hugovk@users.noreply.github.com>
| lazy import queue | ||
| lazy import smtplib | ||
| lazy import socket | ||
| lazy import ssl |
There was a problem hiding this comment.
Should ssl stay inside the TLS branch too? Moving it here breaks inspect.getmembers(logging.handlers) on builds without _ssl, for the same reason as the optional Windows imports.
|
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 |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
|
@sprajs, thanks for wanting to help. This review largely repeats findings already reported, which adds to the maintainers’ reading workload without helping move the PR forward. Please read CPython’s guidelines for using AI tools, particularly the expectations around judgment and productive contributions. Please stop posting AI-assisted reviews that restate existing feedback. We’d appreciate keeping the discussion focused on new, actionable information. |
Keep ssl and multiprocessing.queues as function-level imports. A module-level lazy import is resolved by inspect.getmembers() and pydoc, so ssl breaks them on a build without _ssl, and multiprocessing defeats the deferral the comment there asks for. Neither costs anything: a nested import and a module-level lazy import defer identically. Leave pickle, socket and struct eager in handlers.py. A record emitted from a __del__ during finalization can no longer import, so a SocketHandler pickled its first record too late and dropped it. Resolve logging.handlers in fileConfig() before it evaluates any config expression against vars(logging), so the documented handlers.X spelling works again in a fresh process, both in class= and in defaults=. Add tests for each, and pin the laziness that is left.
pickle, socket and struct go back to lazy. emit() can run during finalization, when importing no longer works, so SocketHandler.__init__ resolves them while it still can. Keeping them eager instead gives back three quarters of the handlers.py import-time win.
Reverts the lazy alias and the nine logging_handlers.* renames that came with it. The alias left "handlers" out of vars(logging), which broke the documented handlers.X spelling in fileConfig() config files, and it was worth only 0.68 ms of the 6.1 ms win on importing logging.config. Also drop the two comments that no longer carry their weight, so the io and ssl hunks disappear from the diff, and trim the new tests.
Drops the "as logging_handlers" alias and the nine renames that came with it. The dotted spelling defers the submodule just as well for import time, and resolving the module's own logging global is what binds handlers in vars(logging) again, so the documented handlers.X spelling in fileConfig() config files works in a fresh process.
|
Addressed comments, added new tests to repro the corner cases @pablogsal pointed out, and cleaned up come of the churn that had slipped in to the changes. |
|
(I merged in |
| from socketserver import ThreadingTCPServer, StreamRequestHandler | ||
| lazy import configparser | ||
| lazy import json | ||
| lazy import logging.handlers |
There was a problem hiding this comment.
Nit: this also rebinds logging in this module to a lazy object, and that is what makes class=handlers.X work in fileConfig() (the logging._lock access reifies it before the eval). Could we add a small comment here explaining this? It is very easy to break by turning it back into lazy from logging import handlers.
| import threading | ||
| import time | ||
| lazy import base64 | ||
| lazy import copy |
There was a problem hiding this comment.
Same problem as with SocketHandler: QueueHandler.prepare() calls copy.copy(), so a QueueHandler emitting its first record from a __del__ at shutdown now cannot import copy and drops the record. copy was eager before, so I think we should keep it eager (or resolve it in QueueHandler.__init__ like you did for SocketHandler). Can we add a shutdown test for this one too?
|
Thanks! I checked the three fixes and the new tests. They look good to me. |
6 participants
summary
perf
import logging.configimport logging.handlersimport logging