iframe-proxy

effigies · 2024-06-07T14:12:52Z

As noted in gh-117983, the import importlib.util can be triggered at interpreter startup under some circumstances, so adding threading makes it a potentially obligatory load.
Lazy loading is not used in the stdlib, so this removes an unnecessary load for the majority of users and slightly increases the cost of the first lazily loaded module.

An obligatory threading load breaks gevent, which monkeypatches the stdlib. Although unsupported, there doesn't seem to be an offsetting benefit to breaking their use case.

For reference, here are benchmarks for the current main branch:

❯ hyperfine -w 8 './python -c "import importlib.util"'
Benchmark 1: ./python -c "import importlib.util"
  Time (mean ± σ):       9.7 ms ±   0.7 ms    [User: 7.7 ms, System: 1.8 ms]
  Range (min … max):     8.4 ms …  13.1 ms    313 runs

And with this patch:

❯ hyperfine -w 8 './python -c "import importlib.util"'
Benchmark 1: ./python -c "import importlib.util"
  Time (mean ± σ):       8.4 ms ±   0.7 ms    [User: 6.8 ms, System: 1.4 ms]
  Range (min … max):     7.2 ms …  11.7 ms    352 runs

Compare to:

❯ hyperfine -w 8 './python -c pass'
Benchmark 1: ./python -c pass
  Time (mean ± σ):       7.6 ms ±   0.6 ms    [User: 5.9 ms, System: 1.6 ms]
  Range (min … max):     6.7 ms …  11.3 ms    390 runs

This roughly halves the import time of importlib.util.

As noted in pythongh-117983, the import importlib.util can be triggered at interpreter startup under some circumstances, so adding threading makes it a potentially obligatory load. Lazy loading is not used in the stdlib, so this removes an unnecessary load for the majority of users and slightly increases the cost of the first lazily loaded module. An obligatory threading load breaks gevent, which monkeypatches the stdlib. Although unsupported, there doesn't seem to be an offsetting benefit to breaking their use case. For reference, here are benchmarks for the current main branch: ``` ❯ hyperfine -w 8 './python -c "import importlib.util"' Benchmark 1: ./python -c "import importlib.util" Time (mean ± σ): 9.7 ms ± 0.7 ms [User: 7.7 ms, System: 1.8 ms] Range (min … max): 8.4 ms … 13.1 ms 313 runs ``` And with this patch: ``` ❯ hyperfine -w 8 './python -c "import importlib.util"' Benchmark 1: ./python -c "import importlib.util" Time (mean ± σ): 8.4 ms ± 0.7 ms [User: 6.8 ms, System: 1.4 ms] Range (min … max): 7.2 ms … 11.7 ms 352 runs ``` Compare to: ``` ❯ hyperfine -w 8 './python -c pass' Benchmark 1: ./python -c pass Time (mean ± σ): 7.6 ms ± 0.6 ms [User: 5.9 ms, System: 1.6 ms] Range (min … max): 6.7 ms … 11.3 ms 390 runs ``` This roughly halves the import time of importlib.util.

hauntsaninja

This seems reasonable to me (although this shouldn't be taken as any sort of promise of support for gevent's crimes :-) )

While it's unfortunate that the lazy loader bug fix caused a regression in a late patch release, note that I can't backport to 3.11 because it's in the security-only point of its lifecycle / changes need to be coordinated with the 3.11 release manager.

effigies · 2024-07-03T19:19:39Z

pablogsal · 2024-07-03T20:34:39Z

I can't backport to 3.11 because it's in the security-only point of its lifecycle / changes need to be coordinated with the 3.11 release manager.

@pablogsal Ping about whether this would be acceptable to backport to 3.11, if merged into main. I'm okay either way.

