Skip to content
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.

Sign up for GitHub

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

Jump to bottom

build, win: make LTCG optional #21186

Closed
wants to merge 1 commit into from
Closed

build, win: make LTCG optional #21186

wants to merge 1 commit into from

Conversation

bzoz
Copy link
Contributor

@bzoz bzoz commented Jun 7, 2018

Add noltcg option to vcbuild that disables Link Time Code Generation. While clean build is only slightly faster (19 min vs 20), rebuild after change in one of the js files now takes 20 seconds instead of 2 minutes.

/cc @nodejs/platform-windows

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. windows Issues and PRs related to the Windows platform. labels Jun 7, 2018
@bzoz
Copy link
Contributor Author

bzoz commented Jun 7, 2018

CI: https://ci.nodejs.org/job/node-test-pull-request/15309/

For reference, there was also similar PR for Linux: #7408

@targos
Copy link
Member

targos commented Jun 7, 2018

I can't review the code changes but +1000 on making Windows rebuilds faster!

addaleax
addaleax approved these changes Jun 7, 2018
View reviewed changes
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank You.

We may want to advertise this in BUILDING.md as well, I think.

@seishun
Copy link
Contributor

seishun commented Jun 7, 2018

How does it affect the performance of the resulting binary?

@Fishrock123
Copy link
Contributor

To be clear, our windows releases should use --with-ltcg now, right?

@bzoz
Copy link
Contributor Author

bzoz commented Jun 7, 2018

@seishun: I did not check that yet, but surely there is some
@Fishrock123: yes, but it is on by default

refack
refack reviewed Jun 7, 2018
View reviewed changes
configure
parser.add_option('--with-ltcg',
action='store_true',
dest='with_ltcg',
help='Use Link Time Code Generation. This feature is only available on Windows.')
Copy link
Contributor

@refack refack Jun 7, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest flipping the switch to --disable-windows-ltcg (and use store-_false) similar to:
https://github.com/nodejs/node/blob/23efa8cc0692e690fd5138f5b93ccfd4e47c2f58/configure#L509-L517

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to keep this in line with Linux - there was PR to add --with-lto, at some point, when it will be ready that can be merged into this option

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The difference is that if we want LTCG to be the default, it makes more sense to have an opt-out --without flag so the call to python configure could stay simple.

BTW I meant to refeance: https://github.com/nodejs/node/blob/23efa8cc0692e690fd5138f5b93ccfd4e47c2f58/configure#L525-L528

refack
refack suggested changes Jun 7, 2018
View reviewed changes
Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A very blessed optimization.
But I would suggest flipping the switch semantics.

common.gypi
@@ -16,6 +16,7 @@
'node_use_v8_platform%': 'true',
'node_use_bundled_v8%': 'true',
'node_module_version%': '',
'node_with_ltcg%': '',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO default should be 'true'

vcbuild.bat Outdated
@@ -15,6 +15,7 @@ cd %~dp0
set config=Release
set target=Build
set target_arch=x64
set ltcg=1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again IMHO switch should be flipped (since default should stay to enable LTCG).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean it should be set no-ltcg=0

@richardlau
Copy link
Member

Since this touches common.gypi how will this affect addons built with node-gyp? There's a long standing node-gyp issue to move away from using core's common.gypi (nodejs/node-gyp#1118) but for now it will be used.

For example, nodejs/node-gyp#1457 was recently opened regarding compiling addons for a different version of Node.js than was used to run node-gyp.

@addaleax
Copy link
Member

addaleax commented Jun 10, 2018 •
edited
Loading

@refack Setting no LTCG as the default is definitely the more developer-friendly thing to do here … we should just make sure to keep it enabled in CI?

@refack
Copy link
Contributor

refack commented Jun 10, 2018

Setting no LTCG as the default is definitely the more developer-friendly thing to do here … we should just make sure to keep it enabled in CI?

I agree, that's why I prefer to have an opt-out switch (LTCG on by default, switch to turn it off).
I usually call configure manually so it will make a difference, while for those using vcbuild I agree it doesn't really matter.

@refack
Copy link
Contributor

refack commented Jun 10, 2018

Also if we turn this into an opt-out switch, we could keep vcbuild.bat unchanged and use the environment variable config_flags

set config_flags=--without-ltcg
vcbuild.bat

joyeecheung
joyeecheung approved these changes Jun 11, 2018
View reviewed changes
Copy link
Member

@joyeecheung joyeecheung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, I am also +1 on disabling LTO on Windows by default as long as we make sure it is enabled in CI.

@bzoz
Copy link
Contributor Author

bzoz commented Jun 11, 2018

@refack: how about making noltcg the default for vcbuild (but keeping LTCG on for build-release)?

@bzoz bzoz force-pushed the bartek-optional-ltcg branch from 23efa8c to d7f9e50 Compare June 12, 2018 13:42
@bzoz
Copy link
Contributor Author

bzoz commented Jun 12, 2018

Rebased on master.

@bzoz
Copy link
Contributor Author

bzoz commented Jun 13, 2018

Updated: made LTCG disabled by default. It will be enabled for build-release builds.

@joaocgreis
Copy link
Member

Test CI uses vcbuild release, so LTCG needs to be enabled on that case as well.

@refack refack added semver-minor PRs that contain new features and should be released in the next minor version. discuss Issues opened for discussions and feedbacks. blocked PRs that are blocked by other issues or PRs. labels Jun 13, 2018
@refack
Copy link
Contributor

refack commented Jun 13, 2018

how about making noltcg the default for vcbuild (but keeping LTCG on for build-release)?

I'm Ok with that but it might have higher semver-ity. Definitely minor for a new option, maybe major since we are changing default behaviour.

There is a tradeoff here

  • Faster link time (which is minutes in 100% CPU)
    on the other hand
  • We are creating two tiers of non-debug binaries: Release and Release+LTCG. Which one should be tested in CI

/CC @nodejs/platform-windows @nodejs/build Any opinions?

@richardlau
Copy link
Member

We are creating two tiers of non-debug binaries: Release and Release+LTCG. Which one should be tested in CI

Whichever we end up using for the shipped binaries (i.e., Release+LTCG), given that at the moment we don't run tests on the release builds.

@bzoz
Copy link
Contributor Author

bzoz commented Jun 14, 2018

Made LTCG enabled for release builds. CI will compile and use Node with LTCG enabled, same as published binary.

@bzoz
Copy link
Contributor Author

bzoz commented Jun 20, 2018

@refack, @bnoordhuis, @jasnell and @joyeecheung - this was changed since your reviews. Now, LTCG is off by default, enabled only for CI testing and releases. Is this ok with you?

@bzoz
Copy link
Contributor Author

bzoz commented Jun 26, 2018

CI: https://ci.nodejs.org/job/node-test-pull-request/15628/

@bzoz
build, win: make LTCG optional
f2a6b28 
Disables Link Time Code Generation by default. Adds ‘ltcg’ vcbuild
option to enable it. LTCG will be used by default by release and CI
builds.
@bzoz bzoz force-pushed the bartek-optional-ltcg branch from fd6cf18 to f2a6b28 Compare June 26, 2018 17:45
@bzoz
Copy link
Contributor Author

bzoz commented Jun 26, 2018

Rebased, new CI: https://ci.nodejs.org/job/node-test-pull-request/15636/

@bzoz
Copy link
Contributor Author

bzoz commented Jun 27, 2018

Green CI: https://ci.nodejs.org/job/node-test-commit/19488/

@bzoz
Copy link
Contributor Author

bzoz commented Jun 27, 2018

Landed in 7c45284

@bzoz bzoz closed this Jun 27, 2018
bzoz added a commit that referenced this pull request Jun 27, 2018
@bzoz
build, win: make LTCG optional
7c45284 
Disables Link Time Code Generation by default. Adds ‘ltcg’ vcbuild
option to enable it. LTCG will be used by default by release and CI
builds.

PR-URL: #21186
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
targos pushed a commit that referenced this pull request Jun 28, 2018
@bzoz @targos
build, win: make LTCG optional
76ef7ac 
Disables Link Time Code Generation by default. Adds ‘ltcg’ vcbuild
option to enable it. LTCG will be used by default by release and CI
builds.

PR-URL: #21186
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
@targos targos mentioned this pull request Jul 3, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joyeecheung joyeecheung joyeecheung approved these changes

@addaleax addaleax addaleax approved these changes

@jasnell jasnell jasnell approved these changes

@bnoordhuis bnoordhuis bnoordhuis approved these changes

@refack refack refack approved these changes

Assignees
No one assigned
Labels
blocked PRs that are blocked by other issues or PRs. build Issues and PRs related to build files or the CI. discuss Issues opened for discussions and feedbacks. semver-minor PRs that contain new features and should be released in the next minor version. windows Issues and PRs related to the Windows platform.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.