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

bpo-29585: optimize site.py startup time #136

Merged
merged 9 commits into from Jun 28, 2017

Conversation

Projects
None yet
5 participants
@methane
Member

methane commented Feb 16, 2017

Skip importing sysconfig when possible.

Median +- std dev: [default] 15.8 ms +- 0.0 ms -> [patched] 14.7 ms +- 0.0 ms: 1.07x faster (-7%)

(bpo-29585)

@methane

This comment has been minimized.

Show comment
Hide comment
@methane

methane Feb 16, 2017

Member
$ cp  build/lib.linux-x86_64-3.7/_sysconfigdata_m_linux_x86_64-linux-gnu.py sysconfigdata
$ ./python -c 'import sysconfigdata'  # create pyc file
$ ./python -m timeit -s 'import sysconfigdata, importlib' -- 'importlib.reload(sysconfigdata)'
1000 loops, best of 5: 269 usec per loop

Since 'PYTHONFRAMEWORK' is in sysconfigdata, I cannot stop importing them.
But other cases, I can skip importing sysconfig and sysconfigdata completely.

Member

methane commented Feb 16, 2017

$ cp  build/lib.linux-x86_64-3.7/_sysconfigdata_m_linux_x86_64-linux-gnu.py sysconfigdata
$ ./python -c 'import sysconfigdata'  # create pyc file
$ ./python -m timeit -s 'import sysconfigdata, importlib' -- 'importlib.reload(sysconfigdata)'
1000 loops, best of 5: 269 usec per loop

Since 'PYTHONFRAMEWORK' is in sysconfigdata, I cannot stop importing them.
But other cases, I can skip importing sysconfig and sysconfigdata completely.

@ned-deily

This is a major change: by duplicating code from sysconfig.py into site.py, there would now be a implicit link between the two and a potential maintenance problem. If we were to do this, there should at least be mention of these duplications in sysconfig.py and/or tests for the duplicated behavior. Also, there should be a b.p.o issue for this proposed change.

@methane methane changed the title from optimize site.py to [RFC] bpo-29585: optimize site.py startup time Feb 17, 2017

@vstinner

I dislike this approach. I suggest to experiment a _site extension module: see the issue.

@methane

This comment has been minimized.

Show comment
Hide comment
@methane

methane Feb 18, 2017

Member

I've addes sys._framework and remove sysconf dependency completely.

Member

methane commented Feb 18, 2017

I've addes sys._framework and remove sysconf dependency completely.

