Fix building on Windows x86

[joaocgreis]

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.

[cjihrig]

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?

[jasnell]

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.

[joaocgreis]

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.

[gibfahn]

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.

[cjihrig]

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

[bnoordhuis]

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

[joaocgreis]

@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.

[joaocgreis]

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?

[joaocgreis]

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/

[joaocgreis]

@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/).

[Trott]

LGTM if CI is green

Whoops, wrong tab.

[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?

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': [

[refack]

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

[refack]

Nit++

[refack]

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

[refack]

Builds on my machine. vcbuild x86 test passes. 👍

[gibfahn]

Builds on my machine. vcbuild x86 test passes. 👍

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

[refack]

Could you check @bnoordhuis's patch?

👎 makes only 4 shards...

[bnoordhuis]

LGTM then.

[refack]

LGTM then.

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