Bug 177057 – build-webkit spawns fewer subprocesses than ninja uses by default

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Tim Horton]

build-webkit spawns fewer subprocesses than ninja uses by default

[Tim Horton]

Created attachment 321054 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 321054 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=321054&action=review

> Tools/Scripts/build-webkit:245
> +        # so don't override the number of jobs if building with Ninja.

Would this number be better when not building with ninja, too? The comment should at least explain why (e.g. “I didn’t test, and didn’t want to make the change blindly”).

[Tim Horton]

Comment on attachment 321054 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=321054&action=review

>> Tools/Scripts/build-webkit:245
>> +        # so don't override the number of jobs if building with Ninja.
> 
> Would this number be better when not building with ninja, too? The comment should at least explain why (e.g. “I didn’t test, and didn’t want to make the change blindly”).

I considered it, and would happily make the change blindly, but I’m also not sure anybody cares about those builds (it won’t affect Xcode, for example — just non-Windows non-Ninja CMake builds).

[Alex Christensen]

Comment on attachment 321054 [details]
Patch

We should only make this change for ninja, which automatically determines the number of jobs to do at the same time.  If we do this for non-ninja builds, it would make them take ~8x as long because make defaults to using 1 cpu.

[Tim Horton]

I think (at least I assume) ap meant adding the “+2” to the make invocation

[Alexey Proskuryakov]

Correct.

[WebKit Commit Bot]

Comment on attachment 321054 [details]
Patch

Clearing flags on attachment: 321054

Committed r222160: <http://trac.webkit.org/changeset/222160>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Konstantin Tokarev]

Comment on attachment 321054 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=321054&action=review

> Tools/Scripts/build-webkit:242
>  if (isCMakeBuild() && !isAnyWindows()) {
> +    if (!canUseNinja()) {

This condition is wrong, $willUseNinja in generateBuildSystemFromCMakeProject() determines what cmake generator is used, and it's defined as canUseNinja() && canUseNinjaGenerator()

[Konstantin Tokarev]

Though canUseNinjaGenerator can be eleminated, as all required cmake versions should support -G Ninja

[Tim Horton]

(In reply to Konstantin Tokarev from comment #10)
> Though canUseNinjaGenerator can be eleminated, as all required cmake
> versions should support -G Ninja

Doesn't that then make the patch right? :P

[Konstantin Tokarev]

Yes, it does. CMake enables Ninja generator on all supported platforms since  v2.8.9 (https://github.com/Kitware/CMake/commit/5d365b26ec6ce089f1a5e0bfed523cb5f916f1da) and we require v3.3

[Tim Horton]

Nice

[Tim Horton]

Got rid of it in bug 177330

[Konstantin Tokarev]

Thanks!

[Carlos Alberto Lopez Perez]

(In reply to WebKit Commit Bot from comment #7)
> Comment on attachment 321054 [details]
> Patch
> 
> Clearing flags on attachment: 321054
> 
> Committed r222160: <http://trac.webkit.org/changeset/222160>

This patch has broken our bots.

We need to control the number of build parallel process there as we share the machine between several bots (each one run in a container). And if we allow all of them to use all the available cores bad things happen. See bug 177223


I think that when the environment variable NUMBER_OF_PROCESSORS is defined it  should be respected (both for ninja and make). I will try to upload a patch to bug 177223 ASAP

[Radar WebKit Bug Importer]

<rdar://problem/34693285>