build,win: configuration tweaks for faster build with MSVC #25931

refack · 2019-02-04T21:19:46Z

Several optimization to reduce build time on Windows.

Run link-time optimizations and codegen only for the node.exe target.
Add more .h files to pch for V8
Use V8 pch with more targets
Add configuration to use pch while building node code
Set to always use pch.
IIUC the reason not to build V8 with pch was as not to diverge, but V8 is anyway built with clang_cl and not with the MSVC compiler, so divergence exists anyway.

Total build time on https://ci.nodejs.org/computer/test-rackspace-win2008r2-x64-3/:

Minutes	PR	master
release, no pch	22:56	46:10
"dev mode" - no pch, no LTCG	10:06	12:08
release with pch	10:18	-

/CC @nodejs/build-files @nodejs/platform-windows @nodejs/v8-update

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

nodejs-github-bot · 2019-02-04T21:19:49Z

@refack build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/2509/pipeline

refack · 2019-02-04T21:22:46Z

P.S. this also reduce divergence between "dev" builds (no LTCG) and "release" builds (with LTCG), as now they both use pch.

refack · 2019-02-05T01:41:05Z

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

refack · 2019-02-05T02:34:28Z

Rerun Windows https://ci.nodejs.org/job/node-test-commit-windows-fanned/24505/

joaocgreis

Was not using PCH by default as it carries some risk. It makes the build very different from other platforms (the headers are the same for all files, ignoring defines) and gives the compiler different opportunities for optimization. Since now it has been in use for some time, it's probably a good time to make it default.

deps/v8/gypfiles/v8.gyp

src/node_pch.cc

joaocgreis · 2019-02-05T21:09:00Z

src/node_pch.h

@@ -0,0 +1,15 @@
+#pragma once
+#define NAPI_EXPERIMENTAL


Is this always safe? I see in some files where this is defined other headers are included before. With this here, NAPI_EXPERIMENTAL will always be defined, can probably be removed from everywhere in the code.

There is also NODE_WANT_INTERNALS. There is and explicit define in inspector_socket.cc which will be ignored by all headers pulled by these here.

This might be safe, but is not obvious to me. Might also become difficult to maintain, since anyone not using Windows will only see errors when running CI (if at all).

joaocgreis · 2019-02-05T21:49:52Z

This should probably be semver major to be safe. I don't expect anything to break, but wouldn't be surprised if there were differences compiling modules or with performance. In my benchmarking of the PCH we have now, I noticed somethings were faster and some slower, by small margins, and I'm not sure how conclusive that was.

@bzoz can you take a look at the linking changes here?

refack · 2019-02-06T00:30:41Z

Very rough comparison

Baseline:

PR:

gengjiawen · 2019-02-06T01:11:21Z

So on windows run vcbuild.bat will be much more faster after this patch get merged, right ?

refack · 2019-02-06T02:15:14Z

So on windows run vcbuild.bat will be much more faster after this patch get merged, right ?

🤞 that's the dream

joaocgreis

LGTM, but it'd be good to have confirmation that there's no issue with the NODE_WANT_INTERNALS define in inspector_socket.cc. Thanks @refack!

refack · 2019-02-14T14:58:34Z

Since there should not be any explicit breaking changes, maybe we can commute the semver-major to dont-land-on-*?

/CC @nodejs/tsc

tl;dr this cuts build time on Windows by ~70%

targos · 2019-02-14T15:27:45Z

Since there should not be any explicit breaking changes, maybe we can commute the semver-major to dont-land-on-*?

SGTM

joaocgreis · 2019-02-14T15:42:14Z

SGTM. I added it, so I went ahead and changed it. Actually only the commit build,win: always build with PCH should be affected by this, and that only preemptively.

refack · 2019-02-14T17:43:40Z

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

* ASCIIbetize directives * Merge duplicate directives from 'conditions' PR-URL: nodejs#25931 Reviewed-By: João Reis <[email protected]>

* Use relative path for easy reuse in other projects PR-URL: nodejs#25931 Reviewed-By: João Reis <[email protected]>

* rename files to represent reuse PR-URL: nodejs#25931 Reviewed-By: João Reis <[email protected]>

PR-URL: nodejs#25931 Reviewed-By: João Reis <[email protected]>

deepak1556 · 2019-10-09T22:46:54Z

@refack this change moved the configuration of node_with_ltcg from common.gypi to node.gyp , so native addons built from node-gyp no longer get the configuration as result leads to increase in size. The following observation was made with node v12.4.0

Without ltcg (current change):

With ltcg added to node-gyp addon.gypi:

Would it make sense to add this definition to node-gyp rather than common.gypi so that even Electron and other embedders could benefit from this ? Thanks!

This was found while investigating electron/electron#18670

Trott · 2019-10-10T03:39:17Z

@nodejs/platform-windows @nodejs/node-gyp @nodejs/embedders

richardlau · 2019-10-10T04:42:14Z

Discussion is in #29501.

refack added windows Issues and PRs related to the Windows platform. build Issues and PRs related to build files or the CI. v8 engine Issues and PRs related to the V8 dependency. gyp Issues and PRs related to the GYP tool and .gyp build files labels Feb 4, 2019

refack self-assigned this Feb 4, 2019

nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Feb 4, 2019

joaocgreis reviewed Feb 5, 2019

View reviewed changes

joaocgreis added the semver-major PRs that contain breaking changes and should be released in the next major version. label Feb 5, 2019

refack force-pushed the msvs-speed-tweaks branch 3 times, most recently from 395d5a1 to cfbde88 Compare February 5, 2019 23:12

joaocgreis approved these changes Feb 13, 2019

View reviewed changes

joaocgreis added the tsc-review label Feb 13, 2019

joaocgreis added dont-land-on-v6.x and removed semver-major PRs that contain breaking changes and should be released in the next major version. labels Feb 14, 2019

refack force-pushed the msvs-speed-tweaks branch from cfbde88 to 86c985d Compare February 14, 2019 17:42

refack added 7 commits February 14, 2019 15:14

build: move optimizing link directives to node.exe target

20a917c

* ASCIIbetize directives * Merge duplicate directives from 'conditions' PR-URL: nodejs#25931 Reviewed-By: João Reis <[email protected]>

build,tools: add more headers to V8 PCH file

82b7989

* Use relative path for easy reuse in other projects PR-URL: nodejs#25931 Reviewed-By: João Reis <[email protected]>

build,deps: use PCH also for v8_initializers

cd5c7bf

* rename files to represent reuse PR-URL: nodejs#25931 Reviewed-By: João Reis <[email protected]>

build,deps: remove cygwin configuration which is not supported

4698757

PR-URL: nodejs#25931 Reviewed-By: João Reis <[email protected]>

build,src: add PCH to node.gypi

6ac80f0

PR-URL: nodejs#25931 Reviewed-By: João Reis <[email protected]>

build,win: always build with PCH

ff4adab

PR-URL: nodejs#25931 Reviewed-By: João Reis <[email protected]>

build,win: emit MSBuild summary

2ffd20b

PR-URL: nodejs#25931 Reviewed-By: João Reis <[email protected]>

refack force-pushed the msvs-speed-tweaks branch from 86c985d to 2ffd20b Compare February 14, 2019 20:15

refack merged commit 2ffd20b into nodejs:master Feb 14, 2019

refack deleted the msvs-speed-tweaks branch February 14, 2019 20:38

refack removed their assignment Mar 11, 2019

miniak mentioned this pull request Jul 11, 2019

Native node modules binaries targeting Electron 4+ are bloated electron/electron#18670

Closed

3 tasks

deepak1556 mentioned this pull request Oct 10, 2019

VC++ on v12+ builds bloated addon binaries. #29501

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build,win: configuration tweaks for faster build with MSVC #25931

build,win: configuration tweaks for faster build with MSVC #25931

refack commented Feb 4, 2019

nodejs-github-bot commented Feb 4, 2019

refack commented Feb 4, 2019

refack commented Feb 5, 2019

refack commented Feb 5, 2019

joaocgreis left a comment

joaocgreis Feb 5, 2019

refack Feb 5, 2019

joaocgreis commented Feb 5, 2019

refack commented Feb 6, 2019

gengjiawen commented Feb 6, 2019

refack commented Feb 6, 2019

joaocgreis left a comment

refack commented Feb 14, 2019

targos commented Feb 14, 2019

joaocgreis commented Feb 14, 2019

refack commented Feb 14, 2019

deepak1556 commented Oct 9, 2019 •

edited

Loading

Trott commented Oct 10, 2019

richardlau commented Oct 10, 2019

build,win: configuration tweaks for faster build with MSVC #25931

build,win: configuration tweaks for faster build with MSVC #25931

Conversation

refack commented Feb 4, 2019

Checklist

nodejs-github-bot commented Feb 4, 2019

refack commented Feb 4, 2019

refack commented Feb 5, 2019

refack commented Feb 5, 2019

joaocgreis left a comment

Choose a reason for hiding this comment

joaocgreis Feb 5, 2019

Choose a reason for hiding this comment

refack Feb 5, 2019

Choose a reason for hiding this comment

joaocgreis commented Feb 5, 2019

refack commented Feb 6, 2019

Very rough comparison

gengjiawen commented Feb 6, 2019

refack commented Feb 6, 2019

joaocgreis left a comment

Choose a reason for hiding this comment

refack commented Feb 14, 2019

targos commented Feb 14, 2019

joaocgreis commented Feb 14, 2019

refack commented Feb 14, 2019

deepak1556 commented Oct 9, 2019 • edited Loading

Trott commented Oct 10, 2019

richardlau commented Oct 10, 2019

deepak1556 commented Oct 9, 2019 •

edited

Loading