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

Fix building on Windows x86 #12184

Closed

Conversation

joaocgreis
Copy link
Member

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.

@joaocgreis 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 nodejs-github-bot added the v8 engine Issues and PRs related to the V8 dependency. label Apr 3, 2017
@cjihrig
Copy link
Contributor

cjihrig commented Apr 3, 2017

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
Copy link
Member

jasnell commented Apr 3, 2017

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
Copy link
Member Author

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.

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

gibfahn commented Apr 3, 2017

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
Copy link
Contributor

cjihrig commented Apr 3, 2017

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

@bnoordhuis
Copy link
Member

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

@joaocgreis
Copy link
Member Author

@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 joaocgreis force-pushed the joaocgreis-H42-msvs_shard branch 2 times, most recently from e830fd0 to 431c314 Compare April 4, 2017 20:05
@joaocgreis
Copy link
Member Author

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
Copy link
Member Author

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

Temporary jobs that will be merged when this lands:

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

joaocgreis commented Apr 8, 2017

@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
Trott previously approved these changes Apr 8, 2017
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

LGTM if CI is green

@Trott Trott dismissed their stale review April 8, 2017 16:12

Whoops, wrong tab.

@bnoordhuis
Copy link
Member

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

vcbuild.bat Outdated
@@ -190,9 +190,11 @@ echo Project files generated.
if defined nobuild goto sign

@rem Build the sln with msbuild.
set "msbcpu=/m:2"
if "%NUMBER_OF_PROCESSORS%"=="1" set "msbcpu="
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Nit++

Copy link
Contributor

Choose a reason for hiding this comment

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

@refack
Copy link
Contributor

refack commented Apr 9, 2017

Builds on my machine. vcbuild x86 test passes. 👍

@gibfahn
Copy link
Member

gibfahn commented Apr 9, 2017

Builds on my machine. vcbuild x86 test passes. 👍

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

@refack
Copy link
Contributor

refack commented Apr 9, 2017

Could you check @bnoordhuis's patch?

👎 makes only 4 shards...

Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

LGTM then.

@refack
Copy link
Contributor

refack commented Apr 9, 2017

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>
nodejs-ci pushed a commit to nodejs/node-v8 that referenced this pull request Jun 17, 2017
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/node#12184
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
nodejs-ci pushed a commit to nodejs/node-v8 that referenced this pull request Jun 18, 2017
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/node#12184
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
nodejs-ci pushed a commit to nodejs/node-v8 that referenced this pull request Jun 19, 2017
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/node#12184
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
nodejs-ci pushed a commit to nodejs/node-v8 that referenced this pull request Jun 20, 2017
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/node#12184
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
nodejs-ci pushed a commit to nodejs/node-v8 that referenced this pull request Jun 21, 2017
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/node#12184
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
nodejs-ci pushed a commit to nodejs/node-v8 that referenced this pull request Jun 22, 2017
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/node#12184
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
nodejs-ci pushed a commit to nodejs/node-v8 that referenced this pull request Jun 23, 2017
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/node#12184
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
targos pushed a commit that referenced this pull request Jun 23, 2017
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: #12184
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
nodejs-ci pushed a commit to nodejs/node-v8 that referenced this pull request Jun 23, 2017
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/node#12184
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
nodejs-ci pushed a commit to nodejs/node-v8 that referenced this pull request Jun 24, 2017
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/node#12184
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
nodejs-ci pushed a commit to nodejs/node-v8 that referenced this pull request Jun 25, 2017
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/node#12184
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
nodejs-ci pushed a commit to nodejs/node-v8 that referenced this pull request Jun 26, 2017
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/node#12184
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
targos pushed a commit that referenced this pull request Jun 26, 2017
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: #12184
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
targos pushed a commit to nodejs/node-v8 that referenced this pull request Jun 26, 2017
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/node#12184
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
nodejs-ci pushed a commit to nodejs/node-v8 that referenced this pull request Jun 26, 2017
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/node#12184
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
targos pushed a commit that referenced this pull request Jun 27, 2017
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: #12184
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
nodejs-ci pushed a commit to nodejs/node-v8 that referenced this pull request Jun 27, 2017
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/node#12184
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
nodejs-ci pushed a commit to nodejs/node-v8 that referenced this pull request Jun 27, 2017
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/node#12184
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
targos pushed a commit that referenced this pull request Jun 28, 2017
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: #12184
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
nodejs-ci pushed a commit to nodejs/node-v8 that referenced this pull request Jun 28, 2017
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/node#12184
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
nodejs-ci pushed a commit to nodejs/node-v8 that referenced this pull request Jun 29, 2017
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/node#12184
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
nodejs-ci pushed a commit to nodejs/node-v8 that referenced this pull request Jun 30, 2017
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/node#12184
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
duqingnian pushed a commit to duqingnian/gyp that referenced this pull request Apr 26, 2024
Patch Set 1:

> Patch Set 1:
> 
> > Patch Set 1:
> > 
> > > Patch Set 1:
> > > 
> > > > P.S. Mark, what is the importance?
> > > 
> > > The same as it is here: https://chromium.googlesource.com/external/gyp/+/e8850240a433259052705fb8c56e51795b7dc9c3/pylib/gyp/MSVSVersion.py#107. Not doing this for 2017 is effectively a regression from what we did for 2013 and 2015.
> > > 
> > > Why wouldn’t you choose the native 64-bit executable, larger address space and all, if it’s available to you? The fact that you’re targeting 32-bit should have absolutely no bearing on the decision.
> > 
> > So there's no "real" importance
> 
> I wouldn’t say that at all. We have, in the past, found that the larger address space available to a 64-bit process meant the difference between being able to build a large chunk of code under heavy optimization and not.

Yeah, we've seen that in nodejs/node#12184

Patch-set: 1
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. v8 engine Issues and PRs related to the V8 dependency. windows Issues and PRs related to the Windows platform.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants