-
-
Notifications
You must be signed in to change notification settings - Fork 168
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
Enable full LTO on Python 3.12 and 3.13 #529
Conversation
@@ -427,7 +427,11 @@ if [ -n "${CPYTHON_OPTIMIZED}" ]; then | |||
fi | |||
|
|||
if [ -n "${CPYTHON_LTO}" ]; then | |||
CONFIGURE_FLAGS="${CONFIGURE_FLAGS} --with-lto" | |||
# On Python 3.12 and 3.13, `--with-lto` enables ThinLTO by default, while on other versions 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.
What about 3.14? Did it change in 3.14?
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.
3.14a5 and below still defaults to ThinLTO. 3.14a6 (upcoming) will default back to fullLTO.
It's safe to just pass --with-lto=full
to all versions of 3.14. That will always work.
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.
Yes the linked documentation says they're reverting the ThinLTO default in 3.14.
Seems best to just set it on all versions.
I'll try to do a brief analysis of the performance and build time trade-offs here. |
Total job time (main / branch)
Target job time (main / branch)
|
Surprisingly, this is consistently a bit slower on the pystones benchmark used for #524 on
|
This is less pronounced (but still present?) if I compare to an artifact from
|
I'll find another benchmark to try. The summary at python/cpython#122580 (comment) is relatively compelling. |
With the script from python/cpython#122580 (comment) — which was the original reproduction for a regression, I also don't see an improvement
|
@zanieb It might be that this was a bug in Apple Clang but fixed in normal Clang. The version of clang Ned used was So I'm sorry for the noise if that turns out to be the case. |
I think my guess is probably right. I will close the issue as the build time tradeoff is not worth it. So sorry for the noise! |
Does this mean I want to explicitly request |
I think I will revert the change upstream. It's probably somewhat disruptive now anyways to flip-flop. Thanks. |
Closes #528