build: introduce `./configure --with-lto` by indutny · Pull Request #7408 · nodejs/node

indutny · 2016-06-24T17:46:45Z

Checklist

make -j4 test (UNIX) or vcbuild test nosign (Windows) passes
a test and/or benchmark is included
documentation is changed or added
the commit message follows commit guidelines

Affected core subsystem(s)

build

Description of change

build: introduce ./configure --with-lto

Introduce --with-lto configure option to use Link Time Optimization
compiler pass in gcc/clang compilers.

This flag slightly improves performance of C/C++ heavy code:

tls/throughput.js dur="5" type="buf" size="2": ./out/Release/node-flto: 6.1295 ./out/Release/node: 5.6876 ....... 7.77%
tls/throughput.js dur="5" type="buf" size="1024": ./out/Release/node-flto: 1567 ./out/Release/node: 1455 ........ 7.70%
tls/throughput.js dur="5" type="buf" size="1048576": ./out/Release/node-flto: 4167.8 ./out/Release/node: 4026.7 . 3.50%
tls/throughput.js dur="5" type="asc" size="2": ./out/Release/node-flto: 5.5664 ./out/Release/node: 5.0363 ...... 10.52%
tls/throughput.js dur="5" type="asc" size="1024": ./out/Release/node-flto: 1462.2 ./out/Release/node: 1336.8 .... 9.38%
tls/throughput.js dur="5" type="asc" size="1048576": ./out/Release/node-flto: 3947.5 ./out/Release/node: 3847.4 . 2.60%
tls/throughput.js dur="5" type="utf" size="2": ./out/Release/node-flto: 5.5544 ./out/Release/node: 5.0786 ....... 9.37%
tls/throughput.js dur="5" type="utf" size="1024": ./out/Release/node-flto: 1328.5 ./out/Release/node: 1255.7 .... 5.80%
tls/throughput.js dur="5" type="utf" size="1048576": ./out/Release/node-flto: 3051.1 ./out/Release/node: 2985.1 . 2.21%

See: #7400

R= @bnoordhuis @nodejs/collaborators

Introduce `--with-lto` configure option to use Link Time Optimization compiler pass in gcc/clang compilers. This flag slightly improves performance of C/C++ heavy code: tls/throughput.js dur="5" type="buf" size="2": ./out/Release/node-flto: 6.1295 ./out/Release/node: 5.6876 ....... 7.77% tls/throughput.js dur="5" type="buf" size="1024": ./out/Release/node-flto: 1567 ./out/Release/node: 1455 ........ 7.70% tls/throughput.js dur="5" type="buf" size="1048576": ./out/Release/node-flto: 4167.8 ./out/Release/node: 4026.7 . 3.50% tls/throughput.js dur="5" type="asc" size="2": ./out/Release/node-flto: 5.5664 ./out/Release/node: 5.0363 ...... 10.52% tls/throughput.js dur="5" type="asc" size="1024": ./out/Release/node-flto: 1462.2 ./out/Release/node: 1336.8 .... 9.38% tls/throughput.js dur="5" type="asc" size="1048576": ./out/Release/node-flto: 3947.5 ./out/Release/node: 3847.4 . 2.60% tls/throughput.js dur="5" type="utf" size="2": ./out/Release/node-flto: 5.5544 ./out/Release/node: 5.0786 ....... 9.37% tls/throughput.js dur="5" type="utf" size="1024": ./out/Release/node-flto: 1328.5 ./out/Release/node: 1255.7 .... 5.80% tls/throughput.js dur="5" type="utf" size="1048576": ./out/Release/node-flto: 3051.1 ./out/Release/node: 2985.1 . 2.21% See: nodejs#7400

indutny · 2016-06-24T17:48:36Z

It appears to be breakingtest/parallel/test-tick-processor.js, and the build is quite slower than the one without LTO.

I wonder if addons would still work just fine?

eljefedelrodeodeljefe · 2016-06-24T17:51:58Z

hmm. side effects are weird. Nothing should pop up really....

Having this has no harm though. If that holds true, LGTM.

addaleax · 2016-06-25T03:45:29Z

btw, this fails to link for me when using gcc (gcc version 5.2.1 20151010 x86_64-linux-gnu), see here for the output.

EDIT: figured out what’s going wrong, I had to set AR=gcc-ar … 🙄

bnoordhuis · 2016-06-25T06:35:10Z

 'ldflags!': [ '-rdynamic' ],
 }],
+ ['node_use_lto=="true"', {
+ 'conditions': [ [ 'clang==1', {


Can you put the condition on a separate line?

bnoordhuis · 2016-06-25T06:43:22Z

It appears to be breakingtest/parallel/test-tick-processor.js

@indutny Let me guess: it can't find the RunInDebugContext symbol?

btw, this fails to link for me when using gcc

@addaleax What binutils version do you have installed and are you using ld.gold to link?

addaleax · 2016-06-25T13:55:01Z

GNU ld (GNU Binutils for Ubuntu) 2.25.1, switching to gold doesn’t seem to make any difference though.

jasnell · 2016-08-18T23:09:54Z

@indutny .. what's the status on this one?

indutny · 2016-08-19T05:25:13Z

@jasnell seems to be breaking profiler tests, and the effects may be local to only OS X. Not sure if we want it at this time.

jasnell · 2016-08-19T14:26:26Z

Ok. do you want to keep it open to work on later or close?

indutny · 2016-08-19T15:21:34Z

@jasnell I would keep it open, the conflicts are not going to be too hard to resolve later on.

jasnell · 2017-01-06T19:39:22Z

ping @indutny ... any updates on this?

addaleax · 2017-02-03T16:51:48Z

@indutny By the way, for me this seems to work now. Setting AR=gcc-ar did the trick. :)

I have a rebased version at d6b510c (resolving the conflict here is trivial), CI results @ https://ci.nodejs.org/job/node-test-commit/7653/. Some of the CI hosts ran into the same problem I had, but the OS X tests look a bit worrying: https://ci.nodejs.org/job/node-test-commit-osx/7615/nodes=osx1010/console

thefourtheye · 2017-02-04T10:02:51Z

 help='build with Lttng (Only available to Linux)')

+parser.add_option('--with-lto',
+ action='store_true',


Tiny nit: It would be better we passed default=False as one of the arguments to add_option.

indutny · 2017-02-06T22:57:29Z

I'm still not sure if we should pursue this... @addaleax except for the build failures, does it provide any measurable performance benefit for you?

addaleax · 2017-02-07T12:24:40Z

@indutny I can try to run some benchmarking later, but my personal experience would be that generally LTO is worth it.

jasnell · 2017-08-24T23:13:21Z

Closing due to lack of further progress. We can reopen later if necessary

Fishrock123 added c++ Issues and PRs that require attention from people who are familiar with C++. build Issues and PRs related to build files or the CI. labels Jun 24, 2016

bnoordhuis reviewed Jun 25, 2016
View reviewed changes

Trott force-pushed the master branch from b0df363 to c5ce7f4Compare September 21, 2016 00:09

rvagg force-pushed the master branch 2 times, most recently from c133999 to 83c7a88Compare October 18, 2016 17:02

jasnell added the stalled Issues and PRs that are stalled. label Jan 6, 2017

MylesBorins force-pushed the master branch from 8df7ee0 to 54fef67Compare February 1, 2017 01:00

thefourtheye reviewed Feb 4, 2017
View reviewed changes

refack force-pushed the master branch from 16073c0 to fbe946bCompare April 14, 2017 04:11

bnoordhuis mentioned this pull request Jul 8, 2017
-flto on clang/gcc? #7400
Closed

jasnell closed this Aug 24, 2017

bzoz mentioned this pull request Jun 7, 2018
build, win: make LTCG optional #21186
Closed
2 tasks

Uh oh!

Uh oh!

Conversation

indutny commented Jun 24, 2016

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

indutny commented Jun 24, 2016

Uh oh!

eljefedelrodeodeljefe commented Jun 24, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

addaleax commented Jun 25, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bnoordhuisJun 25, 2016

Choose a reason for hiding this comment

Uh oh!

bnoordhuis commented Jun 25, 2016

Uh oh!

addaleax commented Jun 25, 2016

Uh oh!

jasnell commented Aug 18, 2016

Uh oh!

indutny commented Aug 19, 2016

Uh oh!

jasnell commented Aug 19, 2016

Uh oh!

indutny commented Aug 19, 2016

Uh oh!

jasnell commented Jan 6, 2017

Uh oh!

addaleax commented Feb 3, 2017

Uh oh!

thefourtheyeFeb 4, 2017

Choose a reason for hiding this comment

Uh oh!

indutny commented Feb 6, 2017

Uh oh!

addaleax commented Feb 7, 2017

Uh oh!

jasnell commented Aug 24, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

eljefedelrodeodeljefe commented Jun 24, 2016•
edited
Loading

addaleax commented Jun 25, 2016•
edited
Loading