Is not a security fix so sadly it won't be possible per our policy as this doesn't qualify as something that we could do an exception. Sorry :(

miss-islington-app · 2024-07-03T20:50:50Z

Thanks @effigies for the PR, and @brettcannon for merging it 🌮🎉.. I'm working now to backport this PR to: 3.12, 3.13.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!

…ythonGH-120233) As noted in pythongh-117983, the import importlib.util can be triggered at interpreter startup under some circumstances, so adding threading makes it a potentially obligatory load. Lazy loading is not used in the stdlib, so this removes an unnecessary load for the majority of users and slightly increases the cost of the first lazily loaded module. An obligatory threading load breaks gevent, which monkeypatches the stdlib. Although unsupported, there doesn't seem to be an offsetting benefit to breaking their use case. For reference, here are benchmarks for the current main branch: ``` ❯ hyperfine -w 8 './python -c "import importlib.util"' Benchmark 1: ./python -c "import importlib.util" Time (mean ± σ): 9.7 ms ± 0.7 ms [User: 7.7 ms, System: 1.8 ms] Range (min … max): 8.4 ms … 13.1 ms 313 runs ``` And with this patch: ``` ❯ hyperfine -w 8 './python -c "import importlib.util"' Benchmark 1: ./python -c "import importlib.util" Time (mean ± σ): 8.4 ms ± 0.7 ms [User: 6.8 ms, System: 1.4 ms] Range (min … max): 7.2 ms … 11.7 ms 352 runs ``` Compare to: ``` ❯ hyperfine -w 8 './python -c pass' Benchmark 1: ./python -c pass Time (mean ± σ): 7.6 ms ± 0.6 ms [User: 5.9 ms, System: 1.6 ms] Range (min … max): 6.7 ms … 11.3 ms 390 runs ``` This roughly halves the import time of importlib.util. (cherry picked from commit 94f50f8) Co-authored-by: Chris Markiewicz <effigies@gmail.com>

bedevere-app · 2024-07-03T20:51:24Z

GH-121349 is a backport of this pull request to the 3.13 branch.

…ythonGH-120233) As noted in pythongh-117983, the import importlib.util can be triggered at interpreter startup under some circumstances, so adding threading makes it a potentially obligatory load. Lazy loading is not used in the stdlib, so this removes an unnecessary load for the majority of users and slightly increases the cost of the first lazily loaded module. An obligatory threading load breaks gevent, which monkeypatches the stdlib. Although unsupported, there doesn't seem to be an offsetting benefit to breaking their use case. For reference, here are benchmarks for the current main branch: ``` ❯ hyperfine -w 8 './python -c "import importlib.util"' Benchmark 1: ./python -c "import importlib.util" Time (mean ± σ): 9.7 ms ± 0.7 ms [User: 7.7 ms, System: 1.8 ms] Range (min … max): 8.4 ms … 13.1 ms 313 runs ``` And with this patch: ``` ❯ hyperfine -w 8 './python -c "import importlib.util"' Benchmark 1: ./python -c "import importlib.util" Time (mean ± σ): 8.4 ms ± 0.7 ms [User: 6.8 ms, System: 1.4 ms] Range (min … max): 7.2 ms … 11.7 ms 352 runs ``` Compare to: ``` ❯ hyperfine -w 8 './python -c pass' Benchmark 1: ./python -c pass Time (mean ± σ): 7.6 ms ± 0.6 ms [User: 5.9 ms, System: 1.6 ms] Range (min … max): 6.7 ms … 11.3 ms 390 runs ``` This roughly halves the import time of importlib.util. (cherry picked from commit 94f50f8) Co-authored-by: Chris Markiewicz <effigies@gmail.com>

bedevere-app · 2024-07-03T20:51:30Z

GH-121350 is a backport of this pull request to the 3.12 branch.

…H-120233) (GH-121350) gh-117983: Defer import of threading for lazy module loading (GH-120233) As noted in gh-117983, the import importlib.util can be triggered at interpreter startup under some circumstances, so adding threading makes it a potentially obligatory load. Lazy loading is not used in the stdlib, so this removes an unnecessary load for the majority of users and slightly increases the cost of the first lazily loaded module. An obligatory threading load breaks gevent, which monkeypatches the stdlib. Although unsupported, there doesn't seem to be an offsetting benefit to breaking their use case. For reference, here are benchmarks for the current main branch: ``` ❯ hyperfine -w 8 './python -c "import importlib.util"' Benchmark 1: ./python -c "import importlib.util" Time (mean ± σ): 9.7 ms ± 0.7 ms [User: 7.7 ms, System: 1.8 ms] Range (min … max): 8.4 ms … 13.1 ms 313 runs ``` And with this patch: ``` ❯ hyperfine -w 8 './python -c "import importlib.util"' Benchmark 1: ./python -c "import importlib.util" Time (mean ± σ): 8.4 ms ± 0.7 ms [User: 6.8 ms, System: 1.4 ms] Range (min … max): 7.2 ms … 11.7 ms 352 runs ``` Compare to: ``` ❯ hyperfine -w 8 './python -c pass' Benchmark 1: ./python -c pass Time (mean ± σ): 7.6 ms ± 0.6 ms [User: 5.9 ms, System: 1.6 ms] Range (min … max): 6.7 ms … 11.3 ms 390 runs ``` This roughly halves the import time of importlib.util. (cherry picked from commit 94f50f8) Co-authored-by: Chris Markiewicz <effigies@gmail.com>

