New issue
Advanced search Search tips

Issue 832286 link

Starred by 1 user

Issue metadata

Status: Assigned
Owner:
Components:
EstimatedDays: ----
NextAction: ----
OS: Windows
Pri: 1
Type: Bug

Blocked on:
issue 830663
issue 830664
issue 830668
issue 830829
issue 837459

Blocking:
issue 717696



Sign in to add a comment

Metrics regressions when the IncompatibleApplicationsWarning feature is enabled

Project Member Reported by pmonette@chromium.org, Apr 12 2018

Issue description

A fieldtrial config was added for the IncompatibleApplicationsWarning feature, and a number of metrics regressed.

https://chromium-review.googlesource.com/c/chromium/src/+/996581

They should be addressed before enabling the feature on beta.
 
Blockedon: 830663
Blockedon: 830668
Blockedon: 830664
Blocking: 717696
Blockedon: 830829
Blockedon: 837459
A lot of those performance regressions are happening because they measure total CPU time, which counts the module inspection work that happens on a background thread. In theory this should not impact the performance of Chrome since the scheduler should only run these tasks when there are spare cycles.
Project Member

Comment 8 by bugdroid1@chromium.org, May 11 2018

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

commit 381d19a1d68c467bd0c8cbf12199a220c529fe2b
Author: Patrick Monette <pmonette@chromium.org>
Date: Fri May 11 20:45:32 2018

Fix a startup performance issue with the ModuleDatabase feature

The ModuleEventSink interface was bound on the UI thread, which caused
cross-process IPC to retrieve information about a loaded module in the
renderer process.

A new sequence with background priority is instead used so that it
doesn't impact startup.

Bug: 832286
Change-Id: I5ab50bc45386c9a1ddb2888f80293d78bd768a88
Reviewed-on: https://chromium-review.googlesource.com/1037935
Reviewed-by: Chris Hamilton <chrisha@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: Patrick Monette <pmonette@chromium.org>
Cr-Commit-Position: refs/heads/master@{#558010}
[modify] https://crrev.com/381d19a1d68c467bd0c8cbf12199a220c529fe2b/chrome/browser/chrome_browser_main_win.cc
[modify] https://crrev.com/381d19a1d68c467bd0c8cbf12199a220c529fe2b/chrome/browser/chrome_content_browser_client.cc
[modify] https://crrev.com/381d19a1d68c467bd0c8cbf12199a220c529fe2b/chrome/browser/conflicts/module_event_sink_impl_win.cc

Project Member

Comment 9 by bugdroid1@chromium.org, May 14 2018

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

commit b710e60381a933bec9ba59993e626f6979f0b19e
Author: Patrick Monette <pmonette@chromium.org>
Date: Mon May 14 14:03:22 2018

Revert "Fix a startup performance issue with the ModuleDatabase feature"

This reverts commit 381d19a1d68c467bd0c8cbf12199a220c529fe2b.

Reason for revert: Crashes on canary

Original change's description:
> Fix a startup performance issue with the ModuleDatabase feature
>
> The ModuleEventSink interface was bound on the UI thread, which caused
> cross-process IPC to retrieve information about a loaded module in the
> renderer process.
>
> A new sequence with background priority is instead used so that it
> doesn't impact startup.
>
> Bug: 832286
> Change-Id: I5ab50bc45386c9a1ddb2888f80293d78bd768a88
> Reviewed-on: https://chromium-review.googlesource.com/1037935
> Reviewed-by: Chris Hamilton <chrisha@chromium.org>
> Reviewed-by: Scott Violet <sky@chromium.org>
> Commit-Queue: Patrick Monette <pmonette@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#558010}

TBR=sky@chromium.org,chrisha@chromium.org,pmonette@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 832286, 842506
Change-Id: I95befdad542c085e83e2ba5e292ae43f0545430e
Reviewed-on: https://chromium-review.googlesource.com/1056687
Commit-Queue: Patrick Monette <pmonette@chromium.org>
Reviewed-by: Patrick Monette <pmonette@chromium.org>
Cr-Commit-Position: refs/heads/master@{#558290}
[modify] https://crrev.com/b710e60381a933bec9ba59993e626f6979f0b19e/chrome/browser/chrome_browser_main_win.cc
[modify] https://crrev.com/b710e60381a933bec9ba59993e626f6979f0b19e/chrome/browser/chrome_content_browser_client.cc
[modify] https://crrev.com/b710e60381a933bec9ba59993e626f6979f0b19e/chrome/browser/conflicts/module_event_sink_impl_win.cc

Project Member

Comment 10 by bugdroid1@chromium.org, May 14 2018

Labels: merge-merged-3430
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/42625cb20b8a093393965b163c08ea8c92d59744

commit 42625cb20b8a093393965b163c08ea8c92d59744
Author: Patrick Monette <pmonette@chromium.org>
Date: Mon May 14 19:04:48 2018

Revert "Fix a startup performance issue with the ModuleDatabase feature"

This reverts commit 381d19a1d68c467bd0c8cbf12199a220c529fe2b.

Reason for revert: Crashes on canary

Original change's description:
> Fix a startup performance issue with the ModuleDatabase feature
>
> The ModuleEventSink interface was bound on the UI thread, which caused
> cross-process IPC to retrieve information about a loaded module in the
> renderer process.
>
> A new sequence with background priority is instead used so that it
> doesn't impact startup.
>
> Bug: 832286
> Change-Id: I5ab50bc45386c9a1ddb2888f80293d78bd768a88
> Reviewed-on: https://chromium-review.googlesource.com/1037935
> Reviewed-by: Chris Hamilton <chrisha@chromium.org>
> Reviewed-by: Scott Violet <sky@chromium.org>
> Commit-Queue: Patrick Monette <pmonette@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#558010}

TBR=sky@chromium.org,chrisha@chromium.org,pmonette@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 832286, 842506
Change-Id: I95befdad542c085e83e2ba5e292ae43f0545430e
Reviewed-on: https://chromium-review.googlesource.com/1056687
Commit-Queue: Patrick Monette <pmonette@chromium.org>
Reviewed-by: Patrick Monette <pmonette@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#558290}(cherry picked from commit b710e60381a933bec9ba59993e626f6979f0b19e)
Reviewed-on: https://chromium-review.googlesource.com/1057848
Cr-Commit-Position: refs/branch-heads/3430@{#5}
Cr-Branched-From: e7023f8c2d97b4e7c0a109659dfb7e3ed4961ae3-refs/heads/master@{#558173}
[modify] https://crrev.com/42625cb20b8a093393965b163c08ea8c92d59744/chrome/browser/chrome_browser_main_win.cc
[modify] https://crrev.com/42625cb20b8a093393965b163c08ea8c92d59744/chrome/browser/chrome_content_browser_client.cc
[modify] https://crrev.com/42625cb20b8a093393965b163c08ea8c92d59744/chrome/browser/conflicts/module_event_sink_impl_win.cc

Project Member

Comment 11 by bugdroid1@chromium.org, May 15 2018

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

commit 9dd3eaada3238a17ed4f1d39ddf76023923b4982
Author: Patrick Monette <pmonette@chromium.org>
Date: Tue May 15 17:07:56 2018

Retrieve module information on a background task in ModuleEventSinkImpl

Retrieving this information requires IPC to the remote process, which
is blocking and thus must not be done on the UI thread.

But the ModuleEventSinkImpl instance must still live on the UI thread,
because thats where the RenderProcessHost instance lives. The
RenderProcessHost must be accessed to get the handle to the remote
process inside the ModuleEventSinkImpl::Create() factory function.

To make the process handle available from the background task, it is
now duplicated and passed to the task via the PostTaskWithTraits()
call.

Bug: 832286
Change-Id: I3948a585f3d7645d706b0e8977cdf3182f212306
Reviewed-on: https://chromium-review.googlesource.com/1058339
Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: Patrick Monette <pmonette@chromium.org>
Cr-Commit-Position: refs/heads/master@{#558758}
[modify] https://crrev.com/9dd3eaada3238a17ed4f1d39ddf76023923b4982/chrome/browser/chrome_browser_main_win.cc
[modify] https://crrev.com/9dd3eaada3238a17ed4f1d39ddf76023923b4982/chrome/browser/chrome_content_browser_client.cc
[modify] https://crrev.com/9dd3eaada3238a17ed4f1d39ddf76023923b4982/chrome/browser/conflicts/module_event_sink_impl_win.cc
[modify] https://crrev.com/9dd3eaada3238a17ed4f1d39ddf76023923b4982/chrome/browser/conflicts/module_event_sink_impl_win.h
[modify] https://crrev.com/9dd3eaada3238a17ed4f1d39ddf76023923b4982/chrome/browser/conflicts/module_event_sink_impl_win_unittest.cc

Labels: -M-66 -merge-merged-3430 Merge-Request-67
Requesting Merge to m67 for CL at comment #11. It is a Chrome startup performance fix for a feature we want to release for that milestone. It has been verified in canary.
Project Member

Comment 13 by sheriffbot@chromium.org, May 17 2018

Labels: -Merge-Request-67 Merge-Review-67 Hotlist-Merge-Review
This bug requires manual review: We are only 11 days from stable.
Please contact the milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), kbleicher@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Approving merge for CL listed at #11 to M67 branch 3396 based on comment #12. Additional details - https://bugs.chromium.org/p/chromium/issues/detail?id=717696#c72.
Labels: -Merge-Review-67 Merge-Approved-67
Project Member

Comment 16 by bugdroid1@chromium.org, May 18 2018

Labels: -merge-approved-67 merge-merged-3396
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c10ec5b97f95eabfd365d2c333b5f323e0070d24

commit c10ec5b97f95eabfd365d2c333b5f323e0070d24
Author: Patrick Monette <pmonette@chromium.org>
Date: Fri May 18 00:56:46 2018

Retrieve module information on a background task in ModuleEventSinkImpl

Retrieving this information requires IPC to the remote process, which
is blocking and thus must not be done on the UI thread.

But the ModuleEventSinkImpl instance must still live on the UI thread,
because thats where the RenderProcessHost instance lives. The
RenderProcessHost must be accessed to get the handle to the remote
process inside the ModuleEventSinkImpl::Create() factory function.

To make the process handle available from the background task, it is
now duplicated and passed to the task via the PostTaskWithTraits()
call.

This is a cherry-pick of
https://chromium-review.googlesource.com/c/chromium/src/+/1058339
on branch 3396.

Tbr: sky@chromium.org
Bug: 832286
Change-Id: Ia56499dfec96cfb0cc32863233b3265133a31034
Reviewed-on: https://chromium-review.googlesource.com/1065251
Reviewed-by: Patrick Monette <pmonette@chromium.org>
Cr-Commit-Position: refs/branch-heads/3396@{#634}
Cr-Branched-From: 9ef2aa869bc7bc0c089e255d698cca6e47d6b038-refs/heads/master@{#550428}
[modify] https://crrev.com/c10ec5b97f95eabfd365d2c333b5f323e0070d24/chrome/browser/chrome_browser_main_win.cc
[modify] https://crrev.com/c10ec5b97f95eabfd365d2c333b5f323e0070d24/chrome/browser/chrome_content_browser_client.cc
[modify] https://crrev.com/c10ec5b97f95eabfd365d2c333b5f323e0070d24/chrome/browser/conflicts/module_event_sink_impl_win.cc
[modify] https://crrev.com/c10ec5b97f95eabfd365d2c333b5f323e0070d24/chrome/browser/conflicts/module_event_sink_impl_win.h
[modify] https://crrev.com/c10ec5b97f95eabfd365d2c333b5f323e0070d24/chrome/browser/conflicts/module_event_sink_impl_win_unittest.cc

Sign in to add a comment