Issue metadata
Sign in to add a comment
|
Regression: Chrome Canary is no longer idle without an open window |
||||||||||||||||||||||
Issue descriptionChrome Version: Canary 61.0.3114.0 OS: 10.12.5 What steps will reproduce the problem? (1) run Chrome without an open window (or only with a NewTabPage open) (2) open Chrome's Activity Monitor or Apple Activity.app What is the expected result? CPU usage is near idle with <0,5% What happens instead? CPU usage is at 4-6% Please use labels and text to provide additional information. This is a regression. Works fine (<0,5% CPU usage) in latest Chrome Stable.
,
May 29 2017
This issue seems to be extensions related. Without an installed extension CPU is <1%. With installed extension e.g. with 4 extensions CPU is at >9%
,
May 29 2017
I did a bisect and I found the following regression range: https://chromium.googlesource.com/chromium/src/+log/3a0429db8b4530814d217a23c580f636d2366d62..0f1c7d27b0dbc4074e92d5901c3c9ee94277831e Not sure which CL could cause this issue. Maybe one of you Devs have an idea?! Thanks.
,
May 30 2017
The Task Manager's data collection activity is expensive, and increases with each renderer. Extensions run in renderers, so it makes sense that a handful of renderers would mean high browser process CPU when the Task Manager is visible. If you hide the Task Manager, what does Activity Monitor report for CPU of the browser process?
,
May 30 2017
Hi shrike@, Thank you for the clarification. Yes, checking the Apple's Activity Monitor App (and hiding Chrome's Task Manager) I am seeing the same result. In build 474541 higher CPU (3-4%) usage than in build 474537). Please find enclosed two screenshots that is showing the results. If you want and if it helps, I can take a sample of the browser process. Please just let me know it. Thank you for your help.
,
May 30 2017
Hello mehmet@, Thanks for the additional info. I am looking into this now.
,
May 30 2017
Thank you! May be interesting for you: In the bad build 474537 it seems that the CPU usage needs a long time (approx. after 30-60 seconds). But if you do something with the browser e.g. open a new tab and then close it again, the CPU stays at 3-4% and after 30-60 seconds it goes down again. But if you do the same with the good build 474537, you'll notice, that the CPU usage goes down immediately without any open window.
,
May 30 2017
sorry typo -> bad build = 474541
,
May 30 2017
Looking at Chromium 60.0.3115.0 in Instruments I see a lot of activity in cc:: code, as if a repeating timer is active. Suspecting b520968 cc::SurfaceDependencyTracker should not crash when a Display goes away by fsamuel
,
May 30 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/505078044cfca8e67070b4daed6e6157d7641774 commit 505078044cfca8e67070b4daed6e6157d7641774 Author: fsamuel <fsamuel@chromium.org> Date: Tue May 30 18:21:01 2017 SurfaceDependencyTracker: Only observe BeginFrames if a deadline is needed Always observing deadlines could potentially prevent the CPU from idling because that requires a timer to always fire. This CL only observes BeginFrames if there are unresolved dependencies. A new object type, SurfaceDependencyDeadline is introduced that observes BeginFrames within its scope and reports back to SurfaceDependencyTracker when a deadline hits. The deadline must be canceled prior to going out of scope or else a DCHECK will fail. BUG= 722064 , 672962 , 727162 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2887453002 Cr-Commit-Position: refs/heads/master@{#475601} [modify] https://crrev.com/505078044cfca8e67070b4daed6e6157d7641774/cc/surfaces/BUILD.gn [add] https://crrev.com/505078044cfca8e67070b4daed6e6157d7641774/cc/surfaces/surface_dependency_deadline.cc [add] https://crrev.com/505078044cfca8e67070b4daed6e6157d7641774/cc/surfaces/surface_dependency_deadline.h [modify] https://crrev.com/505078044cfca8e67070b4daed6e6157d7641774/cc/surfaces/surface_dependency_tracker.cc [modify] https://crrev.com/505078044cfca8e67070b4daed6e6157d7641774/cc/surfaces/surface_dependency_tracker.h
,
May 30 2017
,
May 30 2017
I've tried before and after my initial suspect patch and I'm seeing the same CPU load roughly. I don't think I caused this regression.
,
May 31 2017
Hmm... that's strange :-( shrike@: Do you have any further idea which CL from the range could have cause this? Should we reopen this issue (and close the other one - see: https://bugs.chromium.org/p/chromium/issues/detail?id=722064#c14)? Thank you in advance.
,
May 31 2017
It's indeed strange. When I back out the change I don't see an improvement, however when I bisect I get the exact same regression range as you in c#3. So I'm not convinced the cc::SurfaceDependencyTracker change isn't to blame, but I can't show that it is. Deduping this bug for now. I will experiment more with finding the culprit.
,
May 31 2017
I built 60.0.3112.0 and confirmed the excess CPU, and then confirmed that the patch in c#10 fixes the problem. Next I built 61.0.3113.0, applied the patch in c#10, and saw that the excess CPU still existed. I found that reverting the following patch brought CPU back down to zero: commit e356c64f253923983248656d304d8aa471aad5f0 author eseckler <eseckler@chromium.org> Fri May 26 12:59:12 2017 committer Commit bot <commit-bot@chromium.org> Fri May 26 12:59:36 2017 [cc] Plumb BeginFrameAcks through SurfaceManager to DisplayScheduler. Review-Url: https://codereview.chromium.org/2854163003 Finally, I built 61.0.3117.0 (which is the version I was working with yesterday), reverted e356c64f253923983248656d304d8aa471aad5f0, and confirmed the excess CPU no longer occurred. So it looks like two separate patches, in back-to-back versions, triggered the same mechanism that burned CPU with no windows open. The patch in c#10 is being cherry-picked back to M60 in Issue 722064 . e356c64f253923983248656d304d8aa471aad5f0 landed in M61 so this bug is no longer an M60 blocker. The fix for e356c64f253923983248656d304d8aa471aad5f0 is now blocking M61.
,
May 31 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b13490405b5fffdda9d4ff79b3d74695335a11e9 commit b13490405b5fffdda9d4ff79b3d74695335a11e9 Author: Fady Samuel <fsamuel@chromium.org> Date: Wed May 31 20:18:19 2017 SurfaceDependencyTracker: Only observe BeginFrames if a deadline is needed Always observing deadlines could potentially prevent the CPU from idling because that requires a timer to always fire. This CL only observes BeginFrames if there are unresolved dependencies. A new object type, SurfaceDependencyDeadline is introduced that observes BeginFrames within its scope and reports back to SurfaceDependencyTracker when a deadline hits. The deadline must be canceled prior to going out of scope or else a DCHECK will fail. BUG= 722064 , 672962 , 727162 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2887453002 Cr-Original-Commit-Position: refs/heads/master@{#475601} Review-Url: https://codereview.chromium.org/2917743002 . Cr-Commit-Position: refs/branch-heads/3112@{#63} Cr-Branched-From: b6460e24cf59f429d69de255538d0fc7a425ccf9-refs/heads/master@{#474897} [modify] https://crrev.com/b13490405b5fffdda9d4ff79b3d74695335a11e9/cc/surfaces/BUILD.gn [add] https://crrev.com/b13490405b5fffdda9d4ff79b3d74695335a11e9/cc/surfaces/surface_dependency_deadline.cc [add] https://crrev.com/b13490405b5fffdda9d4ff79b3d74695335a11e9/cc/surfaces/surface_dependency_deadline.h [modify] https://crrev.com/b13490405b5fffdda9d4ff79b3d74695335a11e9/cc/surfaces/surface_dependency_tracker.cc [modify] https://crrev.com/b13490405b5fffdda9d4ff79b3d74695335a11e9/cc/surfaces/surface_dependency_tracker.h
,
Jun 1 2017
Stupid question: How do you run chrome without open tabs? Does this only happen on Mac? shrike@, did you happen to capture a trace with and without my patch? Sadly, the traceviewer doesn't seem able to load the trace you attached in #9. Thanks!
,
Jun 1 2017
I managed to repro on Mac. Seems it is caused by a DisplayScheduler (likely the one created for the extension renderer) receiving SetRootSurfaceResourcesLocked while inside a BeginFrame deadline. The root surface never becomes unlocked, which means that the DisplayScheduler continues to observe BeginFrames. I believe that this situation is triggered by my patch because we now also call UpdateRootSurfaceResourcesLocked for no-damage acks in Display::OnSurfaceDamaged. I'll send a patch to fix that.
,
Jun 1 2017
Quick update, no-damage acks aren't the issue. It's actually caused by us passing on *changed in Display::OnSurfaceDamaged, which may have been modified by a different Display (rather than "this" Display). That leads to a DisplayScheduler starting to observe even if its Display didn't actually have any damage.
,
Jun 1 2017
> Stupid question: How do you run chrome without open tabs? Does this only happen on Mac? Not at all. Yes, this probably only happens on the Mac. The trace I attached is from the Instruments app on the Mac which shows backtraces and time spent in various functions within a running app. Sorry I didn't attach a Chrome tracing trace - it sounds like, though, that you have zeroed in on the problem.
,
Jun 1 2017
Thanks, yeah, this would only happen if there is more than 1 cc::Display, which is more common on Mac. Patch to fix is out here: https://chromium-review.googlesource.com/c/520766/
,
Jun 5 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/6444a99819d7fe77ee5c8f895e1e47b95698c716 commit 6444a99819d7fe77ee5c8f895e1e47b95698c716 Author: Eric Seckler <eseckler@chromium.org> Date: Mon Jun 05 15:20:38 2017 [cc] Fix display_damaged calculation in Display::OnSurfaceDamaged. We were previously passing on *changed, which could lead to propagation of the flag value from other Displays / SurfaceObservers. Calculate the value passed on to DisplayScheduler locally instead. Also updates the OnSurfaceDamaged interface to return a boolean and removes unnecessary calls to UpdateRootSurfaceResourcesLocked and SurfaceDamageExpected. Bug: 727162 , 697086 Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel Change-Id: Ie0f78705ee8f76417a7d9f2d8ed87365c8a1cb0a Reviewed-on: https://chromium-review.googlesource.com/520766 Reviewed-by: Fady Samuel <fsamuel@chromium.org> Reviewed-by: Sunny Sachanandani <sunnyps@chromium.org> Commit-Queue: Eric Seckler <eseckler@chromium.org> Cr-Commit-Position: refs/heads/master@{#476985} [modify] https://crrev.com/6444a99819d7fe77ee5c8f895e1e47b95698c716/cc/surfaces/compositor_frame_sink_support.cc [modify] https://crrev.com/6444a99819d7fe77ee5c8f895e1e47b95698c716/cc/surfaces/display.cc [modify] https://crrev.com/6444a99819d7fe77ee5c8f895e1e47b95698c716/cc/surfaces/display.h [modify] https://crrev.com/6444a99819d7fe77ee5c8f895e1e47b95698c716/cc/surfaces/display_unittest.cc [modify] https://crrev.com/6444a99819d7fe77ee5c8f895e1e47b95698c716/cc/surfaces/surface_manager.cc [modify] https://crrev.com/6444a99819d7fe77ee5c8f895e1e47b95698c716/cc/surfaces/surface_observer.h [modify] https://crrev.com/6444a99819d7fe77ee5c8f895e1e47b95698c716/cc/test/fake_surface_observer.cc [modify] https://crrev.com/6444a99819d7fe77ee5c8f895e1e47b95698c716/cc/test/fake_surface_observer.h [modify] https://crrev.com/6444a99819d7fe77ee5c8f895e1e47b95698c716/components/viz/frame_sinks/mojo_frame_sink_manager.cc [modify] https://crrev.com/6444a99819d7fe77ee5c8f895e1e47b95698c716/components/viz/frame_sinks/mojo_frame_sink_manager.h
,
Jun 5 2017
,
Jun 6 2017
Great, looks good to me again in latest Canary 61.0.3122.0 {#477124}
Thank you all !!!
|
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by meh...@chromium.org
, May 28 2017130 KB
130 KB View Download
106 KB
106 KB View Download