New issue
Advanced search Search tips

Issue 883727 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Sep 22
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Support jumbo compilation in //headless

Project Member Reported by brat...@opera.com, Sep 13

Issue description

//headless uses 1-2% of the compilation time in jumbo builds. By having it support jumbo that time should mostly go away.
 
15.3 CPU minutes, out of 712.3 CPU minutes total, for a share of 2.1%. I think it can be cut to about 1-2 CPU minutes but we'll see when it's done.
Owner: brat...@opera.com
Status: Started (was: Available)
Project Member

Comment 3 by bugdroid1@chromium.org, Sep 14

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/777cd8c8dc490b06d8d0334120265b54842f1c35

commit 777cd8c8dc490b06d8d0334120265b54842f1c35
Author: Daniel Bratell <bratell@opera.com>
Date: Fri Sep 14 13:34:58 2018

[headless] Specify namespace in cases where it might be ambiguous

There are sub-namespaces to ::headless named storage and
switches and those will be chosen by the compiler instead of
::storage or ::switches if the compiler knows about them.

This problem appeared in jumbo builds where the compiler knew
more about all the namespaces. Fix is to either rename/remove
the sub-namespaces or to do what this patch does, prefix
storage with :: (so ::storage and ::switches) to clarify what
namespace is intended.

Bug:  883727 
Change-Id: I1f6e428926f0a959787c2205f92505280d96b831
Reviewed-on: https://chromium-review.googlesource.com/1225792
Reviewed-by: Sami Kyöstilä <skyostil@chromium.org>
Commit-Queue: Daniel Bratell <bratell@opera.com>
Cr-Commit-Position: refs/heads/master@{#591336}
[modify] https://crrev.com/777cd8c8dc490b06d8d0334120265b54842f1c35/headless/lib/browser/headless_browser_context_impl.h
[modify] https://crrev.com/777cd8c8dc490b06d8d0334120265b54842f1c35/headless/lib/browser/headless_content_browser_client.cc
[modify] https://crrev.com/777cd8c8dc490b06d8d0334120265b54842f1c35/headless/lib/browser/headless_content_browser_client.h
[modify] https://crrev.com/777cd8c8dc490b06d8d0334120265b54842f1c35/headless/lib/headless_content_main_delegate.cc
[modify] https://crrev.com/777cd8c8dc490b06d8d0334120265b54842f1c35/headless/lib/headless_crash_reporter_client.cc

Project Member

Comment 4 by bugdroid1@chromium.org, Sep 17

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e3084d058bb609b850656b79760a293f5ccb3d0d

commit e3084d058bb609b850656b79760a293f5ccb3d0d
Author: Daniel Bratell <bratell@opera.com>
Date: Mon Sep 17 13:38:29 2018

Reorganize headless printing support slightly

Including rendererer only code in the library "headless" that will be
used in the browser process only works if that code is dead and is
packaged in an unused object file inside a static library and linked
with a linker that ignores such object files. That is why including
"lib/renderer/headless_print_render_frame_helper_delegate.cc" kind of
worked but in jumbo build experiments it didn't.

This patch moves the renderer only printing code to libheadless.so
build code and to headless_renderer code.

Bug:  883727 
Change-Id: I2ff6edfbf8c1cd79c84973933dd34a9de8fd3632
Reviewed-on: https://chromium-review.googlesource.com/1227937
Reviewed-by: Sami Kyöstilä <skyostil@chromium.org>
Commit-Queue: Daniel Bratell <bratell@opera.com>
Cr-Commit-Position: refs/heads/master@{#591659}
[modify] https://crrev.com/e3084d058bb609b850656b79760a293f5ccb3d0d/headless/BUILD.gn

Project Member

Comment 5 by bugdroid1@chromium.org, Sep 17

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/39f843d0ad1c76a258a0772b80c49177eb0a7b4d

commit 39f843d0ad1c76a258a0772b80c49177eb0a7b4d
Author: Daniel Bratell <bratell@opera.com>
Date: Mon Sep 17 14:17:19 2018

One copy of headless shell switches should be enough

Many different parts of headless use the headless command
line switches, and compiled them into their lib/binary. This
caused build failures due to duplicate symbols in jumbo
build experiment. It seems the code depends on the dead object
file removal feature in the linker. Better is to just compile
and link the code once and export it from there.

Bug:  883727 
Change-Id: I47ff0c86e56f2f86fbdaef6f5f7589ac82d16c66
Reviewed-on: https://chromium-review.googlesource.com/1225761
Reviewed-by: Sami Kyöstilä <skyostil@chromium.org>
Commit-Queue: Daniel Bratell <bratell@opera.com>
Cr-Commit-Position: refs/heads/master@{#591663}
[modify] https://crrev.com/39f843d0ad1c76a258a0772b80c49177eb0a7b4d/headless/BUILD.gn
[modify] https://crrev.com/39f843d0ad1c76a258a0772b80c49177eb0a7b4d/headless/app/headless_shell_switches.h

Project Member

Comment 6 by bugdroid1@chromium.org, Sep 17

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/08c20e46e1a7a80e141ac46bd3b5937954f6eeff

commit 08c20e46e1a7a80e141ac46bd3b5937954f6eeff
Author: Daniel Bratell <bratell@opera.com>
Date: Mon Sep 17 15:34:16 2018

[headless] Avoid compiling files that will not be used

In component builds headless_shell_lib compiled
lib/browser/headless_content_browser_client.cc which referred to
a lot of other code that was not compiled. Thanks to a linker
feature/quirk, the whole object file was ignored and nothing
bad happened. In jumbo build experiments the linker couldn't
do dead object file removal and the result was a lot of linkage
errors.

Bug:  883727 
Change-Id: I3ef9f69cbf61ddccdb55d0929963ad33a6fb442b
Reviewed-on: https://chromium-review.googlesource.com/1226615
Commit-Queue: Daniel Bratell <bratell@opera.com>
Reviewed-by: Sami Kyöstilä <skyostil@chromium.org>
Cr-Commit-Position: refs/heads/master@{#591683}
[modify] https://crrev.com/08c20e46e1a7a80e141ac46bd3b5937954f6eeff/headless/BUILD.gn

Project Member

Comment 7 by bugdroid1@chromium.org, Sep 21

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/93ca28932e6ae3040537002ee8d1e64bb9aeb68f

commit 93ca28932e6ae3040537002ee8d1e64bb9aeb68f
Author: Daniel Bratell <bratell@opera.com>
Date: Fri Sep 21 07:38:50 2018

Retire ToValueImpl and rely on ToValue for headless devtools

The previous ToValue -> ToValueImpl implementation caused
template resolution complexities and prevented jumbo support
by having a clone of the same template in every domain_types_cc
file.

Bug:  883727 
Change-Id: Iec49d7bde93515fe180f26073cea59db3cf4be82
Reviewed-on: https://chromium-review.googlesource.com/1236355
Reviewed-by: Sami Kyöstilä <skyostil@chromium.org>
Reviewed-by: Andrey Kosyakov <caseq@chromium.org>
Commit-Queue: Daniel Bratell <bratell@opera.com>
Cr-Commit-Position: refs/heads/master@{#593104}
[modify] https://crrev.com/93ca28932e6ae3040537002ee8d1e64bb9aeb68f/headless/lib/browser/devtools_api/domain_type_conversions_h.template
[modify] https://crrev.com/93ca28932e6ae3040537002ee8d1e64bb9aeb68f/headless/lib/browser/devtools_api/domain_types_cc.template
[modify] https://crrev.com/93ca28932e6ae3040537002ee8d1e64bb9aeb68f/headless/public/internal/value_conversions.h

Project Member

Comment 8 by bugdroid1@chromium.org, Sep 21

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6428dc343fa4ef2898ac9005e0976b59eabda203

commit 6428dc343fa4ef2898ac9005e0976b59eabda203
Author: Daniel Bratell <bratell@opera.com>
Date: Fri Sep 21 16:33:49 2018

Support jumbo in //headless (-12 CPU minutes)

Jumbo is a unity build system for Chromium where many files are
compiled together. Since so much code is in headers and templates
this is much more efficient, and depending on your hardware, can
significantly speed up builds.

The headless module needs 26 CPU minutes to compile without jumbo.
With jumbo for everything, it needs 3.1 CPU minutes. Half of that
is already on master through the jumbo support in mojo. This patch
adds jumbo support for the rest, changing the compilation effort
from 15 CPU minutes to 3.

In real world time, this saves about 1.5 minutes on a full build on an
8 thread machine.

Bug:  883727 
Change-Id: I3ede4805064344574c314d03904208110fdcb462
Reviewed-on: https://chromium-review.googlesource.com/1225702
Reviewed-by: Andrey Kosyakov <caseq@chromium.org>
Commit-Queue: Daniel Bratell <bratell@opera.com>
Cr-Commit-Position: refs/heads/master@{#593216}
[modify] https://crrev.com/6428dc343fa4ef2898ac9005e0976b59eabda203/headless/BUILD.gn

Status: Fixed (was: Started)

Sign in to add a comment