@methane methane changed the title from [RFC] bpo-29585: optimize site.py startup time to [RFC] [bpo-29585](https://bugs.python.org/issue29585): optimize site.py startup time Feb 18, 2017

@methane methane changed the title from [RFC] [bpo-29585](https://bugs.python.org/issue29585): optimize site.py startup time to [RFC] bpo-29585: optimize site.py startup time Feb 18, 2017

@methane methane changed the title from [RFC] bpo-29585: optimize site.py startup time to bpo-29585: optimize site.py startup time Feb 19, 2017

methane added some commits Feb 18, 2017

optimize site.py
Skip importing sysconfig when possible.

Median +- std dev: [default] 15.8 ms +- 0.0 ms -> [patched] 14.7 ms +- 0.0 ms: 1.07x faster (-7%)
@methane

This comment has been minimized.

Show comment
Hide comment
@methane

methane Jun 28, 2017

Member

@ned-deily I created bpo issue and added comment about duplicated code.

Member

methane commented Jun 28, 2017

@ned-deily I created bpo issue and added comment about duplicated code.

@methane

This comment has been minimized.

Show comment
Hide comment
@methane

methane Jun 28, 2017

Member

@haypo sysconfig (and _sysconfigdata_...) module is relatively large and
most applications (except packaging tools) doesn't use it at all.
Now this pull request is focusing it.

Slow code path is skipped by #167 although abspath is still slow.

Member

methane commented Jun 28, 2017

@haypo sysconfig (and _sysconfigdata_...) module is relatively large and
most applications (except packaging tools) doesn't use it at all.
Now this pull request is focusing it.

Slow code path is skipped by #167 although abspath is still slow.

@vstinner

This comment has been minimized.

Show comment
Hide comment
@vstinner

vstinner Jun 28, 2017

Member

A quick & dirty benchmark using my perf module: I see an improvement of -0.8 ms (1.05x faster) on Python startup time.

haypo@selma$ ./python -m perf command --inherit=PYTHONPATH -v -o pr136.json -- ./python -c pass  
...
haypo@selma$ ./python -m perf command --inherit=PYTHONPATH -v -o ref.json -- ./python -c pass
...
haypo@selma$ ./python -m perf compare_to ref.json pr136.json 
Mean +- std dev: [ref] 17.4 ms +- 0.8 ms -> [pr136] 16.6 ms +- 1.1 ms: 1.05x faster (-5%)

EDIT: this benchmark was run on my Linux laptop.

Member

vstinner commented Jun 28, 2017

A quick & dirty benchmark using my perf module: I see an improvement of -0.8 ms (1.05x faster) on Python startup time.

haypo@selma$ ./python -m perf command --inherit=PYTHONPATH -v -o pr136.json -- ./python -c pass  
...
haypo@selma$ ./python -m perf command --inherit=PYTHONPATH -v -o ref.json -- ./python -c pass
...
haypo@selma$ ./python -m perf compare_to ref.json pr136.json 
Mean +- std dev: [ref] 17.4 ms +- 0.8 ms -> [pr136] 16.6 ms +- 1.1 ms: 1.05x faster (-5%)

EDIT: this benchmark was run on my Linux laptop.

@vstinner

This comment has been minimized.

Show comment
Hide comment
@vstinner

vstinner Jun 28, 2017

Member

Hum, did I miss something? sysconfig is still imported by the site module on macOS by getsitepackages():

        if sys.platform == "darwin":
            # for framework builds *only* we add the standard Apple
            # locations.
            from sysconfig import get_config_var
            framework = get_config_var("PYTHONFRAMEWORK")
            if framework:
                sitepackages.append(
                        os.path.join("/Library", framework,
                            '%d.%d' % sys.version_info[:2], "site-packages"))

macbook:master haypo$ ./python.exe -c 'import sys; print("sysconfig" in sys.modules)'
True

I suggest this additionnal change:

diff --git a/Lib/site.py b/Lib/site.py
index 500c59b..929252d 100644
--- a/Lib/site.py
+++ b/Lib/site.py
@@ -334,15 +334,12 @@ def getsitepackages(prefixes=None):
         else:
             sitepackages.append(prefix)
             sitepackages.append(os.path.join(prefix, "lib", "site-packages"))
-        if sys.platform == "darwin":
-            # for framework builds *only* we add the standard Apple
-            # locations.
-            from sysconfig import get_config_var
-            framework = get_config_var("PYTHONFRAMEWORK")
-            if framework:
-                sitepackages.append(
-                        os.path.join("/Library", framework,
-                            '%d.%d' % sys.version_info[:2], "site-packages"))
+        # for framework builds *only* we add the standard Apple
+        # locations.
+        if sys.platform == "darwin" and sys._framework:
+            sitepackages.append(
+                    os.path.join("/Library", sys._framework,
+                        '%d.%d' % sys.version_info[:2], "site-packages"))
     return sitepackages
 
 def addsitepackages(known_paths, prefixes=None):

With this additionnal change, the speedup on macOS is quite significant: -13.4 ms (1.61x faster)!

macbook:master haypo$ ./python.exe -m perf command --inherit=PYTHONPATH -v -o ref.json -- ./python.exe -c pass 
...
macbook:master haypo$ ./python.exe -m perf command --inherit=PYTHONPATH -v -o pr136.json -- ./python.exe -c pass
...
macbook:master haypo$ ./python.exe -m perf compare_to ref.json pr136.json 
Mean +- std dev: [ref] 35.4 ms +- 1.7 ms -> [pr136] 22.0 ms +- 1.9 ms: 1.61x faster (-38%)

cc @1st1: Yury, since you use macOS, you probably want to see this change merged :-)

Member

vstinner commented Jun 28, 2017

Hum, did I miss something? sysconfig is still imported by the site module on macOS by getsitepackages():

        if sys.platform == "darwin":
            # for framework builds *only* we add the standard Apple
            # locations.
            from sysconfig import get_config_var
            framework = get_config_var("PYTHONFRAMEWORK")
            if framework:
                sitepackages.append(
                        os.path.join("/Library", framework,
                            '%d.%d' % sys.version_info[:2], "site-packages"))

macbook:master haypo$ ./python.exe -c 'import sys; print("sysconfig" in sys.modules)'
True

I suggest this additionnal change:

diff --git a/Lib/site.py b/Lib/site.py
index 500c59b..929252d 100644
--- a/Lib/site.py
+++ b/Lib/site.py
@@ -334,15 +334,12 @@ def getsitepackages(prefixes=None):
         else:
             sitepackages.append(prefix)
             sitepackages.append(os.path.join(prefix, "lib", "site-packages"))
-        if sys.platform == "darwin":
-            # for framework builds *only* we add the standard Apple
-            # locations.
-            from sysconfig import get_config_var
-            framework = get_config_var("PYTHONFRAMEWORK")
-            if framework:
-                sitepackages.append(
-                        os.path.join("/Library", framework,
-                            '%d.%d' % sys.version_info[:2], "site-packages"))
+        # for framework builds *only* we add the standard Apple
+        # locations.
+        if sys.platform == "darwin" and sys._framework:
+            sitepackages.append(
+                    os.path.join("/Library", sys._framework,
+                        '%d.%d' % sys.version_info[:2], "site-packages"))
     return sitepackages
 
 def addsitepackages(known_paths, prefixes=None):

With this additionnal change, the speedup on macOS is quite significant: -13.4 ms (1.61x faster)!

macbook:master haypo$ ./python.exe -m perf command --inherit=PYTHONPATH -v -o ref.json -- ./python.exe -c pass 
...
macbook:master haypo$ ./python.exe -m perf command --inherit=PYTHONPATH -v -o pr136.json -- ./python.exe -c pass
...
macbook:master haypo$ ./python.exe -m perf compare_to ref.json pr136.json 
Mean +- std dev: [ref] 35.4 ms +- 1.7 ms -> [pr136] 22.0 ms +- 1.9 ms: 1.61x faster (-38%)

cc @1st1: Yury, since you use macOS, you probably want to see this change merged :-)