…H-120233) (GH-121349) gh-117983: Defer import of threading for lazy module loading (GH-120233) As noted in gh-117983, the import importlib.util can be triggered at interpreter startup under some circumstances, so adding threading makes it a potentially obligatory load. Lazy loading is not used in the stdlib, so this removes an unnecessary load for the majority of users and slightly increases the cost of the first lazily loaded module. An obligatory threading load breaks gevent, which monkeypatches the stdlib. Although unsupported, there doesn't seem to be an offsetting benefit to breaking their use case. For reference, here are benchmarks for the current main branch: ``` ❯ hyperfine -w 8 './python -c "import importlib.util"' Benchmark 1: ./python -c "import importlib.util" Time (mean ± σ): 9.7 ms ± 0.7 ms [User: 7.7 ms, System: 1.8 ms] Range (min … max): 8.4 ms … 13.1 ms 313 runs ``` And with this patch: ``` ❯ hyperfine -w 8 './python -c "import importlib.util"' Benchmark 1: ./python -c "import importlib.util" Time (mean ± σ): 8.4 ms ± 0.7 ms [User: 6.8 ms, System: 1.4 ms] Range (min … max): 7.2 ms … 11.7 ms 352 runs ``` Compare to: ``` ❯ hyperfine -w 8 './python -c pass' Benchmark 1: ./python -c pass Time (mean ± σ): 7.6 ms ± 0.6 ms [User: 5.9 ms, System: 1.6 ms] Range (min … max): 6.7 ms … 11.3 ms 390 runs ``` This roughly halves the import time of importlib.util. (cherry picked from commit 94f50f8) Co-authored-by: Chris Markiewicz <effigies@gmail.com>

bedevere-bot · 2024-07-03T21:23:35Z

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot x86-64 MacOS Intel NoGIL 3.x has failed when building commit 94f50f8.

What do you need to do:

Don't panic.
Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
Go to the page of the buildbot that failed (https://buildbot.python.org/all/#builders/1258/builds/2289) and take a look at the build logs.
Check if the failure is related to this commit (94f50f8) or if it is a false positive.
If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/all/#builders/1258/builds/2289

Failed tests:

test_pyrepl
test_eintr

Failed subtests:

test_python_basic_repl - test.test_pyrepl.test_pyrepl.TestMain.test_python_basic_repl
test_all - test.test_eintr.EINTRTests.test_all
test_lockf - main.FNTLEINTRTest.test_lockf
test_flock - main.FNTLEINTRTest.test_flock

Summary of the results of the build (if available):

==

Click to see traceback logs

Traceback (most recent call last):
  File "/Users/ec2-user/buildbot/buildarea/3.x.itamaro-macos-intel-aws.nogil/build/Lib/test/support/__init__.py", line 2622, in wrapper
    return func(*args, **kwargs)
  File "/Users/ec2-user/buildbot/buildarea/3.x.itamaro-macos-intel-aws.nogil/build/Lib/test/test_pyrepl/test_pyrepl.py", line 890, in test_python_basic_repl
    self.assertEqual(exit_code, 0)
    ~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^
AssertionError: 1 != 0


Traceback (most recent call last):
  File "/Users/ec2-user/buildbot/buildarea/3.x.itamaro-macos-intel-aws.nogil/build/Lib/test/_test_eintr.py", line 532, in test_lockf
    self._lock(fcntl.lockf, "lockf")
    ~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/ec2-user/buildbot/buildarea/3.x.itamaro-macos-intel-aws.nogil/build/Lib/test/_test_eintr.py", line 517, in _lock
    raise Exception("failed to sync child in %.1f sec" % dt)
Exception: failed to sync child in 300.6 sec


Traceback (most recent call last):
  File "/Users/ec2-user/buildbot/buildarea/3.x.itamaro-macos-intel-aws.nogil/build/Lib/test/_test_eintr.py", line 535, in test_flock
    self._lock(fcntl.flock, "flock")
    ~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/ec2-user/buildbot/buildarea/3.x.itamaro-macos-intel-aws.nogil/build/Lib/test/_test_eintr.py", line 517, in _lock
    raise Exception("failed to sync child in %.1f sec" % dt)
Exception: failed to sync child in 300.3 sec


Traceback (most recent call last):
  File "/Users/ec2-user/buildbot/buildarea/3.x.itamaro-macos-intel-aws.nogil/build/Lib/test/test_eintr.py", line 17, in test_all
    script_helper.run_test_script(script)
    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^
  File "/Users/ec2-user/buildbot/buildarea/3.x.itamaro-macos-intel-aws.nogil/build/Lib/test/support/script_helper.py", line 318, in run_test_script
    raise AssertionError(f"{name} failed")
AssertionError: script _test_eintr.py failed

…ython#120233) As noted in pythongh-117983, the import importlib.util can be triggered at interpreter startup under some circumstances, so adding threading makes it a potentially obligatory load. Lazy loading is not used in the stdlib, so this removes an unnecessary load for the majority of users and slightly increases the cost of the first lazily loaded module. An obligatory threading load breaks gevent, which monkeypatches the stdlib. Although unsupported, there doesn't seem to be an offsetting benefit to breaking their use case. For reference, here are benchmarks for the current main branch: ``` ❯ hyperfine -w 8 './python -c "import importlib.util"' Benchmark 1: ./python -c "import importlib.util" Time (mean ± σ): 9.7 ms ± 0.7 ms [User: 7.7 ms, System: 1.8 ms] Range (min … max): 8.4 ms … 13.1 ms 313 runs ``` And with this patch: ``` ❯ hyperfine -w 8 './python -c "import importlib.util"' Benchmark 1: ./python -c "import importlib.util" Time (mean ± σ): 8.4 ms ± 0.7 ms [User: 6.8 ms, System: 1.4 ms] Range (min … max): 7.2 ms … 11.7 ms 352 runs ``` Compare to: ``` ❯ hyperfine -w 8 './python -c pass' Benchmark 1: ./python -c pass Time (mean ± σ): 7.6 ms ± 0.6 ms [User: 5.9 ms, System: 1.6 ms] Range (min … max): 6.7 ms … 11.3 ms 390 runs ``` This roughly halves the import time of importlib.util.

effigies requested review from brettcannon, ericsnowcurrently, ncoghlan and warsaw as code owners June 7, 2024 14:12

bedevere-app Bot added the awaiting review label Jun 7, 2024

bedevere-app Bot mentioned this pull request Jun 7, 2024

GH-114781 potentially breaks gevent: threading becomes pre-imported at startup #117983

Closed

ffernand mentioned this pull request Jun 21, 2024

Unable to start application with Python 3.11.9 + gevent + ddtrace DataDog/dd-trace-py#8903

Closed

Merge branch 'main' into fix-issue-117983

f82a31f

hauntsaninja added needs backport to 3.12 only security fixes needs backport to 3.13 bugs and security fixes labels Jul 2, 2024

hauntsaninja approved these changes Jul 2, 2024

View reviewed changes

bedevere-app Bot added awaiting merge and removed awaiting review labels Jul 2, 2024

Merge branch 'main' into fix-issue-117983

02acef8

brettcannon enabled auto-merge (squash) July 3, 2024 20:27

brettcannon approved these changes Jul 3, 2024

View reviewed changes

brettcannon merged commit 94f50f8 into python:main Jul 3, 2024

bedevere-app Bot removed the awaiting merge label Jul 3, 2024

bedevere-app Bot removed the needs backport to 3.13 bugs and security fixes label Jul 3, 2024

bedevere-app Bot removed the needs backport to 3.12 only security fixes label Jul 3, 2024

effigies deleted the fix-issue-117983 branch July 3, 2024 20:52

Sunbelt Computer Software

PL/B Language Development and Support

Navigation Menu

Uh oh!

Uh oh!

gh-117983: Defer import of threading for lazy module loading - #120233

gh-117983: Defer import of threading for lazy module loading#120233
brettcannon merged 3 commits into
python:mainfrom
effigies:fix-issue-117983

effigies commented Jun 7, 2024

Uh oh!

hauntsaninja left a comment

Uh oh!

effigies commented Jul 3, 2024

Uh oh!

pablogsal commented Jul 3, 2024

Uh oh!

miss-islington-app Bot commented Jul 3, 2024

Uh oh!

bedevere-app Bot commented Jul 3, 2024

Uh oh!

bedevere-app Bot commented Jul 3, 2024

Uh oh!

bedevere-bot commented Jul 3, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Sunbelt Computer Software

PL/B Language Development and Support

Uh oh!

Uh oh!

Conversation

effigies commented Jun 7, 2024

Uh oh!

hauntsaninja left a comment

Choose a reason for hiding this comment

Uh oh!

effigies commented Jul 3, 2024

Uh oh!

pablogsal commented Jul 3, 2024

Uh oh!

miss-islington-app Bot commented Jul 3, 2024

Uh oh!

bedevere-app Bot commented Jul 3, 2024

Uh oh!

bedevere-app Bot commented Jul 3, 2024

Uh oh!

bedevere-bot commented Jul 3, 2024

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants