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.

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

build, win: faster Release rebuilds #17393

Closed
wants to merge 1 commit into from

Conversation

bzoz
Copy link
Contributor

@bzoz bzoz commented Nov 29, 2017

Sets Link Time Code Generation to INCREMENTAL, improving Release rebuilds speed. The /LTCG:INCREMENTAL was added in VS2015 and is not directly supported in gyp, thus it is implemented with AdditionalOptions. Some more information can be found in this vcblog post.

Adds exe-only option to vcbuild.bat, which will build only node.exe binary. It will skip building cctest, which takes as long as building node.exe itself.

Currently, on my box vcbuild rebuild takes 8 minutes. With those changes vcbuild exe-only takes only 1.5 minute.

Ref: #17253

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

build

/cc @nodejs/build @nodejs/platform-windows

@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 Nov 29, 2017
@bzoz
Copy link
Contributor Author

bzoz commented Nov 29, 2017

@@ -186,10 +186,12 @@
],
},
'VCLinkerTool': {
'LinkTimeCodeGeneration': 1, # link-time code generation
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It adds /LTCG to the linker options, we want to use /LTCG:INCREMENTAL. Right now gyp does not support this option directly, so we add it through additional options.

vcbuild.bat Outdated
@@ -51,6 +51,7 @@ set "common_test_suites=%js_test_suites% doctool addons addons-napi&set build_ad
set http2_debug=
set nghttp2_debug=
set link_module=
set exe_only=
Copy link
Contributor

Choose a reason for hiding this comment

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

That's inconsistent with other arguments which looks like <feature> or no<feature>. How about nocctest instead? Or even better - maybe build cctest only if the test argument is specified?

@seishun
Copy link
Contributor

seishun commented Nov 30, 2017

Is there any difference in benchmark results?

@bzoz
Copy link
Contributor Author

bzoz commented Dec 4, 2017

Updated, changed exe-only to no-cctest

I did run benchmarks over the weekend, comparing node built with vs2017 - "old" using /LTCG and "new" using /LTCG:INCREMENTAL. Here are gists with results:

They seem to pretty much the same, although I'm not an expert in this.

@jasnell jasnell requested review from bnoordhuis and refack December 5, 2017 17:30
@bzoz bzoz force-pushed the bartek-faster-rebuild branch 2 times, most recently from ba4ac26 to 9185f89 Compare December 7, 2017 11:27
Sets Link Time Code Generation to INCREMENTAL improving Release
rebuilds speed.

Adds no-cctest option to vcbuild.bat, which will skip building
cctest.exe

PR-URL: nodejs#17393
Reviewed-By: Refael Ackermann <[email protected]>
@bzoz bzoz force-pushed the bartek-faster-rebuild branch from 9185f89 to ca0bb19 Compare December 7, 2017 11:28
@bzoz
Copy link
Contributor Author

bzoz commented Dec 7, 2017

@bzoz
Copy link
Contributor Author

bzoz commented Dec 7, 2017

Landed in 8514ea9

@bzoz bzoz closed this Dec 7, 2017
bzoz added a commit that referenced this pull request Dec 7, 2017
Sets Link Time Code Generation to INCREMENTAL improving Release
rebuilds speed.

Adds no-cctest option to vcbuild.bat, which will skip building
cctest.exe

PR-URL: #17393
Reviewed-By: Refael Ackermann <[email protected]>
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
Sets Link Time Code Generation to INCREMENTAL improving Release
rebuilds speed.

Adds no-cctest option to vcbuild.bat, which will skip building
cctest.exe

PR-URL: #17393
Reviewed-By: Refael Ackermann <[email protected]>
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
Sets Link Time Code Generation to INCREMENTAL improving Release
rebuilds speed.

Adds no-cctest option to vcbuild.bat, which will skip building
cctest.exe

PR-URL: #17393
Reviewed-By: Refael Ackermann <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Dec 12, 2017
@gibfahn gibfahn added baking-for-lts PRs that need to wait before landing in a LTS release. lts-watch-v6.x labels Dec 20, 2017
@MylesBorins
Copy link
Contributor

@nodejs/platform-windows is this something we want to backport to 8.x?

Seems like windows tooling for 8.x in general has fallen behind

@refack
Copy link
Contributor

refack commented Aug 17, 2018

I'd say backport if it lands smoothly. It's mostly a dev time optimization, IMHO the LTS line can take it or leave it...

@targos targos removed the baking-for-lts PRs that need to wait before landing in a LTS release. label Sep 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. windows Issues and PRs related to the Windows platform.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants