wrapper for web_contents_impl CLs

[John Abd-El-Malek]

Hi Takuto,

First, thank you for working on improving the build speed; this is always much appreciated.

A few content owners and I saw https://crbug.com/322102823 and we had a few questions. The motivation is to learn more about the intended improvement, and whether there are other ways that are less of a maintenance burden.

-how much does this speed up the build? We ask because we generally prefer to trade off computing time than developer time, unless the former is non-trivial.

-do we know why including render_frame_host_impl.h is so slow? It's included by over 300 files so this could be hard to really decrease at scale, and not (re)introduce (even transitively) often which would make other cc files get bigger.

Thank you

[Takuto Ikuta]

Hi John,

On Thu, Feb 8, 2024 at 1:57 AM John Abd-El-Malek <j...@chromium.org> wrote:

Hi Takuto,

First, thank you for working on improving the build speed; this is always much appreciated.

A few content owners and I saw https://crbug.com/322102823 and we had a few questions. The motivation is to learn more about the intended improvement, and whether there are other ways that are less of a maintenance burden.

-how much does this speed up the build? We ask because we generally prefer to trade off computing time than developer time, unless the former is non-trivial.

Currently, browser_interface_binders.cc is large enough that our remote worker cannot compile and reclient builds always see local fallback for that compile. (http://b/289968566)

So main focus on the issue is browser_interface_binders.cc instead of web_contents_impl.h/render_frame_host_impl.h.

Although browser_interface_binders.cc is shown as the largest time consuming target in go/chrome-build-stats, getting clear improvement for most builds only by fixing this file is hard.

So I'd like to reduce the inclusion of large headers from other time consuming targets too to reduce critical paths on build trace.

 

-do we know why including render_frame_host_impl.h is so slow? It's included by over 300 files so this could be hard to really decrease at scale, and not (re)introduce (even transitively) often which would make other cc files get bigger.

I think that is because the header also includes other large headers.

https://commondatastorage.googleapis.com/chromium-browser-clang/chrome_includes_2024-02-08_120812.html#view=edges&filter=%5Econtent%2Fbrowser%2Frenderer_host%2Frender_frame_host_impl%5C.h%24&sort=asize&reverse=&includer=%5Econtent%2Fbrowser%2Frenderer_host%2Frender_frame_host_impl%5C.h%24&included=&limit=1000

I think we can improve build time by focusing compiles which appear in critical paths more than other targets, but that doesn't need to remove the header include from every file.

 

Thank you

[John Abd-El-Malek]

This is a great resource, I've never seen that before (I've wanted to see something like this for a long time!). This is very helpful in figuring out what's expensive, and then we can find ways of more naturally reducing header bloat. I can take care of storage_parititon_impl.h which is the biggest include in render_frame_impl.h.

In the meantime can we revert the other changes?

For future problems like this, feel free to email content-owners@ if it's something specific inside content, or other top level owners of other directories when a file is too big to build.

Is this chrome_includes site widely advertised and I just missed it? If not, maybe it's something we can share more widely on chromium-dev so that devs have visibility into expensive headers?

 

Thank you

[Takuto Ikuta]

On Fri, Feb 9, 2024 at 4:22 AM John Abd-El-Malek <j...@chromium.org> wrote:

On Thu, Feb 8, 2024 at 6:31 AM Takuto Ikuta <tik...@chromium.org> wrote:

Hi John,

On Thu, Feb 8, 2024 at 1:57 AM John Abd-El-Malek <j...@chromium.org> wrote:

Hi Takuto,

First, thank you for working on improving the build speed; this is always much appreciated.

A few content owners and I saw https://crbug.com/322102823 and we had a few questions. The motivation is to learn more about the intended improvement, and whether there are other ways that are less of a maintenance burden.

-how much does this speed up the build? We ask because we generally prefer to trade off computing time than developer time, unless the former is non-trivial.

Currently, browser_interface_binders.cc is large enough that our remote worker cannot compile and reclient builds always see local fallback for that compile. (http://b/289968566)

So main focus on the issue is browser_interface_binders.cc instead of web_contents_impl.h/render_frame_host_impl.h.

Although browser_interface_binders.cc is shown as the largest time consuming target in go/chrome-build-stats, getting clear improvement for most builds only by fixing this file is hard.

So I'd like to reduce the inclusion of large headers from other time consuming targets too to reduce critical paths on build trace.

 

-do we know why including render_frame_host_impl.h is so slow? It's included by over 300 files so this could be hard to really decrease at scale, and not (re)introduce (even transitively) often which would make other cc files get bigger.

I think that is because the header also includes other large headers.

https://commondatastorage.googleapis.com/chromium-browser-clang/chrome_includes_2024-02-08_120812.html#view=edges&filter=%5Econtent%2Fbrowser%2Frenderer_host%2Frender_frame_host_impl%5C.h%24&sort=asize&reverse=&includer=%5Econtent%2Fbrowser%2Frenderer_host%2Frender_frame_host_impl%5C.h%24&included=&limit=1000

I think we can improve build time by focusing compiles which appear in critical paths more than other targets, but that doesn't need to remove the header include from every file.

This is a great resource, I've never seen that before (I've wanted to see something like this for a long time!). This is very helpful in figuring out what's expensive, and then we can find ways of more naturally reducing header bloat. I can take care of storage_parititon_impl.h which is the biggest include in render_frame_impl.h.

I see, I filed http://b/324512302 for that. Let me know if you need help with that.

 

In the meantime can we revert the other changes?

OK, I'll do that.

 

For future problems like this, feel free to email content-owners@ if it's something specific inside content, or other top level owners of other directories when a file is too big to build.

Thanks, I'll do that too.

 

Is this chrome_includes site widely advertised and I just missed it? If not, maybe it's something we can share more widely on chromium-dev so that devs have visibility into expensive headers?

I think so in https://groups.google.com/a/chromium.org/g/chromium-dev/c/K43OhI1_or0/m/vWomom8qAQAJ.

 

 

Thank you

[John Abd-El-Malek]

Hi Takuto, are things better after reducing storage_partition_impl.h's includes landed?

I can't directly tell because go/chrome-build-stats says I don't have access. Is there a reason why this is locked down, or can all Googlers be given access?

The before and after for content/browser/browser_interface_binders.cc doesn't look changed. I'm finding it hard to figure out why though. In both links it says 55MB for expanded/added size, however if I click on the cc file and switch to per-edge analysis, the includes it shows (both before/after) only sum up to 8MB in added size.

Thanks

[John Abd-El-Malek]

I wonder if content/browser/browser_interface_binders.cc is just large because of the 101 mojom files it includes. It would be nice if the chrome-includes showed this, but for example for frame.mojom.h it shows 0 added bytes. Is that because it's counted in another include indirectly?

[Takuto Ikuta]

On Tue, Feb 13, 2024 at 4:56 AM John Abd-El-Malek <j...@chromium.org> wrote:

I wonder if content/browser/browser_interface_binders.cc is just large because of the 101 mojom files it includes. It would be nice if the chrome-includes showed this, but for example for frame.mojom.h it shows 0 added bytes. Is that because it's counted in another include indirectly?

Yeah, I think some mojo headers are still large.

I got a result like https://screenshot.googleplex.com/YTLJP6tyngCqtzi

when I run the SQL query

```

SELECT
  dur / 1e9 as seconds,
  display_value
FROM experimental_slice_with_thread_and_process_info
JOIN args ON args.arg_set_id = experimental_slice_with_thread_and_process_info.arg_set_id
WHERE key = 'args.detail'
  AND display_value LIKE '%mojom%'
  AND name = 'Source'
ORDER BY seconds DESC

```

in

https://ui.perfetto.dev/#!/?s=10a5dd949acd3468f6c5e8b1946429670203c2b6afacebe8f058cf2c22c2fd1a

which is a trace of browser_interface_binders.cc compile.

+Hans Wennborg for the feature request to chrome-includes analysis. But I'm not sure whether chrome-includes should recognize such non-primitive include structure in chrome build.

 

On Mon, Feb 12, 2024 at 11:21 AM John Abd-El-Malek <j...@chromium.org> wrote:

Hi Takuto, are things better after reducing storage_partition_impl.h's includes landed?

Sorry, I think we should have taken a look at the trace from the compiler more carefully.

https://ui.perfetto.dev/#!/?s=10a5dd949acd3468f6c5e8b1946429670203c2b6afacebe8f058cf2c22c2fd1a

shows current trace of compiling browser_interface_binders.cc which we can get by `compiler_timing=true` gn args.

From the trace, processing time of storage_partition_impl.h is not so large to see a good amount of improvement for the compiling time.

I think we want to reduce includes from background_fetch_service_impl.h and ad_auction_service_impl.h somehow too.

Reducing more mojom headers might help, but I guess decoupling non-mojom headers is also necessary to get further improvement for the file.

I can't directly tell because go/chrome-build-stats says I don't have access. Is there a reason why this is locked down, or can all Googlers be given access?

Filed http://b/324991111 for that.

 

The before and after for content/browser/browser_interface_binders.cc doesn't look changed. I'm finding it hard to figure out why though. In both links it says 55MB for expanded/added size, however if I click on the cc file and switch to per-edge analysis, the includes it shows (both before/after) only sum up to 8MB in added size.

+Hans Wennborg again.

[John Abd-El-Malek]

On Mon, Feb 12, 2024 at 10:16 PM Takuto Ikuta <tik...@chromium.org> wrote:

On Tue, Feb 13, 2024 at 4:56 AM John Abd-El-Malek <j...@chromium.org> wrote:

I wonder if content/browser/browser_interface_binders.cc is just large because of the 101 mojom files it includes. It would be nice if the chrome-includes showed this, but for example for frame.mojom.h it shows 0 added bytes. Is that because it's counted in another include indirectly?

Yeah, I think some mojo headers are still large.

I got a result like https://screenshot.googleplex.com/YTLJP6tyngCqtzi

when I run the SQL query

```

SELECT
  dur / 1e9 as seconds,
  display_value
FROM experimental_slice_with_thread_and_process_info
JOIN args ON args.arg_set_id = experimental_slice_with_thread_and_process_info.arg_set_id
WHERE key = 'args.detail'
  AND display_value LIKE '%mojom%'
  AND name = 'Source'
ORDER BY seconds DESC

```

in

https://ui.perfetto.dev/#!/?s=10a5dd949acd3468f6c5e8b1946429670203c2b6afacebe8f058cf2c22c2fd1a

which is a trace of browser_interface_binders.cc compile.

+Hans Wennborg for the feature request to chrome-includes analysis. But I'm not sure whether chrome-includes should recognize such non-primitive include structure in chrome build.

What do you mean by "non-primitive includes"?

 

 

On Mon, Feb 12, 2024 at 11:21 AM John Abd-El-Malek <j...@chromium.org> wrote:

Hi Takuto, are things better after reducing storage_partition_impl.h's includes landed?

Sorry, I think we should have taken a look at the trace from the compiler more carefully.

https://ui.perfetto.dev/#!/?s=10a5dd949acd3468f6c5e8b1946429670203c2b6afacebe8f058cf2c22c2fd1a

shows current trace of compiling browser_interface_binders.cc which we can get by `compiler_timing=true` gn args.

From the trace, processing time of storage_partition_impl.h is not so large to see a good amount of improvement for the compiling time.

I think we want to reduce includes from background_fetch_service_impl.h and ad_auction_service_impl.h somehow too.

Are we sure this is what we should target, when we don't have visibility per below on where all the bytes are coming from? I'd like to avoid trying to reduce more headers without knowing that they are the biggest culprits as happened so far.

[Takuto Ikuta]

On Wed, Feb 14, 2024 at 2:25 AM John Abd-El-Malek <j...@chromium.org> wrote:

On Mon, Feb 12, 2024 at 10:16 PM Takuto Ikuta <tik...@chromium.org> wrote:

On Tue, Feb 13, 2024 at 4:56 AM John Abd-El-Malek <j...@chromium.org> wrote:

I wonder if content/browser/browser_interface_binders.cc is just large because of the 101 mojom files it includes. It would be nice if the chrome-includes showed this, but for example for frame.mojom.h it shows 0 added bytes. Is that because it's counted in another include indirectly?

Yeah, I think some mojo headers are still large.

I got a result like https://screenshot.googleplex.com/YTLJP6tyngCqtzi

when I run the SQL query

```

SELECT
  dur / 1e9 as seconds,
  display_value
FROM experimental_slice_with_thread_and_process_info
JOIN args ON args.arg_set_id = experimental_slice_with_thread_and_process_info.arg_set_id
WHERE key = 'args.detail'
  AND display_value LIKE '%mojom%'
  AND name = 'Source'
ORDER BY seconds DESC

```

in

https://ui.perfetto.dev/#!/?s=10a5dd949acd3468f6c5e8b1946429670203c2b6afacebe8f058cf2c22c2fd1a

which is a trace of browser_interface_binders.cc compile.

+Hans Wennborg for the feature request to chrome-includes analysis. But I'm not sure whether chrome-includes should recognize such non-primitive include structure in chrome build.

What do you mean by "non-primitive includes"?

I meant that I think it is better to focus on primitive include relationships in include analysis rather than handling include separately whether the file is related to mojo or not.

 

 

 

On Mon, Feb 12, 2024 at 11:21 AM John Abd-El-Malek <j...@chromium.org> wrote:

Hi Takuto, are things better after reducing storage_partition_impl.h's includes landed?

Sorry, I think we should have taken a look at the trace from the compiler more carefully.

https://ui.perfetto.dev/#!/?s=10a5dd949acd3468f6c5e8b1946429670203c2b6afacebe8f058cf2c22c2fd1a

shows current trace of compiling browser_interface_binders.cc which we can get by `compiler_timing=true` gn args.

From the trace, processing time of storage_partition_impl.h is not so large to see a good amount of improvement for the compiling time.

I think we want to reduce includes from background_fetch_service_impl.h and ad_auction_service_impl.h somehow too.

Are we sure this is what we should target, when we don't have visibility per below on where all the bytes are coming from? I'd like to avoid trying to reduce more headers without knowing that they are the biggest culprits as happened so far.

We need to do whack a mole until large header includes are removed by confirming -ftime-trace.

At least, the current trace shows that transitive includes from background_fetch_service_impl.h and ad_auction_service_impl.h are the dominant part of compiling time.

And I think that is the correct direction to reduce the compile time of browser_interface_binders.cc as https://crrev.com/c/5252622 showed faster compiling time by removing those includes from browser_interface_binders.cc.