Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 34k
gh-90108: Disable LTO on _freeze_module and _testembed#109581
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
Uh oh!
There was an error while loading. Please reload this page.
Conversation
vstinner commented Sep 19, 2023 • edited by bedevere-app bot
Loading Uh oh!
There was an error while loading. Please reload this page.
edited by bedevere-app bot
Uh oh!
There was an error while loading. Please reload this page.
LTO optimization is nice to make Python faster, but _freeze_module and _testembed performance is not important. Using LTO to build these two programs make a whole Python build way slower, especially combined with a sanitizer (like ASAN).
vstinner commented Sep 19, 2023
vstinner commented Sep 19, 2023
I don't think that this change is important to document in the Changelog, since it has no impact on users, it only makes the build a little bit faster when LTO is used. |
vstinner commented Sep 19, 2023
I noticed this issue when working on PR #109576. |
corona10 commented Sep 19, 2023 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
Did you check that build is passed with the clang(thinLTO)? |
corona10 commented Sep 19, 2023 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
PTAL: #96761 (comment) I am not sure that it will happen again, but you need check. |
vstinner commented Sep 19, 2023
Before, I only tested my PR with GCC. It seems like in practice, this change has no effect on clang with Python is built with
Sure. It just works, but there is a trick: Code added by you in commit 83d84e6 :-) In short, "NOLTO" does not disable LTO but enable LTO instead :-) I also tried:
Both commands produce Python build works (with my PR) in both cases. If I modify manually Makefile to use But... Python also fails on the main branch (without this PR) if manually edit Makefile to use Well: |
vstinner commented Sep 19, 2023
I discovered "NOLTO" variables when I worked on PR #109576: "Don't export -fsanitize compiler options". |
corona10 left a comment
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.
LGTM
vstinner commented Sep 20, 2023
bedevere-bot commented Sep 20, 2023
|
erlend-aasland commented Sep 21, 2023
Nice, thanks Victor! |
…109581) LTO optimization is nice to make Python faster, but _freeze_module and _testembed performance is not important. Using LTO to build these two programs make a whole Python build way slower, especially combined with a sanitizer (like ASAN).
…e and _testembed (python#109581)" This reverts commit 3e3a7da.
…109581) LTO optimization is nice to make Python faster, but _freeze_module and _testembed performance is not important. Using LTO to build these two programs make a whole Python build way slower, especially combined with a sanitizer (like ASAN).
…e and _teste… (python#110720) pythongh-110313: Revert "pythongh-90108: Disable LTO on _freeze_module and _testembed (python#109581)" This reverts commit 3e3a7da.
LTO optimization is nice to make Python faster, but _freeze_module and _testembed performance is not important. Using LTO to build these two programs make a whole Python build way slower, especially combined with a sanitizer (like ASAN).