-
-
Notifications
You must be signed in to change notification settings - Fork 33.7k
gh-137855: Improve import time of sqlite3
#131796
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
@Wulian233 In your benchmark the Also |
|
I'm also a bit surprised that Which, if I'm reading it right, says that importing |
I think I got the name wrong at the time, it was a copy of the previous PR content, and here is the test I just run |
Lib/sqlite3/dbapi2.py
Outdated
| # 3. This notice may not be removed or altered from any source distribution. | ||
|
|
||
| import datetime | ||
| import time |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the point of the PR if the import is not removed? Lazy imports are therefore not used, then why should any speedup be expected, a slowdown is more likely?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Already pointed out in #131796 (comment) :)
(But the PR author self-reviewed the review comment by declaring the review comment to be resolved, which is a controversial feature for github to even allow.)
|
To be precise, it's not importing Also, the benchmarks are quite unstable. The standard deviation is much higher than the mean itself! In practice, I think we don't gain anything else than stability (the stdev becomes smaller with the new implementation but again this could be noise). Can you check if And to be precise, we're not gaining a 2x speed-up. On average, we're only gaining a 1.1x speed-up and the standard deviation of this speed-up is And yes, I'm also surprised that we're gaining "that much" when we just make |
|
@Wulian233, can you address @picnixz's last remark? |
|
I've reworked another version and it's going to be faster now I ran the command
Raw output: Detailsimport time: self [us] | cumulative | imported package import time: 296 | 296 | winreg import time: 248 | 248 | _io import time: 70 | 70 | marshal import time: 312 | 312 | nt import time: 1637 | 2266 | _frozen_importlib_external import time: 1103 | 1103 | time import time: 1089 | 2191 | zipimport import time: 75 | 75 | _codecs import time: 793 | 868 | codecs import time: 1926 | 1926 | encodings.aliases import time: 797 | 797 | encodings._win_cp_codecs import time: 3400 | 6989 | encodings import time: 960 | 960 | encodings.utf_8 import time: 75 | 75 | _codecs_cn import time: 126 | 126 | _multibytecodec import time: 1389 | 1590 | encodings.gbk import time: 86 | 86 | _signal import time: 59 | 59 | _abc import time: 323 | 382 | abc import time: 104 | 104 | _stat import time: 527 | 630 | stat import time: 1745 | 1745 | _collections_abc import time: 126 | 126 | genericpath import time: 229 | 229 | _winapi import time: 3109 | 3463 | ntpath import time: 2543 | 8760 | os import time: 584 | 584 | _sitebuiltins import time: 1004 | 1004 | sitecustomize import time: 563 | 563 | usercustomize import time: 2043 | 12953 | site import time: 1104 | 1104 | linecache import time: 787 | 787 | _datetime import time: 1003 | 1790 | datetime import time: 704 | 704 | itertools import time: 1147 | 1147 | keyword import time: 128 | 128 | _operator import time: 1412 | 1540 | operator import time: 2293 | 2293 | reprlib import time: 164 | 164 | _collections import time: 4104 | 9950 | collections import time: 2917 | 12866 | collections.abc import time: 64 | 64 | _types import time: 1134 | 1197 | types import time: 136 | 136 | _functools import time: 2652 | 3984 | functools import time: 2658 | 6641 | _sqlite3 import time: 74 | 74 | _contextvars import time: 1406 | 1479 | _py_warnings import time: 2508 | 3987 | warnings import time: 6431 | 31714 | sqlite3.dbapi2 import time: 8592 | 40306 | sqlite3D:\Python314>python -X importtime -c "import sqlite3_new" hyperfine test: |
| def main(*args): | ||
| from argparse import ArgumentParser | ||
| from textwrap import dedent | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not see a valid reason to do code churn and delay imports for the sake of speeding up import sqlite3.__main__ while assuming that sqlite3.__main__.main() is never called.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I presume the following: execute is used by the REPL in addition to CLI; user code could import to use SQLiteInteractiveConsole. Whether worth it, not my call.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The REPL is of course part of calling main(), which makes my point quite well. :)
User code certainly could import SQLiteInteractiveConsole, as it lies within their technical capability. I think any user code importing an interactive console from a module named __main__ ought to be prepared to use it without worrying about the startup time of importing argparse and textwrap. A module called __main__ feels rather special to me in that sense -- one ought not casually resort to importing it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @eli-schwartz. This change looks meaningless.
sqlite3sqlite3
|
I agree with @picnixz and @eli-schwartz. The effect of moving |
Improve import time of
sqlite3by 1.5x fasterBenchmark on Windows10, CPython 3.14.0b1
I just realized today that I accidentally deleted an unmerged branch last month, which resulted in closing #129118 . My apologies for the oversight. I've now recreated the pull request - the content remains exactly the same as before
_sqlite3sqlite3.dbapi2sqlite3(main)_sqlite3is not much different.sqlite3.dbapi2decreased significantly from 6431 to 5607sqlite3module dropped noticeably from 8592 to 4213