-
Notifications
You must be signed in to change notification settings - Fork 308
[Python] Remove obsolete configure args, build with optimizations + build with LTO for GCC >= 8.0 #1876
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
Conversation
|
Good point about |
|
Seems it already exists since v1: #157 |
easybuild/easyblocks/p/python.py
Outdated
| extra_vars = { | ||
| 'ulimit_unlimited': [False, "Ensure stack size limit is set to '%s' during build" % UNLIMITED, CUSTOM], | ||
| 'ebpythonprefixes': [True, "Create sitecustomize.py and allow use of $EBPYTHONPREFIXES", CUSTOM], | ||
| 'use_lto': [False, "Build with Link Time Optimization. Potentially unstable on some toolchains", CUSTOM], |
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.
@Flamefire Using use_lto = True doesn't do anything at all right now?
Also, can you clarify the "potentially unstable on some toolchains"?
Is it worth using None as a default here, and auto-enabling LTO for blessed toolchains (e.g. when a sufficiently recent version of GCC or Intel compiler is used as toolchain compiler)?
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.
Ups, rebase conflict gone wrong. Thanks
From https://docs.python.org/3.7/whatsnew/changelog.html#python-3-7-5-final
A –with-lto configure option has been added that will enable link time optimizations at build time during a make profile-opt. Some compilers and toolchains are known to not produce stable code when using LTO, be sure to test things thoroughly before relying on it. It can provide a few % speed up over profile-opt alone.
The suggestion with None==auto-detect is good. https://bugs.python.org/issue25702 seems to state that GCC>=4.8 is fine and apparently "Ubuntu" might have same kind of whitelist or blacklist when to enable this for Python. Not sure where to look for that though.
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.
https://bugs.python.org/issue28032 indicates GCC 5.4+
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.
I guess the question is how to determine if the resulting build is stable, and how to "test things thoroughly".
If it's only a few %, I'm not sure it's worth taking the risk to auto-enable 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.
The Python devs seem to be quite confident and if Ubuntu does it... But maybe we can at least do it in the ECS where we know the compilers, so no surprises
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.
How about this:
- use
Noneas default, so people can opt-in (True) or opt-out (False) in thePythoneasyconfig easily - auto-enable if
Noneis used, but only if a sufficiently recentGCCis used as compiler (probably not worth the auto-enable when Intel compilers are used in toolchain for installingPython, so we pulled downPythonto usingGCCcoreas toolchain a while ago)
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.
Sure. What would you deem "sufficiently recent GCC"? GCC 6+? 8+?
Do you have a snipped how to check for the used compiler and version?
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.
I'd prefer being conservative here, so why not go with GCC 8+.
Should be something like this:
if self.cfg['uselto'] is None and self.toolchain.comp_family() == toolchain.GCC:
gcc_ver = get_software_version('GCCcore') or get_software_version('GCC')
if LooseVersion(gcc_ver) >= LooseVersion('8.0'):
self.log.info("Auto-enabling --with-lto since GCC >= v8.0 is used as toolchain compiler")
self.cfg['uselto'] = TrueThere 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.
Ok. Slightly adapted and removed get_software_version('GCCcore'). If I got it right the if-check already ensures we have GCC
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.
Oh, it seems GCCcore is needed because it does not load the GCC module. This is an annoying pitfall and I'm not the only one:
easybuild-easyblocks/easybuild/easyblocks/l/libsmm.py: gccVersion = LooseVersion(get_software_version('GCC'))
There are probably more... Maybe make get_software_version('GCC) also check `GCCcore'?
|
FWIW: version checks look OK to me based on this: |
improve doc for 'optimized' and 'use_lto' custom easyconfig parameter for Python
|
Tested by rebuilding various No problems encounered, so good to go... Thanks @Flamefire! |
Current Python versions (3.0+ for unicode, 3.7+ for threads) include the asked features and removed the configure options resulting in
configure: WARNING: unrecognized options: --enable-unicodeThis PR removes those for the relevant python versions (checked on source checkouts)
Additionally Python 3.5.3 introduced optimized builds employing PGO and LTO for faster runtimes. Users report 5-30% improvement (see deadsnakes/python3.6#1) As EB is HPC centric using an optimized Python-runtime makes sense to me.
On the downside this adds 1h to the build time on our power machine due to the double build and test runs.
I added options to both enable the optimized build as well as
--with-ltowhich does not use profiling to speed up runtime but cross-object-optimizations. That will be easier on build time (expected slight increase) but with less performance benefit. Both can be combinedAs a follow-up I'd suggest making output like "WARNING: unrecognized options: " a hard failure to detect changed configure arguments. Imagine this was an important option or e.g. a rename (as happened from
--with-optimizationsto--enable-optimizations)