build,win: re-enable ninja & multiple build optimizations #22698

refack · 2018-09-05T00:13:55Z

Checklist

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

nodejs-github-bot · 2018-09-05T00:13:56Z

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

vcbuild.bat

richardlau · 2018-09-05T01:11:16Z

common.gypi

@@ -269,23 +269,11 @@
    'msvs_settings': {
      'VCCLCompilerTool': {
        'StringPooling': 'true', # pool string literals
-        'DebugInformationFormat': 3, # Generate a PDB
+        'DebugInformationFormat': 1, # /Z7 embed info in .obj files


We currently publish the pdb, e.g. https://nodejs.org/dist/v10.9.0/win-x64/node_pdb.zip so this is semver-major?

cc @nodejs/platform-windows @nodejs/diagnostics

See also

node/vcbuild.bat

Lines 372 to 378 in 4cea01a

echo Creating node_pdb.7z

del node_pdb.7z > nul 2> nul

7z a -mx9 -t7z node_pdb.7z node.pdb > nul

echo Creating node_pdb.zip

del node_pdb.zip > nul 2> nul

7z a -mx9 -tzip node_pdb.zip node.pdb > nul

which is only run on the CI for releases (not for regular PR's).

Ohh. This generates a .pdb file but only a single one during linking.

joaocgreis · 2018-09-05T03:06:24Z

vcbuild.bat

-set "msbcpu=/m:2"
-if "%NUMBER_OF_PROCESSORS%"=="1" set "msbcpu=/m:1"
+set "msbcpu=/maxcpucount"
+if defined NUMBER_OF_PROCESSORS set "msbcpu=%msbcpu%:%NUMBER_OF_PROCESSORS%"


This will exhaust the memory available in some machines (including our CI). Ref 6cfedf0 and #12184 (comment) . I tested this extensively when I made this change, to find the best combination that uses a reasonable number of threads, and this was the best. CL will use one thread for each processor, but since some of our projects are quite small this helps a lot. If we did it the other way around, not limiting here and limiting in CL, V8 would keep compiling long after everything else finished.

On my 4 CPU box this does not affect compilation time.

I'll continue testing this, but meanwhile I'll remove this from this PR.

joaocgreis · 2018-09-05T03:19:39Z

node.gyp

+
+      # - "C4244: conversion from 'type1' to 'type2', possible loss of data"
+      #   Ususaly safe. Disable for `dep`, enable for `src`
+      'msvs_disabled_warnings!': [4244],


I didn't test yet, but this looks quite nice. Is it possible to simply set a much lower warning level for deps?

I've been thinking about that, the problem is common.gypi is used by node-gyp so I didn't want to make a big change without further testing.

This was needed in L482 as well

I didn't test yet, but this looks quite nice.

output is filled with:

d:\code\node\src\node_internals.h(321): warning C4244: 'argument': conversion from 'const uint64_t' to 'const NativeT', possible loss of data

refack · 2018-09-05T13:53:36Z

CI: https://ci.nodejs.org/job/node-test-commit-windows-fanned/20559/ ✔️

Fishrock123 · 2018-09-06T16:03:02Z

Could you describe this a bit more? Why are these required to re-enable ninja builds?

refack · 2018-09-06T19:21:53Z

Could you describe this a bit more? Why are these required to re-enable ninja builds?

'DebugInformationFormat': 1, # /Z7 embed info in .obj files
That's to enable clcache (https://github.com/frerich/clcache#how-clcache-works)
Merging the msvs_disabled_warnings, and adding 4244 to all targets, except node and
node_lib
removing the \lib\ prefix from /WHOLEARCHIVE:<(PRODUCT_DIR)\\lib\\:
1. It's unnecessary, WHOLEARCHIVE just needs the name of the library, not it's full path
2. when building with ninja (instead of MSBuild) the libraries are outputted to: <(PRODUCT_DIR)\\obj\\, so having a wrong full path breaks linking.

joaocgreis · 2018-09-08T00:43:21Z

This looks mostly good. @refack just two notes about the commit messages:

build,win: enable clcache: can you reword this to describe what it actually does? Something like build,win: generate a single PDB at link time and then mention clcache in the description.
build,win: unify warning exclusion: Focusing on the removed warning makes more sense to me here, something like: build,win: remove C4244 only for deps.

refack · 2018-09-11T18:25:33Z

https://ci.nodejs.org/job/node-test-commit-windows-fanned/20664/

@joaocgreis updated messages
@nodejs/platform-windows @nodejs/build-files PTAL

joaocgreis

Looks great, thanks @refack!

* Fixes ninja build PR-URL: nodejs#22698 Reviewed-By: João Reis <[email protected]>

* previusly set to generate a .pdb file for each .obj file * enables clcache PR-URL: nodejs#22698 Reviewed-By: João Reis <[email protected]>

* also unify warning exclusion directive PR-URL: nodejs#22698 Reviewed-By: João Reis <[email protected]>

refack · 2018-09-11T23:59:55Z

landed in
e85007f
7a10b86
54e76fb

* Fixes ninja build PR-URL: #22698 Reviewed-By: João Reis <[email protected]>

* previusly set to generate a .pdb file for each .obj file * enables clcache PR-URL: #22698 Reviewed-By: João Reis <[email protected]>

* also unify warning exclusion directive PR-URL: #22698 Reviewed-By: João Reis <[email protected]>

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 Sep 5, 2018

refack changed the title ~~build,win: multiple build optimizations~~ build,win: re-enable ninja & multiple build optimizations Sep 5, 2018

richardlau reviewed Sep 5, 2018

View reviewed changes

vcbuild.bat Outdated Show resolved Hide resolved

refack force-pushed the win-build-cleanup branch 2 times, most recently from a8638d0 to e705961 Compare September 5, 2018 00:37

richardlau reviewed Sep 5, 2018

View reviewed changes

joaocgreis reviewed Sep 5, 2018

View reviewed changes

refack force-pushed the win-build-cleanup branch 2 times, most recently from 209a88a to 84bed12 Compare September 5, 2018 13:45

refack force-pushed the win-build-cleanup branch from 84bed12 to 6c8cb91 Compare September 11, 2018 18:24

joaocgreis approved these changes Sep 11, 2018

View reviewed changes

BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 11, 2018

refack added 3 commits September 11, 2018 19:58

build,win: WHOLEARCHIVE needs just lib name

e85007f

* Fixes ninja build PR-URL: nodejs#22698 Reviewed-By: João Reis <[email protected]>

build,win: generate single PDB file per target

7a10b86

* previusly set to generate a .pdb file for each .obj file * enables clcache PR-URL: nodejs#22698 Reviewed-By: João Reis <[email protected]>

build,win: exclude warning 4244 only for deps

54e76fb

* also unify warning exclusion directive PR-URL: nodejs#22698 Reviewed-By: João Reis <[email protected]>

refack force-pushed the win-build-cleanup branch from 6c8cb91 to 54e76fb Compare September 11, 2018 23:59

refack merged commit 54e76fb into nodejs:master Sep 12, 2018

refack deleted the win-build-cleanup branch September 12, 2018 00:01

targos pushed a commit that referenced this pull request Sep 12, 2018

build,win: WHOLEARCHIVE needs just lib name

b3bc642

* Fixes ninja build PR-URL: #22698 Reviewed-By: João Reis <[email protected]>

targos pushed a commit that referenced this pull request Sep 12, 2018

build,win: generate single PDB file per target

dbe27d7

* previusly set to generate a .pdb file for each .obj file * enables clcache PR-URL: #22698 Reviewed-By: João Reis <[email protected]>

targos pushed a commit that referenced this pull request Sep 12, 2018

build,win: exclude warning 4244 only for deps

b4a79ac

* also unify warning exclusion directive PR-URL: #22698 Reviewed-By: João Reis <[email protected]>

targos mentioned this pull request Sep 18, 2018

Release proposal: v10.11.0 #22932

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build,win: re-enable ninja & multiple build optimizations #22698

build,win: re-enable ninja & multiple build optimizations #22698

refack commented Sep 5, 2018

nodejs-github-bot commented Sep 5, 2018

richardlau Sep 5, 2018

richardlau Sep 5, 2018 •

edited

Loading

refack Sep 5, 2018

joaocgreis Sep 5, 2018

bzoz Sep 5, 2018

refack Sep 5, 2018

joaocgreis Sep 5, 2018

refack Sep 5, 2018

refack Sep 5, 2018

refack Sep 5, 2018

refack commented Sep 5, 2018 •

edited

Loading

Fishrock123 commented Sep 6, 2018

refack commented Sep 6, 2018

joaocgreis commented Sep 8, 2018

refack commented Sep 11, 2018 •

edited

Loading

joaocgreis left a comment

refack commented Sep 11, 2018

	echo Creating node_pdb.7z
	del node_pdb.7z > nul 2> nul
	7z a -mx9 -t7z node_pdb.7z node.pdb > nul

	echo Creating node_pdb.zip
	del node_pdb.zip > nul 2> nul
	7z a -mx9 -tzip node_pdb.zip node.pdb > nul

build,win: re-enable ninja & multiple build optimizations #22698

build,win: re-enable ninja & multiple build optimizations #22698

Conversation

refack commented Sep 5, 2018

Checklist

nodejs-github-bot commented Sep 5, 2018

Choose a reason for hiding this comment

richardlau Sep 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

refack commented Sep 5, 2018 • edited Loading

Fishrock123 commented Sep 6, 2018

refack commented Sep 6, 2018

joaocgreis commented Sep 8, 2018

refack commented Sep 11, 2018 • edited Loading

joaocgreis left a comment

Choose a reason for hiding this comment

refack commented Sep 11, 2018

richardlau Sep 5, 2018 •

edited

Loading

refack commented Sep 5, 2018 •

edited

Loading

refack commented Sep 11, 2018 •

edited

Loading