Fix building on Windows x86 by joaocgreis · Pull Request #12184 · nodejs/node · GitHub

Building on Windows x86 has been broken on master since the update of V8 to 5.7 (#11752, nodejs/v8#4, nodejs/build#669). This was not detected at the time because the CI matrix does not include a 32 bit build (which I plan to add after this lands).

When building V8 with Gyp, the library v8_base is divided into several shards because its size exceeds the limit. For each shard, a single cl.exe invocation is used to compile all .cc files into .obj files. In this invocation, some functions from runtime.cc are not getting compiled into runtime.obj. Using cl.exe with all the same arguments but to compile only runtime.cc compiles all functions as expected. I was not yet able to determine what set of files interferes, but none of the other files compiled alone with runtime.cc causes the problem. This seems to happens only when a big set of files (>110) is compiled simultaneously.

This might have passed unnoticed in V8 upstream because ninja compiles every source file one at a time.

This PR increases the number of shards to divide v8_base into. This increases the number of calls to cl.exe but decreases the number of files compiled each time.

Fixes: nodejs/v8#4

cc @nodejs/v8 @jasnell

Checklist

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

Affected core subsystem(s)

Deps, V8, build, Windows.

This was not detected at the time because the CI matrix does not include a 32 bit build (which I plan to add after this lands).

Do you have a machine setup and ready to add, that you've tested this on?

hmm... I don't have a 32 bit machine to test on. I can get a VM set up later on but won't have time for a few days.

I've tested a 32 bit build locally. I can create a temporary job to test a 32 bit build in CI.

There seem to be issues with Visual C++ Build Tools, I'll look into it.

Do you have a machine setup and ready to add, that you've tested this on?

Do you mean a 32-bit machine, or running the 32-bit build on a 64-bit machine? If the latter, then we can use the existing machines.

Node 8 is currently failing for me on a 64-bit Windows machine (with vcbuild.bat x86). Adding this patch fixes it.

I mean whatever is required to verify that this PR does what it says it does.

Would setting msvs_shard in common.gypi work? That would save us one floating patch.

@gibfahn I mean building and running the 32-bit exe on a 64-bit machine. We currently have no way of getting a real 32-bit machine in CI.

The problem with Visual C++ Build Tools was over-parallelization. MSBuild is invoked with /m and cl.exe is invoked with /MP. In our CI machines this means a maximum of 64 simultaneous processes, exhausting the available memory (error C1060). Added a commit to limit MSBuild to /m:2. Also tested with removing /m and /MP completely, and also /MP2. This seems to be the best combination, in my tests the build time is increased only 20 seconds.

@bnoordhuis I did not find a way to set msvs_shard in common.gypi. It can be done for all libraries as a default, but gets overridden for v8_base. Do you know of a way to specify it only for v8_base from common.gypi?

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

Temporary jobs that will be merged when this lands:

Compile: https://ci.nodejs.org/view/All/job/reis-compile-windows/1/ - this shows that this PR fixes compilation of x86 on 64-bit machines (as we do in the release servers)
Test: https://ci.nodejs.org/view/All/job/reis-test-binary-windows/2/

@nodejs/v8 @nodejs/build @nodejs/platform-windows

The over-parallelization error just happened testing another pull request (https://ci.nodejs.org/job/node-compile-windows/8076/label=win-vs2015/).

LGTM if CI is green

Whoops, wrong tab.

I did not find a way to set msvs_shard in common.gypi. It can be done for all libraries as a default, but gets overridden for v8_base. Do you know of a way to specify it only for v8_base from common.gypi?

Sorry João, missed your comment. The patch below may do the trick; the shard level is inherited by dependencies if I read MSVSUtil.py right. Is there an easy way for me to test?

diff --git a/node.gypi b/node.gypi
index d78d24d..6321131 100644
--- a/node.gypi
+++ b/node.gypi
@@ -25,6 +25,7 @@
         'deps/v8/src/v8.gyp:v8',
         'deps/v8/src/v8.gyp:v8_libplatform'
       ],
+      'msvs_shard': 10,
     }],
     [ 'node_use_v8_platform=="true"', {
       'defines': [

Could you set /m:1 It make the log output consistent with /m:n >1

Nit++

Ref: http://stackoverflow.com/questions/27794610/msbuild-ignores-its-own-showtimestamp-parameter

Builds on my machine. vcbuild x86 test passes. 👍

Builds on my machine. vcbuild x86 test passes. 👍

@refack is that with this PR as is? Could you check @bnoordhuis's patch?

Could you check @bnoordhuis's patch?

👎 makes only 4 shards...

LGTM then.

LGTM then.

I think it is inherited, but then overridden by deps/v8/src/v8.gyp:1744

MSBuild invokes cl.exe with /MP (set in common.gypi), making it compile sources in parallel using a number of internal processes equal to the number of effective processors. MSBuild /m uses a similar mechanism, so the number of compiler processes can grow to the number of effective processors squared. This limits MSBuild to 2 processes, to still use some parallelization while requiring less memory. Cl.exe is still invoked with /MP, thus the maximum number of processes is limited to twice the number of effective processors. PR-URL: nodejs#12184 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

Increase the number of shards to divide v8_base into. This increases the number of calls to cl.exe but decreases the number of files compiled each time. Fixes: nodejs/v8#4 PR-URL: nodejs#12184 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

I'm assuming that nothing from here needs to be backported to v6.x as it runs on V8 5.1. If I am mistaken please let me know

@MylesBorins since @joaocgreis added an x86 build to the CI matrix, if this job passes https://ci.nodejs.org/job/node-compile-windows/9040/label=win-vs2015-x86/ (building refs/heads/v7.x-staging), there's no need for this PR.

wouldn't it be v6.x-staging that needs to pass?

🤦‍♂️ yes, obv: https://ci.nodejs.org/job/node-compile-windows/9054/

joaocgreis added build Issues and PRs related to build files or the CI. v8 engine Issues and PRs related to the V8 dependency. windows Issues and PRs related to the Windows platform. labels Apr 3, 2017

nodejs-github-bot added the v8 engine Issues and PRs related to the V8 dependency. label Apr 3, 2017

joaocgreis added the wip Issues and PRs that are still a work in progress. label Apr 3, 2017

joaocgreis force-pushed the joaocgreis-H42-msvs_shard branch 2 times, most recently from e830fd0 to 431c314 Compare April 4, 2017 20:05

joaocgreis removed the wip Issues and PRs that are still a work in progress. label Apr 4, 2017

Trott previously approved these changes Apr 8, 2017

View reviewed changes

refack approved these changes Apr 8, 2017

View reviewed changes

bnoordhuis approved these changes Apr 9, 2017

View reviewed changes

joaocgreis added 2 commits April 10, 2017 16:03

joaocgreis force-pushed the joaocgreis-H42-msvs_shard branch from 431c314 to d50bb93 Compare April 10, 2017 15:06

MylesBorins added the dont-land-on-v6.x label May 15, 2017

refack mentioned this pull request May 30, 2017

how to include v8dbg_ symbols in the windows build #5839

Closed

jaimecbernardo mentioned this pull request Jun 28, 2017

deps,win: set MSVS .obj folders in gyp for V8 #13959

Closed

2 tasks

joaocgreis mentioned this pull request Sep 5, 2018

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

Merged

2 tasks

Fix building on Windows x86 by joaocgreis · Pull Request #12184 · nodejs/node · GitHub

Uh oh!

Uh oh!

Fix building on Windows x86 - #12184

Fix building on Windows x86#12184joaocgreis wants to merge 2 commits intonodejs:masternodejs/node:masterfrom JaneaSystems:joaocgreis-H42-msvs_shardJaneaSystems/node:joaocgreis-H42-msvs_shardCopy head branch name to clipboard

Conversation

joaocgreis commented Apr 3, 2017

Checklist

Affected core subsystem(s)

Uh oh!

cjihrig commented Apr 3, 2017

Uh oh!

jasnell commented Apr 3, 2017

Uh oh!

joaocgreis commented Apr 3, 2017

Uh oh!

gibfahn commented Apr 3, 2017

Uh oh!

cjihrig commented Apr 3, 2017

Uh oh!

bnoordhuis commented Apr 3, 2017

Uh oh!

joaocgreis commented Apr 3, 2017

Uh oh!

joaocgreis commented Apr 4, 2017

Uh oh!

joaocgreis commented Apr 4, 2017

Uh oh!

joaocgreis commented Apr 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Trott left a comment

Choose a reason for hiding this comment

Uh oh!

bnoordhuis commented Apr 8, 2017

Uh oh!

refack Apr 8, 2017

Choose a reason for hiding this comment

Uh oh!

refack Apr 8, 2017

Choose a reason for hiding this comment

Uh oh!

refack Apr 8, 2017

Choose a reason for hiding this comment

Uh oh!

refack commented Apr 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gibfahn commented Apr 9, 2017

Uh oh!

refack commented Apr 9, 2017

Uh oh!

bnoordhuis left a comment

Choose a reason for hiding this comment

Uh oh!

refack commented Apr 9, 2017

Uh oh!

MylesBorins commented May 15, 2017

Uh oh!

refack commented May 15, 2017

Uh oh!

MylesBorins commented May 15, 2017

Uh oh!

refack commented May 15, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

Fix building on Windows x86#12184
joaocgreis wants to merge 2 commits into
nodejs:masterfrom
JaneaSystems:joaocgreis-H42-msvs_shard

joaocgreis commented Apr 8, 2017 •
edited
Loading

refack commented Apr 9, 2017 •
edited
Loading