Metrics regressions when the IncompatibleApplicationsWarning feature is enabled |
|||||||||||
Issue descriptionA 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.
,
Apr 12 2018
,
Apr 12 2018
,
Apr 12 2018
,
Apr 12 2018
,
Apr 30 2018
,
Apr 30 2018
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.
,
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
,
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
,
May 14 2018
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
,
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
,
May 17 2018
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.
,
May 17 2018
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
,
May 17 2018
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.
,
May 17 2018
,
May 18 2018
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 |
|||||||||||
Comment 1 by pmonette@chromium.org
, Apr 12 2018