Skip to content
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

Jump to bottom

bpo-46576: Speed up test_peg_generator by using a static library for shared sources #32338

Merged
merged 4 commits into from Apr 6, 2022
Merged

bpo-46576: Speed up test_peg_generator by using a static library for shared sources #32338

merged 4 commits into from Apr 6, 2022

Conversation

jkloth
Copy link
Contributor

@jkloth jkloth commented Apr 5, 2022 •
edited by miss-islington

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 pablogsal added 🤖 automerge PR will be merged once it's been approved and all CI passed skip news labels Apr 5, 2022
@pablogsal
Copy link
Member

pablogsal commented Apr 5, 2022

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

gpshead
gpshead requested changes Apr 5, 2022
View changes
Copy link
Member

@gpshead gpshead left a comment

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

Tools/peg_generator/pegen/build.py Show resolved Hide resolved
Lib/test/test_peg_generator/test_c_parser.py Show resolved Hide resolved
@bedevere-bot
Copy link

bedevere-bot commented Apr 5, 2022

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 and removed 🤖 automerge PR will be merged once it's been approved and all CI passed 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

bedevere-bot commented Apr 6, 2022

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

If you want to schedule another build, you need to add the "🔨 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 approved these changes Apr 6, 2022
View changes
gpshead
gpshead approved these changes Apr 6, 2022
View changes
@gpshead gpshead merged commit 612e422 into python:main Apr 6, 2022
79 of 86 checks passed
@miss-islington
Copy link
Contributor

miss-islington commented Apr 6, 2022

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

miss-islington commented Apr 6, 2022

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

jkloth added a commit to jkloth/cpython that referenced this pull request Apr 6, 2022
@jkloth
[3.10] bpo-46576: Speed up test_peg_generator by using a static libra…
5c07f00 
…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 <jeremy.kloth@gmail.com>
@jkloth jkloth mentioned this pull request Apr 6, 2022
Closed
@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 Apr 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gpshead gpshead

@pablogsal pablogsal

@lysnikolaou lysnikolaou

Assignees

@gpshead gpshead

Labels
performance Performance or resource usage skip news tests Tests in the Lib/test dir
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@jkloth @pablogsal @bedevere-bot @gpshead @miss-islington @the-knights-who-say-ni