Bug 187254

Summary:

[CMake] Use JOB_POOLS to avoid memory-hungry linker processes running at the same time

Product:

WebKit

Reporter:

Adrian Perez <aperez>

Component:

Tools / Tests

Assignee:

Adrian Perez <aperez>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

achristensen, commit-queue, ews-watchlist, keith_miller, lforschler, mcatanzaro, sam, thorton, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

Other

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Archive of layout-test-results from ews200 for win-future	none
Patch	none

Description Adrian Perez 2018-07-02 09:44:42 PDT

Ninja supports the so called “job pools”, to which targets can be
assigned. A job pool allows limiting the amount of parallel jobs
that will run in parallel:

  https://ninja-build.org/manual.html#ref_pool

CMake supports configuring job pools, even when they are only used
by the Ninja build backend:

  https://cmake.org/cmake/help/v3.3/prop_gbl/JOB_POOLS.html

Using this we could limit the amount of parallel linker processes
which are run at the same time. This is particularly important to
avoid OOM situations when linking in non-release builds.

My proposal is doing something like the following in our CMake
build files:

  set_property(GLOBAL PROPERTY JOB_POOLS
               release_link_jobs=4
               debug_link_jobs=2)

  if (${CMAKE_BUILD_TYPE} STREQUAL "Release")
      set(CMAKE_JOB_POOL_LINK release_link_jobs)
  else ()
      set(CMAKE_JOB_POOL_LINK debug_link_jobs)
  endif ()

My tests in a build machine which hosts an EWS and that I use also
for development (so there are sometimes two debug builds running
in parallel) show that the above would avoid hitting OOM situations.

Opinions?

Comment 1 Michael Catanzaro 2018-07-02 09:46:06 PDT

OK

Comment 2 Adrian Perez 2018-07-02 10:06:48 PDT

Created attachment 344111 [details]
Patch

Comment 3 Adrian Perez 2018-07-02 10:08:15 PDT

FTR, I am running a couple of fresh builds to compare built
times with/without the patch before setting the cq? flag.

Comment 4 Adrian Perez 2018-07-02 10:09:56 PDT

Created attachment 344112 [details]
Patch

Fixed patch. Now really using 4/2 for release/debug builds.

Comment 5 Michael Catanzaro 2018-07-02 10:22:17 PDT

Comment on attachment 344112 [details]
Patch

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

> Source/cmake/WebKitCommon.cmake:66
> +    if (${CMAKE_BUILD_TYPE} STREQUAL "Release")

It fails for MinSizeRel, for example. But we can't test for everything, and when the test fails you pick the safe option, so it's good IMO.

Comment 6 Adrian Perez 2018-07-02 10:35:19 PDT

(In reply to Michael Catanzaro from comment #5)
> Comment on attachment 344112 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=344112&action=review
> 
> > Source/cmake/WebKitCommon.cmake:66
> > +    if (${CMAKE_BUILD_TYPE} STREQUAL "Release")
> 
> It fails for MinSizeRel, for example. But we can't test for everything, and
> when the test fails you pick the safe option, so it's good IMO.

Good catch, I'll add a check for MinSizeRel as well.

Comment 7 Adrian Perez 2018-07-02 13:59:03 PDT

Created attachment 344133 [details]
Patch

Handle also MinSizeRel builds.

The build times with and without the patch are roughly the same in my build
machine, so this seems quite safe to land.

Comment 8 Michael Catanzaro 2018-07-02 16:31:52 PDT

Comment on attachment 344133 [details]
Patch

Looks like the GTK EWS died:

ICECC[3965] 14:15:38: flush_writebuf() failed Resource temporarily unavailable
ICECC[3965] 14:15:40: write of source chunk to host 192.168.10.42
ICECC[3965] 14:15:40: failed  Resource temporarily unavailable
ICECC[3965] 14:15:40: got exception Error 15 - write to host failed (192.168.10.42)

Comment 9 EWS Watchlist 2018-07-02 16:39:23 PDT

Comment on attachment 344133 [details]
Patch

Attachment 344133 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8417088

New failing tests:
http/tests/security/local-video-source-from-remote.html

Comment 10 EWS Watchlist 2018-07-02 16:39:35 PDT

Created attachment 344145 [details]
Archive of layout-test-results from ews200 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews200  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

Comment 11 Adrian Perez 2018-07-02 17:04:53 PDT

(In reply to Michael Catanzaro from comment #8)
> Comment on attachment 344133 [details]
> Patch
> 
> Looks like the GTK EWS died:
> 
> ICECC[3965] 14:15:38: flush_writebuf() failed Resource temporarily
> unavailable
> ICECC[3965] 14:15:40: write of source chunk to host 192.168.10.42
> ICECC[3965] 14:15:40: failed  Resource temporarily unavailable
> ICECC[3965] 14:15:40: got exception Error 15 - write to host failed
> (192.168.10.42)

It seems like a temporary failure, I see other patches have passed
looking into page for this EWS bot:

  https://webkit-queues.webkit.org/queue-status/gtk-wk2-ews/bots/igalia4-gtk-wk2-ews

Comment 12 Adrian Perez 2018-07-02 17:06:28 PDT

Created attachment 344152 [details]
Patch

Re-uploading to make the GTK+ EWS go over this again

Comment 13 WebKit Commit Bot 2018-07-02 18:07:26 PDT

Comment on attachment 344152 [details]
Patch

Clearing flags on attachment: 344152

Committed r233454: <https://trac.webkit.org/changeset/233454>

Comment 14 WebKit Commit Bot 2018-07-02 18:07:28 PDT

All reviewed patches have been landed.  Closing bug.

Comment 15 Radar WebKit Bug Importer 2018-07-02 18:09:09 PDT

<rdar://problem/41750773>