Skip to content

Conversation

@jkloth
Copy link
Contributor

@jkloth jkloth commented Apr 5, 2022

By using a static library for the shared (unchanging) sources between extensions, this reduces the number of spawned processes for compiling thus reducing the runtime ~25%.

On my machine: before 85s, after 65s.

https://bugs.python.org/issue46576

Automerge-Triggered-By: GH:pablogsal

@pablogsal
Copy link
Member

Ah, fantastic idea! Let's check that the CI works and I can land it afterwards. Thanks for working on this @jkloth 🙏

Copy link
Member

@gpshead gpshead left a comment

Choose a reason for hiding this comment

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

Great change! My comment are all around expanding on some code comments. otherwise this is good.

@bedevere-bot
Copy link

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 I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@jkloth
Copy link
Contributor Author

jkloth commented Apr 5, 2022

Hrm, in my zeal over the improvement on Windows, I did forget to run through a POSIX build. Once I get that sorted out, I'll address the other changes.

@jkloth
Copy link
Contributor Author

jkloth commented Apr 5, 2022

I have resolved the issues when running on POSIX (well, Linux). I am quite surprised by the speedup there, before 33s, after 6s.

@gpshead gpshead added performance Performance or resource usage tests Tests in the Lib/test dir needs backport to 3.10 only security fixes and removed 🤖 automerge labels Apr 5, 2022
@gpshead
Copy link
Member

gpshead commented Apr 5, 2022

LGTM once it proves it self with testing on a variety of platforms. But I'd wait until after Pablo has released the Alpha and unblocked main before applying the test-with-buildbots label to better verify this change (to avoid tying up buildbot resources until after the release)

@jkloth
Copy link
Contributor Author

jkloth commented Apr 5, 2022

One last change for MacOS (due to deprecated linker option). Hopefully this run (of test_peg_generator) should be error/warning free. And, yes, a run through the buildbots would be very beneficial.

@jkloth
Copy link
Contributor Author

jkloth commented Apr 5, 2022

Hoorah, the MacOS run of test_peg_generator is clean! It is a shame that network errors cause the GHA to fail. Is there something that can be done on our end to, say, add retries to parts of the scripts that can fail on network errors?

@gpshead gpshead added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Apr 6, 2022
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @gpshead for commit 09f2ce2 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Apr 6, 2022
@jkloth
Copy link
Contributor Author

jkloth commented Apr 6, 2022

Looks like everything passed. The failures are refleaks and 1 slow SSL handshake (gentoo).

@gpshead gpshead self-assigned this Apr 6, 2022
@gpshead gpshead merged commit 612e422 into python:main Apr 6, 2022
@miss-islington
Copy link
Contributor

Thanks @jkloth for the PR, and @gpshead for merging it 🌮🎉.. I'm working now to backport this PR to: 3.10.
🐍🍒⛏🤖

@miss-islington
Copy link
Contributor

Sorry, @jkloth and @gpshead, I could not cleanly backport this to 3.10 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 612e422c6ea9df60d3382ed1491d8254c283e93f 3.10

@gpshead gpshead removed the needs backport to 3.10 only security fixes label Apr 6, 2022
jkloth added a commit to jkloth/cpython that referenced this pull request Apr 6, 2022
…ry for shared sources (pythonGH-32338)

Speed up test_peg_generator by using a static library for shared sources to avoid recompiling as much code..
(cherry picked from commit 612e422)

Co-authored-by: Jeremy Kloth <[email protected]>
@jkloth
Copy link
Contributor Author

jkloth commented Apr 6, 2022

Well, I see that you removed the 3.10 backport label while I was working on the backport. Is it desired, or should I just close it?

@jkloth jkloth deleted the issue46576 branch April 6, 2022 22:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

performance Performance or resource usage skip news tests Tests in the Lib/test dir

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants