lto: enabling lto at configure #21677
Conversation
nodejs-github-bot
added
the
build
|
Note for anyone landing this: Subsystem should be @octaviansoldea If you want to amend the commit message for that, it will save someone a few keystrokes later on. |
This modification allows for compiling with link time optimization (lto) using the flag --enable-lto. Refs: nodejs#7400
|
Hello Thank you for the comment. I will update the commit asap. |
octaviansoldea
force-pushed
the
enabling_lto
branch
from
53f94ec
to
9aa749f
Compare
|
Hello Thank you again for the comment. I have updated the label with amending. I am more than happy to receive feedback and comments. |
configure
Outdated
| for compiler in [(CC, 'c'), (CXX, 'c++')]: | ||
| ok, is_clang, clang_version, compiler_version = \ | ||
| try_check_compiler(compiler[0], compiler[1]) | ||
| if is_clang or compiler_version < version_checked: |
There was a problem hiding this comment.
String-based version range checking like this is not foolproof, for example: '10.0.0' < '5.4.1' will evaluate to True. At the very least we would need a version comparison function that simply splits on '.' and compares each integer value between two version strings.
There was a problem hiding this comment.
Or do a tuple comparison, e.g.,
Line 727 in 0a78f7d
There was a problem hiding this comment.
This commit is a refactoring for testing that the compilers used for lto are gcc and g++ only.
|
Dear Reviewers, Thank you for your valuable comments and feedback. I have implemented the suggested modifications. However, I would like to point out an issue I believe it is a problem to be addressed. The calls ok, is_clang, clang_version, gcc_version = try_check_compiler(CXX, 'c++') and ok, is_clang, clang_version, gcc_version = try_check_compiler(CC, 'c') return clang_version and gcc_version as tuple of strings. This means, most probably, the subsequent tests ... are a bit strange. Could you please tell if you want that I open a separate issue on this? Thank you for valuable comments and feedback again. |
addaleax
added
the
author ready
|
CI: https://ci.nodejs.org/job/node-test-pull-request/15788/ Whoever lands this: Please be reminded to change the subsystem label to |
|
Dear Reviewers I saw that "node-test-commit — tests failed". Could you please tell if is there anything I am supposed to do? Thank you in advance, |
|
@octaviansoldea ... looks like an unrelated flaky failure. I'm running that one OSX test again: https://ci.nodejs.org/job/node-test-commit-osx/19766/ If that comes back green we should be able to land. |
|
Landed in 32cad73, thanks for the PR! 🎉 |
This modification allows for compiling with link time optimization (lto) using the flag --enable-lto. Refs: #7400 PR-URL: #21677 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
This modification allows for compiling with link time optimization (lto) using the flag --enable-lto. Refs: #7400 PR-URL: #21677 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
|
@octaviansoldea Is it correct that this PR does not enable Link Time Optimization by default? If so, is that the goal in the future and is there already a PR/issue where we can track that change/discussion? |
This modification allows for compiling with link time optimization (lto) using the flag --enable-lto.
Refs: #7400
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesThis is a modification proposed towards enabling lto compilation. From some preliminary results, I observed that there are no major changes in performance when running Node-DC-EIS. However, when coupling lto with pgo, see also Refs: #21583, I observed improvements of even more than 10% in Node-DC-EIS.
The proposed solution, when enabling lto, manifests a test failure, which seems to be not very critical. In this context, please see comments and feedback from @addaleax at Refs: #7400.
The experiments were done on
Intel(R) Xeon(R) Platinum 8180 CPU @ 2.50GHz
and
Intel(R) Xeon(R) CPU E5-2699 v4 @ 2.20GHz.