@vstinner

New review.

Show outdated Hide outdated Lib/site.py
Show outdated Hide outdated Lib/site.py
Show outdated Hide outdated Lib/site.py
@vstinner

This comment has been minimized.

Show comment
Hide comment
@vstinner

vstinner Jun 28, 2017

Member

Ned Deily: "This is a major change: by duplicating code from sysconfig.py into site.py, there would now be a implicit link between the two and a potential maintenance problem. If we were to do this, there should at least be mention of these duplications in sysconfig.py and/or tests for the duplicated behavior."

@methane added a comment in sysconfig.py, IMHO it's enough.

About testing: @methane, can you try to write a test to check that site and sysconfig return the same value for the two private functions? You may have to tag the unit test with @cpython_only, since the two new site functions are private.

Ned Deily: "Also, there should be a b.p.o issue for this proposed change."

Done.

Since most Ned's requests are done, I dismiss his review.

Member

vstinner commented Jun 28, 2017

Ned Deily: "This is a major change: by duplicating code from sysconfig.py into site.py, there would now be a implicit link between the two and a potential maintenance problem. If we were to do this, there should at least be mention of these duplications in sysconfig.py and/or tests for the duplicated behavior."

@methane added a comment in sysconfig.py, IMHO it's enough.

About testing: @methane, can you try to write a test to check that site and sysconfig return the same value for the two private functions? You may have to tag the unit test with @cpython_only, since the two new site functions are private.

Ned Deily: "Also, there should be a b.p.o issue for this proposed change."

Done.

Since most Ned's requests are done, I dismiss his review.

@methade modified this PR.

@vstinner

This comment has been minimized.

Show comment
Hide comment
@vstinner

vstinner Jun 28, 2017

Member

Except of my minor commits, I now like the overall shape of the PR. I now agree that writing a new _site module is not necessary, it's better to keep all code in the site.py file.

Member

vstinner commented Jun 28, 2017

Except of my minor commits, I now like the overall shape of the PR. I now agree that writing a new _site module is not necessary, it's better to keep all code in the site.py file.

@methane

This comment has been minimized.

Show comment
Hide comment
@methane

methane Jun 28, 2017

Member

On macOS, performance gain is more impressive than Linux:

$ ./python.exe -m perf compare_to ref.json pr136.json
Mean +- std dev: [ref] 30.4 ms +- 0.9 ms -> [pr136] 21.6 ms +- 4.0 ms: 1.40x faster (-29%)
Member

methane commented Jun 28, 2017

On macOS, performance gain is more impressive than Linux:

$ ./python.exe -m perf compare_to ref.json pr136.json
Mean +- std dev: [ref] 30.4 ms +- 0.9 ms -> [pr136] 21.6 ms +- 4.0 ms: 1.40x faster (-29%)
@methane

This comment has been minimized.

Show comment
Hide comment
@methane

methane Jun 28, 2017

Member

Oh, I'm sorry. I missed your above comment.

Member

methane commented Jun 28, 2017

Oh, I'm sorry. I missed your above comment.

@vstinner

LGTM, but I would like to see an answer to the question on tests (check that site functions return the same result than sysconfig?) before approving your change :-)

@vstinner

Thanks! The latest change is much better than the first iteration, and it now LGTM!

@methane methane merged commit a8f8d5b into python:master Jun 28, 2017

2 checks passed

bedevere/issue-number Issue number 29585 found.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@methane methane deleted the methane:optimize-site-startup branch Jun 28, 2017

@methane

This comment has been minimized.

Show comment
Hide comment
@methane

methane Jun 28, 2017

Member

Thank you for review.

Member

methane commented Jun 28, 2017

Thank you for review.

@1st1

This comment has been minimized.

Show comment
Hide comment
@1st1

1st1 Jun 28, 2017

Member

Why couldn't we make 'sysconfig' to import a couple of private functions from 'site', why copying?

Member

1st1 commented Jun 28, 2017

Why couldn't we make 'sysconfig' to import a couple of private functions from 'site', why copying?

@vstinner

This comment has been minimized.

Show comment
Hide comment
@vstinner

vstinner Jun 28, 2017

Member

Why couldn't we make 'sysconfig' to import a couple of private functions from 'site', why copying?

It's not a pure copy/paste, one function is specialized for site use case. I dislike the idea of importing site code from sysconfig.

Member

vstinner commented Jun 28, 2017

Why couldn't we make 'sysconfig' to import a couple of private functions from 'site', why copying?

It's not a pure copy/paste, one function is specialized for site use case. I dislike the idea of importing site code from sysconfig.

@vstinner

This comment has been minimized.

Show comment
Hide comment
@vstinner

vstinner Jun 28, 2017

Member

Thank you for review.

Thanks for this cool and cheap speedup!

Member

vstinner commented Jun 28, 2017

Thank you for review.

Thanks for this cool and cheap speedup!

@methane

This comment has been minimized.

Show comment
Hide comment
@methane

methane Jun 28, 2017

Member

Ah, I've missed build failure on Windows.

Member

methane commented Jun 28, 2017

Ah, I've missed build failure on Windows.

@vstinner

This comment has been minimized.

Show comment
Hide comment
@vstinner

vstinner Jun 28, 2017

Member

Oh, Windows build is broken :-(

     2>..\Python\sysmodule.c(1968): error C2065: 'PYTHONFRAMEWORK': undeclared identifier [C:\buildbot.python.org\3.x.kloth-win64\build\PCbuild\pythoncore.vcxproj]

Need to fix PC/pyconfig.h?

Member

vstinner commented Jun 28, 2017

Oh, Windows build is broken :-(

     2>..\Python\sysmodule.c(1968): error C2065: 'PYTHONFRAMEWORK': undeclared identifier [C:\buildbot.python.org\3.x.kloth-win64\build\PCbuild\pythoncore.vcxproj]

Need to fix PC/pyconfig.h?

@vstinner

This comment has been minimized.

Show comment
Hide comment
@vstinner

vstinner Jun 28, 2017

Member

Oh, AppVeyor didn't catch the bug simply because it wasn't run on this PR!

Member

vstinner commented Jun 28, 2017

Oh, AppVeyor didn't catch the bug simply because it wasn't run on this PR!

@vstinner

This comment has been minimized.

Show comment
Hide comment
@vstinner

vstinner Jun 28, 2017

Member

@methane proposed PR #2476 to fix PC/pyconfig.h, I proposed PR #2477.

Member

vstinner commented Jun 28, 2017

@methane proposed PR #2476 to fix PC/pyconfig.h, I proposed PR #2477.

vstinner added a commit that referenced this pull request Jun 28, 2017

akruis added a commit to akruis/cpython that referenced this pull request Oct 10, 2017

akruis added a commit to akruis/cpython that referenced this pull request Oct 29, 2017

asvetlov pushed a commit to asvetlov/cpython that referenced this pull request Oct 10, 2018

Merge pull request python#136 from parmentelat/develop
Make note about OpenSSH more visible
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment