Remove /MP from default additonal options

[skelliam]

Forcing this option means that the user-provided option MultiProcessorCompilation in msvs_settings (from, for example, binding.gyp) is useless. It also means that one cannot use #import in their source code or they will be faced with this error when trying to build:

error C2813: #import is not supported with /MP (compiling source file...)

Please see additional discussion of this here: nodejs/node-gyp#1087
and here: nodejs/node-gyp#26

[addaleax]

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

[refack]

/CC @nodejs/build-files

This makes sense to me. It's a low value optimization, and if it has negative effects (re #import) we can remove this.

[refack]

Still LGTM

[skelliam]

There is one more instance; can I add on to this commit?

[tniessen]

Will this make building Node.js itself even slower on Windows?

[skelliam]

@tniessen , if you look at the comment in nodejs/node-gyp#1087, it appears that Ninja already handles parallelization, and in fact it looks like there is some effort filter /MP out anyway.

[tniessen]

cc @seishun who worked on this flag before.

[skelliam]

Here is the documentation from MSDN regarding #import with /MP.

[refack]

FYI: GYP allows for removing items from list with the ! suffix, so the following msvs_settings block removes the /MP:

{
  'targets': [
    {
      'target_name': 'binding',
      'defines': [ 'V8_DEPRECATION_WARNINGS=1' ],
      'sources': [ 'binding.cc' ],
      'msvs_settings': {
        'VCCLCompilerTool': {
          'AdditionalOptions!': [
            '/MP'
          ]
        }
      }
    }
  ]
}

[refack]

Will this make building Node.js itself even slower on Windows?

Highly unlikely, since even when you use MSBuild it does multiprocess compilation by default, and we ask it to use at least two in vcbuild.bat:

node/vcbuild.bat

Lines 300 to 305 in 9f7efd5

	set "msbcpu=/m:2"
	if "%NUMBER_OF_PROCESSORS%"=="1" set "msbcpu=/m:1"
	set "msbplatform=Win32"
	if "%target_arch%"=="x64" set "msbplatform=x64"
	if "%target%"=="Build" if defined no_cctest set target=node
	msbuild node.sln %msbcpu% /t:%target% /p:Configuration=%config% /p:Platform=%msbplatform% /clp:NoSummary;NoItemAndPropertyList;Verbosity=minimal /nologo

P.S. /MP is probably incompatible with clcache

[skelliam]

Thanks @refack; was looking all over for a solution to this, sounds like your proposal would have worked. I did try to set the contents of AdditionalOptions to an empty string, but as mentioned in the linked discussion, this is an additive option.

[refack]

AdditionalOptions to an empty string, but as mentioned in the linked discussion, this is an additive option.

BTW for that GYP has the = suffix, so:

'msvs_settings': { 'VCCLCompilerTool': { 
  'AdditionalOptions=': []
} }

[targos]

@tniessen

Will this make building Node.js itself even slower on Windows?

Building Node.js on Windows is not as slow as it used to be. There were recent improvements.

[bzoz]

I'm -1. On my box this increases clean build time from 9 min 22 sec to 14 min 21 sec.

[seishun]

-1 if it slows down compilation as @bzoz described.

[richardlau]

Longer term the better solution is probably to implement nodejs/node-gyp#1118 and stop node-gyp using core's common.gypi.

[joaocgreis]

This will make compiling core much slower. I see CI hasn't yet been run for this PR, but it should be very noticeable there. If there's a way to make /MP active only for node core then this might be worth considering.

[skelliam]

Can't you still have /MP with the XML option "MultiProcessorCompilation" in msvs_settings? This way the user override is at least more intuitive (IMO).

Otherwise I like the idea from #1118.

Not until we find how to offset

[refack]

I see CI hasn't yet been run for this PR, but it should be very noticeable there.

I did:
Pre on test-rackspace-win2012r2-x64-12 — 35m
Post on test-rackspace-win2012r2-x64-11 — 50m
34% increase in time. But if this is an issue of higher parallelism, I have a feeling we could offset that with a higher number in

node/vcbuild.bat

Line 300 in 9f7efd5

set "msbcpu=/m:2"

[refack]

Can't you still have /MP with the XML option "MultiProcessorCompilation" in msvs_settings? This way the user override is at least more intuitive (IMO).

I think that makes sense.
👍

[skelliam]

Added 'MultiProcessorCompilation': 'true' (implicit /MP) instead of the hardcoded /MP. Users can override this with 'MultiProcessorCompilation': 'false' in their own binding.gyp.

[refack]

💯 Since this is essentially a no-op.

[richardlau]

Approving the change from /MP to 'MultiProcessorCompilation': 'true' provided build times remain unchanged.

Outdated. PTAL

[BridgeAR]

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

[tniessen]

The compilation time on Windows did not increase after the latest CI run.

[tniessen]

Landed in 56d9cd4.