Issue metadata
Sign in to add a comment
|
Opacity animation consuming main thread resources |
|||||||||||||||||||||||
Issue descriptionThis issue is a migration of https://bugs.chromium.org/p/chromium/issues/detail?id=361587#c12. Comment 12 by esprehn@chromium.org: In testing on my 2013 Mac Pro: Safari 59.0.3047.0: Browser process: 0% CPU Render process: 0% WindowServer: 6% (This is where composited animations run for Safari) = 6% Chrome Canary 59.0.3047.0: Browser process: 6% CPU Render process: 3% GPU process 2% WindowServer: 6% = 17% So we're using 11% more CPU. Safari appears to be sleeping in the render process and not ticking at 60fps on main.
,
Mar 24 2017
In my testing all compositor driven animations (transform, opacity, etc.) still pump at 60fps on main doing a recalcStyle and recomposite.
,
Mar 24 2017
Are you testing on Mac? Does it show up in the DevTools performance tab?
,
Mar 24 2017
,
Mar 24 2017
Able to repro on a Linux laptop running Chrome 59, wasn't able to repro on Linux desktop. Also this is flaky, sometimes it composites, sometimes it main threads, sometimes it main threads for a portion of the beginning of the animation then the main thread goes silent. Very strange behaviour, suspecting something racy going on with the cc thread perhaps.
,
Mar 24 2017
Able to repro on Linux using Chrome 58 beta, 59 dev and a ToT build, just needed to persist against the flaky behaviour a bit more. Have not been able to repro on Linux Chrome 57 stable. In the test case: - The animation is always getting composited according to Animation::maybeStartAnimationOnCompositor(). - Animation::timeToEffectChange() is always returning inf. - AnimationTimeline::scheduleNextService() never calls m_timing->serviceOnNextFrame() or m_timing->wakeAfter(). For some reason the scheduler is calling beginMainFrame() for the beginning of the animation and stops after some arbitrary amount of time. These frames aren't being requested by Blink's animation engine. Here's the bottom part of the stack trace of an unnecessary beginMainFrame(): #9 0x7fc2f4a60219 blink::DocumentAnimations::updateAnimations() #10 0x7fc2f5389c83 blink::PaintLayerCompositor::updateIfNeededRecursiveInternal() #11 0x7fc2f538993b blink::PaintLayerCompositor::updateIfNeededRecursive() #12 0x7fc2f4f17a51 blink::FrameView::updateLifecyclePhasesInternal() #13 0x7fc2f4f17691 blink::FrameView::updateAllLifecyclePhases() #14 0x7fc2f548c6ea blink::PageAnimator::updateAllLifecyclePhases() #15 0x7fc2f61209c4 blink::WebViewImpl::updateAllLifecyclePhases() #16 0x7fc2fa83d6a7 content::RenderWidget::UpdateVisualState() #17 0x7fc2f7c10f60 cc::ProxyMain::BeginMainFrame() #18 0x7fc2f7c0fae4 _ZN4base8internal12InvokeHelperILb1EvE8MakeItSoIRKMN2cc9ProxyMainEFvSt10unique_ptrINS4_28BeginMainFrameAndCommitStateESt14default_deleteIS7_EEERKNS_7WeakPtrIS5_EEJSA_EEEvOT_OT0_DpOT1_ #19 0x7fc2f7c0f9c8 _ZN4base8internal7InvokerINS0_9BindStateIMN2cc9ProxyMainEFvSt10unique_ptrINS3_28BeginMainFrameAndCommitStateESt14default_deleteIS6_EEEJNS_7WeakPtrIS4_EENS0_13PassedWrapperIS9_EEEEEFvvEE3RunEPNS0_13BindStateBaseE #20 0x7fc2f963a120 _ZNO4base8CallbackIFvvELNS_8internal8CopyModeE0ELNS2_10RepeatModeE0EE3RunEv #21 0x7fc2f9639f4a base::debug::TaskAnnotator::RunTask() #22 0x7fc2f6507cd0 blink::scheduler::TaskQueueManager::ProcessTaskFromWorkQueue() #23 0x7fc2f6505230 blink::scheduler::TaskQueueManager::DoWork() #24 0x7fc2f650a385 _ZN4base8internal13FunctorTraitsIMN5blink9scheduler16TaskQueueManagerEFvbEvE6InvokeIRKNS_7WeakPtrIS4_EEJRKbEEEvS6_OT_DpOT0_ #25 0x7fc2f963a120 _ZNO4base8CallbackIFvvELNS_8internal8CopyModeE0ELNS2_10RepeatModeE0EE3RunEv #26 0x7fc2f9639f4a base::debug::TaskAnnotator::RunTask() #27 0x7fc2f966af6d base::MessageLoop::RunTask() #28 0x7fc2f966b656 base::MessageLoop::DoWork() #29 0x7fc2f966d019 base::MessagePumpDefault::Run() #30 0x7fc2f966acbe base::MessageLoop::RunHandler() #31 0x7fc2f969f5bc base::RunLoop::Run() #32 0x7fc2fa84aadb content::RendererMain() #33 0x7fc2fa9b51b2 content::RunZygote() #34 0x7fc2fa9b5764 content::RunNamedProcessTypeMain() #35 0x7fc2fa9b63b6 content::ContentMainRunnerImpl::Run() #36 0x7fc2f1c4dee5 service_manager::Main() #37 0x7fc2fa9b4fa2 content::ContentMain() #38 0x00000046bf71 main #39 0x7fc2f21daf45 __libc_start_main Can scheduler team help drill down what's requesting these main thread frames to be generated?
,
Mar 24 2017
Looks like this could be caused by the animation engine after all. Added logging to RenderWidgetCompositor::setNeedsBeginFrame() to find out what's calling it. In the failure mode I see two call sites in every beginMainFrame() call: [1:1:0324/160211.134921:1483826682930:ERROR:render_widget_compositor.cc(1046)] ======== beginMainFrame ======== [1:1:0324/160211.136179:1483826684184:ERROR:render_widget_compositor.cc(736)] #0 0x7f4de2703477 base::debug::StackTrace::StackTrace() #1 0x7f4de384f44a content::RenderWidgetCompositor::setNeedsBeginFrame() #2 0x7f4ddf11c4ed blink::ChromeClientImpl::scheduleAnimation() #3 0x7f4dddd499e3 blink::Document::scheduleLayoutTreeUpdate() #4 0x7f4dddde1655 blink::Node::markAncestorsWithChildNeedsStyleRecalc() #5 0x7f4dddde1982 blink::Node::setNeedsStyleRecalc() #6 0x7f4dddd92cd5 blink::Element::setNeedsAnimationStyleRecalc() #7 0x7f4dddb3ddc5 blink::KeyframeEffectReadOnly::applyEffects() #8 0x7f4dddb3e1c0 blink::KeyframeEffectReadOnly::updateChildrenAndEffects() #9 0x7f4dddae1f4a blink::AnimationEffectReadOnly::updateInheritedTime() #10 0x7f4dddada456 blink::Animation::update() #11 0x7f4dddae7bf0 blink::AnimationTimeline::serviceAnimations() #12 0x7f4dddd47344 blink::Document::updateStyleAndLayoutTree() #13 0x7f4dddd4d137 blink::Document::updateStyleAndLayoutTreeIgnorePendingStylesheets() #14 0x7f4dddd4cb6d blink::Document::updateStyleAndLayoutIgnorePendingStylesheets() #15 0x7f4dddeb19cc blink::FrameSelection::computeVisibleSelectionInDOMTreeDeprecated() #16 0x7f4dddebcb0b blink::InputMethodController::textInputType() #17 0x7f4de390b9ad content::RenderWidget::GetTextInputType() #18 0x7f4de390886e content::RenderWidget::UpdateTextInputStateInternal() #19 0x7f4de39079a3 content::RenderWidget::WillBeginCompositorFrame() #20 0x7f4de0cdaf19 cc::ProxyMain::BeginMainFrame() [1:1:0324/160211.199173:1483826747188:ERROR:render_widget_compositor.cc(736)] #0 0x7f4de2703477 base::debug::StackTrace::StackTrace() #1 0x7f4de384f44a content::RenderWidgetCompositor::setNeedsBeginFrame() #2 0x7f4ddf11c4ed blink::ChromeClientImpl::scheduleAnimation() #3 0x7f4dde4536ef blink::PaintLayerCompositor::setNeedsCompositingUpdate() #4 0x7f4dde5c28e8 blink::PaintLayer::attemptDirectCompositingUpdate() #5 0x7f4dde5c295a blink::PaintLayer::styleDidChange() #6 0x7f4dde35b3ad blink::LayoutBoxModelObject::styleDidChange() #7 0x7f4dde33d068 blink::LayoutBox::styleDidChange() #8 0x7f4dde30cdcd blink::LayoutBlock::styleDidChange() #9 0x7f4dde325df1 blink::LayoutBlockFlow::styleDidChange() #10 0x7f4dde3b88e4 blink::LayoutObject::setStyle() #11 0x7f4dddd91d69 blink::Element::recalcOwnStyle() #12 0x7f4dddd91627 blink::Element::recalcStyle() #13 0x7f4dddd197dc blink::ContainerNode::recalcDescendantStyles() #14 0x7f4dddd91512 blink::Element::recalcStyle() #15 0x7f4dddd197dc blink::ContainerNode::recalcDescendantStyles() #16 0x7f4dddd91512 blink::Element::recalcStyle() #17 0x7f4dddd4bb6e blink::Document::updateStyle() #18 0x7f4dddd47392 blink::Document::updateStyleAndLayoutTree() #19 0x7f4dddd4d137 blink::Document::updateStyleAndLayoutTreeIgnorePendingStylesheets() #20 0x7f4dddd4cb6d blink::Document::updateStyleAndLayoutIgnorePendingStylesheets() #21 0x7f4dddeb19cc blink::FrameSelection::computeVisibleSelectionInDOMTreeDeprecated() #22 0x7f4dddebcb0b blink::InputMethodController::textInputType() #23 0x7f4de390b9ad content::RenderWidget::GetTextInputType() #24 0x7f4de390886e content::RenderWidget::UpdateTextInputStateInternal() #25 0x7f4de39079a3 content::RenderWidget::WillBeginCompositorFrame() #26 0x7f4de0cdaf19 cc::ProxyMain::BeginMainFrame()
,
Mar 27 2017
In Chrome stable the second last BeginMainFrame call appears to trigger the final BeginMainFrame call via ScrollableArea::scheduleAnimation() and ProxyMain::SetAnimationEvents(). In ToT BeginMainFrame appears to trigger it via those as well as via Animation::update() and a (probably consequential) call to styleDidChange(). Compare stack traces for calls to RenderWidgetCompositor::setNeedsBeginFrame() for the same frame between stable and ToT. Stable Chrome (ad51088c0e8776e8dcd963dbe752c4035ba6dab6): BeginMainFrame(2) setNeedsBeginFrame() #0 0x7fb4f279b8de base::debug::StackTrace::StackTrace() #1 0x7fb4f387e44d content::RenderWidgetCompositor::setNeedsBeginFrame() #2 0x7fb4ef6c509d blink::ChromeClientImpl::scheduleAnimation() #3 0x7fb4efb7e092 blink::ScrollableArea::scheduleAnimation() #4 0x7fb4ee07e080 blink::DocumentAnimations::updateAnimations() #5 0x7fb4ee9ee49f blink::PaintLayerCompositor::updateIfNeededRecursiveInternal() #6 0x7fb4ee9ee151 blink::PaintLayerCompositor::updateIfNeededRecursive() #7 0x7fb4ee578fa9 blink::FrameView::updateLifecyclePhasesInternal() #8 0x7fb4ee578c21 blink::FrameView::updateAllLifecyclePhases() #9 0x7fb4eeaf70ea blink::PageAnimator::updateAllLifecyclePhases() #10 0x7fb4ef795888 blink::WebViewImpl::updateAllLifecyclePhases() #11 0x7fb4f101fbae cc::ProxyMain::BeginMainFrame() setNeedsBeginFrame() #0 0x7fb4f279b8de base::debug::StackTrace::StackTrace() #1 0x7fb4f387e44d content::RenderWidgetCompositor::setNeedsBeginFrame() #2 0x7fb4ef6c509d blink::ChromeClientImpl::scheduleAnimation() #3 0x7fb4efb7e092 blink::ScrollableArea::scheduleAnimation() #4 0x7fb4ee03f341 blink::AnimationTimeline::AnimationTimelineTiming::serviceOnNextFrame() #5 0x7fb4ee03fb6a blink::AnimationTimeline::setOutdatedAnimation() #6 0x7fb4ee02cec0 blink::Animation::setCurrentTimeInternal() #7 0x7fb4ee02eb37 blink::Animation::setStartTimeInternal() #8 0x7fb4ee02e892 blink::Animation::notifyStartTime() #9 0x7fb4ee02e696 blink::Animation::notifyCompositorStartTime() #10 0x7fb4ee07ab2e blink::CompositorPendingAnimations::notifyCompositorAnimationStarted() #11 0x7fb4ef9b4361 blink::CompositorAnimationPlayer::NotifyAnimationStarted() #12 0x7fb4eae27ebb cc::AnimationPlayer::NotifyAnimationStarted() #13 0x7fb4eae2cb23 cc::ElementAnimations::NotifyAnimationStarted() #14 0x7fb4eae22a13 cc::AnimationHost::SetAnimationEvents() #15 0x7fb4f0fed501 cc::LayerTreeHostInProcess::SetAnimationEvents() #16 0x7fb4f101f272 cc::ProxyMain::SetAnimationEvents() ToT Chrome (07e9a78420c274fd31d0a7f08d4bc719337f4271): BeginMainFrame(2) setNeedsBeginFrame() #0 0x7efc9ff37477 base::debug::StackTrace::StackTrace() #1 0x7efca108342d content::RenderWidgetCompositor::setNeedsBeginFrame() #2 0x7efc9c9504ed blink::ChromeClientImpl::scheduleAnimation() #3 0x7efc9ce34ec2 blink::ScrollableArea::scheduleAnimation() #4 0x7efc9b35e13d blink::DocumentAnimations::updateAnimations() #5 0x7efc9bc87bb3 blink::PaintLayerCompositor::updateIfNeededRecursiveInternal() #6 0x7efc9bc8786b blink::PaintLayerCompositor::updateIfNeededRecursive() #7 0x7efc9b815981 blink::FrameView::updateLifecyclePhasesInternal() #8 0x7efc9b8155c1 blink::FrameView::updateAllLifecyclePhases() #9 0x7efc9bd8a61a blink::PageAnimator::updateAllLifecyclePhases() #10 0x7efc9ca1e9c4 blink::WebViewImpl::updateAllLifecyclePhases() #11 0x7efca113b7f7 content::RenderWidget::UpdateVisualState() #12 0x7efc9e50ef60 cc::ProxyMain::BeginMainFrame() setNeedsBeginFrame() #0 0x7efc9ff37477 base::debug::StackTrace::StackTrace() #1 0x7efca108342d content::RenderWidgetCompositor::setNeedsBeginFrame() #2 0x7efc9c9504ed blink::ChromeClientImpl::scheduleAnimation() #3 0x7efc9ce34ec2 blink::ScrollableArea::scheduleAnimation() #4 0x7efc9b31c1f1 blink::AnimationTimeline::AnimationTimelineTiming::serviceOnNextFrame() #5 0x7efc9b31c92a blink::AnimationTimeline::setOutdatedAnimation() #6 0x7efc9b30af19 blink::Animation::setCurrentTimeInternal() #7 0x7efc9b30cbad blink::Animation::setStartTimeInternal() #8 0x7efc9b30c902 blink::Animation::notifyStartTime() #9 0x7efc9b30c706 blink::Animation::notifyCompositorStartTime() #10 0x7efc9b35ac0d blink::CompositorPendingAnimations::notifyCompositorAnimationStarted() #11 0x7efc9cc5b581 blink::CompositorAnimationPlayer::NotifyAnimationStarted() #12 0x7efc98175f8b cc::AnimationPlayer::NotifyAnimationStarted() #13 0x7efc9817ac43 cc::ElementAnimations::NotifyAnimationStarted() #14 0x7efc981709d3 cc::AnimationHost::SetAnimationEvents() #15 0x7efc9e4c3b3d cc::LayerTreeHost::SetAnimationEvents() #16 0x7efc9e50e5f2 cc::ProxyMain::SetAnimationEvents() setNeedsBeginFrame() #0 0x7efc9ff37477 base::debug::StackTrace::StackTrace() #1 0x7efca108342d content::RenderWidgetCompositor::setNeedsBeginFrame() #2 0x7efc9c9504ed blink::ChromeClientImpl::scheduleAnimation() #3 0x7efc9b57d9e3 blink::Document::scheduleLayoutTreeUpdate() #4 0x7efc9b615655 blink::Node::markAncestorsWithChildNeedsStyleRecalc() #5 0x7efc9b615982 blink::Node::setNeedsStyleRecalc() #6 0x7efc9b5c6cd5 blink::Element::setNeedsAnimationStyleRecalc() #7 0x7efc9b371dc5 blink::KeyframeEffectReadOnly::applyEffects() #8 0x7efc9b3721c0 blink::KeyframeEffectReadOnly::updateChildrenAndEffects() #9 0x7efc9b315f4a blink::AnimationEffectReadOnly::updateInheritedTime() #10 0x7efc9b30e456 blink::Animation::update() #11 0x7efc9b31bbf0 blink::AnimationTimeline::serviceAnimations() #12 0x7efc9b57b344 blink::Document::updateStyleAndLayoutTree() #13 0x7efc9b581137 blink::Document::updateStyleAndLayoutTreeIgnorePendingStylesheets() #14 0x7efc9b580b6d blink::Document::updateStyleAndLayoutIgnorePendingStylesheets() #15 0x7efc9b6e59cc blink::FrameSelection::computeVisibleSelectionInDOMTreeDeprecated() #16 0x7efc9b6f0b0b blink::InputMethodController::textInputType() #17 0x7efca113f8fd content::RenderWidget::GetTextInputType() #18 0x7efca113c7be content::RenderWidget::UpdateTextInputStateInternal() #19 0x7efca113b8f3 content::RenderWidget::WillBeginCompositorFrame() #20 0x7efc9e50ef19 cc::ProxyMain::BeginMainFrame() setNeedsBeginFrame() #0 0x7efc9ff37477 base::debug::StackTrace::StackTrace() #1 0x7efca108342d content::RenderWidgetCompositor::setNeedsBeginFrame() #2 0x7efc9c9504ed blink::ChromeClientImpl::scheduleAnimation() #3 0x7efc9bc876ef blink::PaintLayerCompositor::setNeedsCompositingUpdate() #4 0x7efc9bdf68e8 blink::PaintLayer::attemptDirectCompositingUpdate() #5 0x7efc9bdf695a blink::PaintLayer::styleDidChange() #6 0x7efc9bb8f3ad blink::LayoutBoxModelObject::styleDidChange() #7 0x7efc9bb71068 blink::LayoutBox::styleDidChange() #8 0x7efc9bb40dcd blink::LayoutBlock::styleDidChange() #9 0x7efc9bb59df1 blink::LayoutBlockFlow::styleDidChange() #10 0x7efc9bbec8e4 blink::LayoutObject::setStyle() #11 0x7efc9b5c5d69 blink::Element::recalcOwnStyle() #12 0x7efc9b5c5627 blink::Element::recalcStyle() #13 0x7efc9b54d7dc blink::ContainerNode::recalcDescendantStyles() #14 0x7efc9b5c5512 blink::Element::recalcStyle() #15 0x7efc9b54d7dc blink::ContainerNode::recalcDescendantStyles() #16 0x7efc9b5c5512 blink::Element::recalcStyle() #17 0x7efc9b57fb6e blink::Document::updateStyle() #18 0x7efc9b57b392 blink::Document::updateStyleAndLayoutTree() #19 0x7efc9b581137 blink::Document::updateStyleAndLayoutTreeIgnorePendingStylesheets() #20 0x7efc9b580b6d blink::Document::updateStyleAndLayoutIgnorePendingStylesheets() #21 0x7efc9b6e59cc blink::FrameSelection::computeVisibleSelectionInDOMTreeDeprecated() #22 0x7efc9b6f0b0b blink::InputMethodController::textInputType() #23 0x7efca113f8fd content::RenderWidget::GetTextInputType() #24 0x7efca113c7be content::RenderWidget::UpdateTextInputStateInternal() #25 0x7efca113b8f3 content::RenderWidget::WillBeginCompositorFrame() #26 0x7efc9e50ef19 cc::ProxyMain::BeginMainFrame()
,
Mar 27 2017
This section of the additional stack trace looks pretty suspicious, it was added in February (2090c9f72c7eebd76726965cfc7d15cb8d1cc12d), Chrome Beta branched in March so it's a candidate for the regression point. Bisecting now. #15 0x7efc9b6e59cc blink::FrameSelection::computeVisibleSelectionInDOMTreeDeprecated() #16 0x7efc9b6f0b0b blink::InputMethodController::textInputType() #17 0x7efca113f8fd content::RenderWidget::GetTextInputType() #18 0x7efca113c7be content::RenderWidget::UpdateTextInputStateInternal() #19 0x7efca113b8f3 content::RenderWidget::WillBeginCompositorFrame() #20 0x7efc9e50ef19 cc::ProxyMain::BeginMainFrame()
,
Mar 27 2017
Reassigning to Alan since he still seems to be working on it.
,
Mar 28 2017
Bisected down to 157413286770a7ac5a24c446a30c08f749738276 as the cause of this issue. With the following patch applied one can easily see the presence of main thread frames ticking: diff --git a/content/renderer/gpu/render_widget_compositor.cc b/content/renderer/gpu/render_widget_compositor.cc index ae598c0253cc..0411d283725e 100644 --- a/content/renderer/gpu/render_widget_compositor.cc +++ b/content/renderer/gpu/render_widget_compositor.cc @@ -1016,6 +1016,7 @@ void RenderWidgetCompositor::DidBeginMainFrame() { } void RenderWidgetCompositor::BeginMainFrame(const cc::BeginFrameArgs& args) { + printf("BeginMainFrame(%d)\n", frame); compositor_deps_->GetRendererScheduler()->WillBeginFrame(args); double frame_time_sec = (args.frame_time - base::TimeTicks()).InSecondsF(); delegate_->BeginMainFrame(frame_time_sec);
,
Mar 28 2017
At the bisected commit the stack trace looks like: #0 0x7fb3b2775067 base::debug::StackTrace::StackTrace() #1 0x7fb3b388e34c content::RenderWidgetCompositor::setNeedsBeginFrame() #2 0x7fb3af4851fd blink::ChromeClientImpl::scheduleAnimation() #3 0x7fb3ae7d50cc blink::PaintLayerCompositor::setNeedsCompositingUpdate() #4 0x7fb3ae941f3a blink::PaintLayer::attemptDirectCompositingUpdate() #5 0x7fb3ae941faa blink::PaintLayer::styleDidChange() #6 0x7fb3ae6e2784 blink::LayoutBoxModelObject::styleDidChange() #7 0x7fb3ae6c4de8 blink::LayoutBox::styleDidChange() #8 0x7fb3ae69539d blink::LayoutBlock::styleDidChange() #9 0x7fb3ae6ae521 blink::LayoutBlockFlow::styleDidChange() #10 0x7fb3ae73e546 blink::LayoutObject::setStyle() #11 0x7fb3ae12bcdc blink::Element::recalcOwnStyle() #12 0x7fb3ae12b658 blink::Element::recalcStyle() #13 0x7fb3ae0b579a blink::ContainerNode::recalcDescendantStyles() #14 0x7fb3ae12b712 blink::Element::recalcStyle() #15 0x7fb3ae0b579a blink::ContainerNode::recalcDescendantStyles() #16 0x7fb3ae12b712 blink::Element::recalcStyle() #17 0x7fb3ae0e6a37 blink::Document::updateStyle() #18 0x7fb3ae0e25d5 blink::Document::updateStyleAndLayoutTree() #19 0x7fb3ae0e7fa9 blink::Document::updateStyleAndLayoutTreeIgnorePendingStylesheets() #20 0x7fb3ae0e7a1d blink::Document::updateStyleAndLayoutIgnorePendingStylesheets() #21 0x7fb3ae24221c blink::FrameSelection::selection() #22 0x7fb3ae24e2db blink::InputMethodController::textInputType() #23 0x7fb3b3944ded content::RenderWidget::GetTextInputType() #24 0x7fb3b3941dae content::RenderWidget::UpdateTextInputStateInternal() #25 0x7fb3b3940ef3 content::RenderWidget::WillBeginCompositorFrame() #26 0x7fb3b0fcf837 cc::ProxyMain::BeginMainFrame() Removing the call to document().updateStyleAndLayoutIgnorePendingStylesheets() in FrameSelection::selection() fixes the issue.
,
Mar 28 2017
Confirmed that removing the line document().updateStyleAndLayoutIgnorePendingStylesheets(); from FrameSelection::computeVisibleSelectionInDOMTreeDeprecated() resolves the issue in ToT. I looked around at perf tests for a dip in performance from this change. Animation tests seem to maintain 60Hz with it which is an unfortunate indication of our animation perf coverage here. I expect to see a battery usage regression, the closest perf tests I've found so far are: https://chromeperf.appspot.com/report?sid=317afe784ada8df20190dbfe4fe12ded5bf71099a41d21fbe96748637d044e98&start_rev=453053&end_rev=453529 ChromiumPerf/chromium-rel-mac-retina/battor.trivial_pages / cpu_time_percentage_avg ChromiumPerf/android-nexus5X/battor.power_cases / cpu_time_percentage_avg Unfortunately their data points start at 2017-02-25 while the regression patch was from 2017-02-12. Started a perf run: https://codereview.chromium.org/2778773003
,
Mar 28 2017
The relevant page for battor.trivial_pages is https://webkit.org/blog-files/3d-transforms/poster-circle.html and for battor.power_cases it's https://cs.chromium.org/chromium/src/tools/perf/page_sets/trivial_sites/trivial_blur_animation.html.
,
Mar 28 2017
It is OK that "begin compositor frame" with dirty layout?
There are two call sites call updateLayoutXX() in InputMethodController::textInputType():
if (!frame()
.selection()
.computeVisibleSelectionInDOMTreeDeprecated()
.rootEditableElement())
return WebTextInputTypeNone;
// This can change not to call updateLayoutXX()
and
document().updateStyleAndLayoutTree();
if (hasEditableStyle(*element))
return WebTextInputTypeContentEditable;
Since hasEditableStyle() uses -webkit-user-modify CSS property, it should be
at least StyleClean.
Even if we change both call sites not to call updateLayoutXXX(), selection painting
requires clean layout.
,
Mar 28 2017
Note: A comment in RenderWidget::WillBeginCompositorFrame() says UpdateTextInputState() can update layout.
void RenderWidget::WillBeginCompositorFrame() {
TRACE_EVENT0("gpu", "RenderWidget::willBeginCompositorFrame");
// The UpdateTextInputState can result in further layout and possibly
// enable GPU acceleration so they need to be called before any painting
// is done.
UpdateTextInputState();
UpdateSelectionBounds();
,
Mar 28 2017
Maybe it's a stupid question. I'm wondering why calling setNeedsBeginFrame() inside BeginMainFrame() isn't a no-op? Or what are the operations in BeginMainFrame() that requires another BeginMainFrame()? To me it's a little bit weird that updating layout in BeginMainFrame() triggers another frame, since BeginMainFrame() updates full lifecycle anyways.
,
Mar 28 2017
That's how requestAnimationFrame works, another call schedules another frame. Note that inside blink we do actually have two paths for this. PageAnimator::scheduleVisualUpdate is a no-op if a frame is already scheduled. PaintLayerCompositor::setNeedsCompositingUpdate() is skipping all that and going directly to scheduleAnimation() like raf which seems like a bug. I think there's an animation engine bug/flaw here though that anything that forces a recalc on main then results in a frame being scheduled, so for example: setInterval(() => document.body.offsetTop; }, 16); document.body.className = "composited-animation"; Would make us pump the main thread BeginMainFrame at 60fps even though the animation is running on the compositor. I have vague memories of discussing this with shane many years ago, we should probably fix it. Causing a style recalc on main should not force the animation to pump on main, the animation engine should understand where the animation is actually running, fast forward if needed, and skip scheduling frames.
,
Mar 29 2017
I believe what's happening here is that animation effects marking elements as having a dirty style which inadvertently requests a new animation frame: #1 0x7efca108342d content::RenderWidgetCompositor::setNeedsBeginFrame() #2 0x7efc9c9504ed blink::ChromeClientImpl::scheduleAnimation() #3 0x7efc9b57d9e3 blink::Document::scheduleLayoutTreeUpdate() #4 0x7efc9b615655 blink::Node::markAncestorsWithChildNeedsStyleRecalc() #5 0x7efc9b615982 blink::Node::setNeedsStyleRecalc() #6 0x7efc9b5c6cd5 blink::Element::setNeedsAnimationStyleRecalc() #7 0x7efc9b371dc5 blink::KeyframeEffectReadOnly::applyEffects() The animation engine already deals with scheduling the next frame via AnimationTimeline::scheduleNextService() which will be a noop for composited animations. I think applyEffects() just needs a way of saying that the style has changed for an element without requesting that we trigger a full pipeline update.
,
Mar 29 2017
In general we don't want to allow anyone to create dirtiness that doesn't schedule a frame since it'd never show up on the screen and we spent years fixing those bugs. One thing we could do is have a bit that tracks if we're inside updateStyleAndLayoutTree(), and avoid scheduling frames for calls to setNeedsStyleRecalc() in the steps at the start since we know a call to updateStyle() right after will clear the bits anyway. This is the same concept as what LayoutObject::markContainerChainForLayout is doing to avoid scheduling a relayout from inside layout.
,
Mar 29 2017
Unfortunately the suggested fix in #20 isn't sufficient for avoiding the main thread pumping frames. The animation engine dirties the style because it has changed. The change to style causes paint to trigger a new frame. See stack trace: #0 0x7efc9ff37477 base::debug::StackTrace::StackTrace() #1 0x7efca108342d content::RenderWidgetCompositor::setNeedsBeginFrame() #2 0x7efc9c9504ed blink::ChromeClientImpl::scheduleAnimation() #3 0x7efc9bc876ef blink::PaintLayerCompositor::setNeedsCompositingUpdate() #4 0x7efc9bdf68e8 blink::PaintLayer::attemptDirectCompositingUpdate() #5 0x7efc9bdf695a blink::PaintLayer::styleDidChange() #6 0x7efc9bb8f3ad blink::LayoutBoxModelObject::styleDidChange() #7 0x7efc9bb71068 blink::LayoutBox::styleDidChange() #8 0x7efc9bb40dcd blink::LayoutBlock::styleDidChange() #9 0x7efc9bb59df1 blink::LayoutBlockFlow::styleDidChange() #10 0x7efc9bbec8e4 blink::LayoutObject::setStyle() #11 0x7efc9b5c5d69 blink::Element::recalcOwnStyle() #12 0x7efc9b5c5627 blink::Element::recalcStyle() #13 0x7efc9b54d7dc blink::ContainerNode::recalcDescendantStyles() #14 0x7efc9b5c5512 blink::Element::recalcStyle() #15 0x7efc9b54d7dc blink::ContainerNode::recalcDescendantStyles() #16 0x7efc9b5c5512 blink::Element::recalcStyle() #17 0x7efc9b57fb6e blink::Document::updateStyle() #18 0x7efc9b57b392 blink::Document::updateStyleAndLayoutTree() #19 0x7efc9b581137 blink::Document::updateStyleAndLayoutTreeIgnorePendingStylesheets() #20 0x7efc9b580b6d blink::Document::updateStyleAndLayoutIgnorePendingStylesheets() #21 0x7efc9b6e59cc blink::FrameSelection::computeVisibleSelectionInDOMTreeDeprecated() #22 0x7efc9b6f0b0b blink::InputMethodController::textInputType() #23 0x7efca113f8fd content::RenderWidget::GetTextInputType() #24 0x7efca113c7be content::RenderWidget::UpdateTextInputStateInternal() #25 0x7efca113b8f3 content::RenderWidget::WillBeginCompositorFrame() #26 0x7efc9e50ef19 cc::ProxyMain::BeginMainFrame() My first thought is to somehow plum the fact that the change was originally caused by KeyframeEffectReadOnly::applyEffect() through to avoid the call to scheduleVisualUpdate() since animations has its own mechanism for requesting that. esprehn suggested disabling setNeedsBeginFrame() for WillBeginCompositorFrame() as a workaround for this case. The setInterval() example in #18 would still be running main thread so it's not a general solution.
,
Mar 29 2017
Both calls to setNeedsBeginFrame() is done via page()->animator().scheduleVisualUpdate(frame()). This already has logic for suppressing frame requests for known states:
void PageAnimator::scheduleVisualUpdate(LocalFrame* frame) {
if (m_servicingAnimations || m_updatingLayoutAndStyleForPainting)
return;
m_page->chromeClient().scheduleAnimation(frame->view());
}
Perhaps this could be extended to include something like m_animationOnlyChange though I fear that the bugs mentioned in #20 would become very easy to crop up again if this bit of state is mismanaged. The behaviour of the existing bools are tied to stack lifetimes and are very easy to reason about. A bool that's turned on/off in response to various changes seems much more likely to mess up over time.
,
Mar 29 2017
While discussing this issue with @tansell we came to the conclusion that if we are processing a frame update then things like setNeedsStyleRecalc() should not end up requesting a new frame. Ideally they would set a bit to say "a visual update is required" and upon completing a frame's processing those bits would be cleared and no subsequent frame would get triggered. It's a "needs visual update" request which may get satisfied immediately if triggered in the context of a visual update. Animation frame requests are different as they are anticipating needing a visual update after some passage of time (Animation::timeToEffectChange()) so they would not get handled in the same way.
,
Mar 29 2017
I think what we want to do is move the call to WillBeginMainFrame() here: https://cs.chromium.org/chromium/src/cc/trees/proxy_main.cc?l=178 to before we change the pipeline stage here: https://cs.chromium.org/chromium/src/cc/trees/proxy_main.cc?l=168 so that calls to scheduleAnimation() inside that setup phase don't schedule another future frame, which isn't needed anyway since we're about to run the frame steps anyway.
,
Mar 30 2017
I change InputMethodController::textTypeInof() not to call update layout, since there is no selection, we return before second update layout call. However, I still see RenderWidgetCompositor::BeginMainFrame() call... [1] http://crrev.com/2782413002: Get rid of computeVisibleSelectionInDOMTreeDeprecated() in InputMethodController::textInputType()
,
Mar 30 2017
alancutter@ and I worked together and we found culprit call sites. Two patch [1] and [2] get rid of redundant update layout call from in this call stack. These call sites used deprecated function and editing team wants to get rid of them too. alancutter@ will introduce perf test to catch regression of opacity animation. [1] http://crrev.com/2782413002: Get rid of computeVisibleSelectionInDOMTreeDeprecated() in InputMethodController::textInput{Info,Type}() [2] http://crrev.com/2784243002: Get rid of computeVisibleSelectionInDOMTreeDeprecated() from WebViewImpl::selectionBounds()
,
Mar 30 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/9744d5bffb3fbdf761455b355ebc4faa757d1aaf commit 9744d5bffb3fbdf761455b355ebc4faa757d1aaf Author: yosin <yosin@chromium.org> Date: Thu Mar 30 09:02:53 2017 Get rid of computeVisibleSelectionInDOMTreeDeprecated() in InputMethodController::textInputType() and textInputInfo() This patch gets rid of |computeVisibleSelectionInDOMTreeDeprecated()| by replacing it with |rootEditableElementOf()| with non-canonicalize position from |textInputInfo()| and |textInputType()| of |InputMethodController| for improving code health. Node: Because of selection can't cross editing boundary, root editable element for non-canonicalized position and canonicalized position are same. This patch is intentionally not change all call sites of |rootEditableElement()| in |InputMethodController| to merge this patch as workaround of [1]. [1] http://crbug.com/704763 Opacity animation consuming main thread resources BUG=698633, 704763 TEST=n/a; no behavior changes Review-Url: https://codereview.chromium.org/2782413002 Cr-Commit-Position: refs/heads/master@{#460705} [modify] https://crrev.com/9744d5bffb3fbdf761455b355ebc4faa757d1aaf/third_party/WebKit/Source/core/editing/InputMethodController.cpp
,
Mar 30 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/8202d740b1c09787cdd3e11d3d15e7211b88f15e commit 8202d740b1c09787cdd3e11d3d15e7211b88f15e Author: yosin <yosin@chromium.org> Date: Thu Mar 30 09:19:49 2017 Get rid of computeVisibleSelectionInDOMTreeDeprecated() from WebViewImpl::selectionBounds() This patch gets rid of |computeVisibleSelectionInDOMTreeDeprecated()| by moving |updateStyleAndLayoutIgnorePendingStylesheets()| before using |VisibleSelection| and checking DOM based empty-selection before updating layout. When selection is empty in DOM tree, e.g. selection.base().isNull(), it can not be non-null on |VisibleSelection|. This patch is a part of workaround of [1] for merging. [1] http://crbug.com/704763 Opacity animation consuming main thread resources BUG=698633, 704763 TEST=n/a; no behavior changes Review-Url: https://codereview.chromium.org/2784243002 Cr-Commit-Position: refs/heads/master@{#460710} [modify] https://crrev.com/8202d740b1c09787cdd3e11d3d15e7211b88f15e/third_party/WebKit/Source/web/WebViewImpl.cpp
,
Mar 30 2017
I'm mostly confused why there's style being dirtied on the main thread during the animation at all. What esprehn suggests in #18 about not running the animation at all on the main thread sounds like the right top level solution here. For what it's worth, the different pipeline stages in ProxyMain reflect different levels of update "severity". LayerTreeHost has a bunch of comments about it, but roughly cc update sanimations (begin main frame), then layers, then does a commit. You can LayerTreeHost::SetNeeds(Animate|UpdateLayers|Commit) to force the update pipeline to get to a particular stage. There's some edge cases, but Blink only SetNeedsAnimate directly or makes property changes that cause SetNeedsUpdateLayers or SetNeedsCommit. It is currently the case that calling most of these (other than the special cases of update layers or commit during the animate stage) will cause another begin frame. This is not very clear to callers. I agree that it sounds like we need another interface here for Blink to be able to SetNeedsAnimate (if not already doing) and also some request another begin frame (even if already doing it). I think this would cover the callstack with updateAllLifecyclePhases in it to not generate a second begin frame. The other sketchy thing going on here is cc::ProxyMain::SetAnimationEvents() which appears to be sent independently of a begin frame. Really, these animation events should be delivered with the begin frame and executed prior to rAF. This is stuff that loyso@ was in progress of working on, although he's out for a while and we don't really have anybody working in the animation area. If this is causing extra begin frames as well, it seems like we should deliver those events with a begin frame so that they fit into the updateAllLifecyclePhases case.
,
Mar 30 2017
I've seen reports of main frames happening during transform animations in the UI as well, though haven't solidly confirmed that there's not something else dirtying the main-thread-side structures to cause the frames.
,
Mar 30 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/dc19a899c0f97dcd259a3ec506396c43db14ef23 commit dc19a899c0f97dcd259a3ec506396c43db14ef23 Author: sullivan <sullivan@chromium.org> Date: Thu Mar 30 22:09:15 2017 Revert of Get rid of computeVisibleSelectionInDOMTreeDeprecated() in InputMethodController (patchset #4 id:60001 of https://codereview.chromium.org/2782413002/ ) Reason for revert: This causes a crash in a telemetry benchmark on android. To reproduce, run: src/tools/perf/run_benchmark -v --browser=android-chromium --output-format=chartjson --upload-results --pageset-repeat=1 --also-run-disabled-tests --story-filter=browse:media:youtube system_health.common_mobile BUG= 706865 Original issue's description: > Get rid of computeVisibleSelectionInDOMTreeDeprecated() in InputMethodController::textInputType() and textInputInfo() > > This patch gets rid of |computeVisibleSelectionInDOMTreeDeprecated()| by replacing > it with |rootEditableElementOf()| with non-canonicalize position from > |textInputInfo()| and |textInputType()| of |InputMethodController| for improving > code health. > > Node: Because of selection can't cross editing boundary, root editable element for > non-canonicalized position and canonicalized position are same. > > This patch is intentionally not change all call sites of |rootEditableElement()| > in |InputMethodController| to merge this patch as workaround of [1]. > > [1] http://crbug.com/704763 Opacity animation consuming main thread resources > > BUG=698633, 704763 > TEST=n/a; no behavior changes > > Review-Url: https://codereview.chromium.org/2782413002 > Cr-Commit-Position: refs/heads/master@{#460705} > Committed: https://chromium.googlesource.com/chromium/src/+/9744d5bffb3fbdf761455b355ebc4faa757d1aaf TBR=tkent@chromium.org,xiaochengh@chromium.org,yoichio@chromium.org,yosin@chromium.org # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG=698633, 704763 Review-Url: https://codereview.chromium.org/2785333002 Cr-Commit-Position: refs/heads/master@{#460903} [modify] https://crrev.com/dc19a899c0f97dcd259a3ec506396c43db14ef23/third_party/WebKit/Source/core/editing/InputMethodController.cpp
,
Mar 31 2017
@sullivan re. #31, could we get a link to the log on the failing bot?
,
Mar 31 2017
Here are a few log links: https://luci-logdog.appspot.com/v/?s=chrome%2Fbb%2Fchromium.perf%2FAndroid_Nexus5_Perf__2_%2F5327%2F%2B%2Frecipes%2Fsteps%2Fsymbolized_breakpad_crashes%2F0%2Fstdout https://luci-logdog.appspot.com/v/?s=chrome%2Fbb%2Fchromium.perf%2FAndroid_Nexus5_Perf__2_%2F5327%2F%2B%2Frecipes%2Fsteps%2Fsystem_health.common_mobile%2F0%2Fstdout summary of bug 706836 is the most relevant snippet.
,
Apr 3 2017
Re #28: > I'm mostly confused why there's style being dirtied on the main thread during the animation at all. What esprehn suggests in #18 about not running the animation at all on the main thread sounds like the right top level solution here. Animation effects are visible to Javascript. We do main thread animation computation whenever JS tries to read something that could be affected by animations. -------- I have a design doc for extending ProxyMain's interface to accommodate Blink's need for a distinction between "I need a frame" vs "I need the next frame". I think this might serve as a permanent solution for this issue. https://docs.google.com/document/d/1A22HVDbVh2xhPU0OeXVWTSAriZMsd_rH5seO7xIuzCI As for the regression in Beta we need a quick fix that is suitable for merging . I was hoping on yosin's changes to be mergable but with the crash on the perf bot that doesn't seem likely. I have an alternate quick fix written up intended for merging to Beta (inspired by enne's "whack-a-mole" description): https://codereview.chromium.org/2794803002
,
Apr 3 2017
I've been pondering this and I think the system level problem is that the animation engine becomes passively dirty without doing anything explicit to dirty the animation state. Everything else that goes through the updateStyle*() system does: MarkDirty() -> ScheduleVisualUpdate() -> BeginFrame happens or a forced recalc. But animations checks the current time to see if things have become dirty: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/animation/AnimationTimeline.cpp?type=cs&q=currentTimeInternal&l=210 which means it can become passively dirty without ever marking anything dirty. Nothing else works this way, and it's the reason animations ends up in this funny situation where it's scheduling frames inside the recalc whereas everything else inside recalc assumes frames have already been scheduled. Can we figure out how to make the animation system actively dirty the system instead when the time changes? https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/animation/AnimationClock.cpp?dr=CSs&l=55 I know part of the problem here is that we're trying to avoid computing style unless the main thread forces... but that's also the thing that triggers this system design mismatch. Another alternative might be to teach BeginMainFrame to always mark the visual state update bit at the start, so it would do: WillBeginMainFrame() { // call into blink and do PageAnimator::willBeginMainFrame() // which disables all calls to scheduleVisualUpdate() until // a call to PageAnimator::updateAllLifecyclePhases(). GetWebWidget()->willBeginFrame(); } PageAnimator::willBeginFrame() { m_visualUpdateSchedulingEnabled = false; } PageAnimator::updateAllLifecyclePhases() { ... do stuff ... m_visualUpdateSchedulingEnabled = true; } and remove the two bools inside PageAnimator, since now the bool marked by the call to ::willBeginFrame and cleared by the call to PageAnimator::updateAllLifecyclePhases right at the end will handle it for you. That should implicitly fix the animation bug discussed here. The former sounds like a good thought exercise, perhaps there's something there. The latter seems like something small we could merge. It's mostly plumbing up through the public API.
,
Apr 3 2017
My objective right now is to find a fix that can be merged into Beta. Unfortunately https://codereview.chromium.org/2794803002 will probably cause too much collateral damage to be suitable. @enne: I don't have any experience with ProxyMain & friends, do you think it's possible to make the fix described in the design doc (#34) in a way that's suitable for merging to Beta? The thing that makes me think otherwise is the considerable amount of subclass implementations that will be affected by the changes in the multiple abstract interfaces. My local half complete WIP implementation touches 27 files already!
,
Apr 3 2017
#35: > Can we figure out how to make the animation system actively dirty the system instead when the time changes? This is another issue in the animation engine in that it reads system time instead of just using time that the scheduler has provided. This is due to the scenario when the animation engine is invoked by Javascript while in the background and the last provided frame time has become arbitrarily stale. Ideally this background execution would be controlled by the scheduler and come with scheduler produced times for the animation engine to use voiding the need to look at the system clock. > Another alternative might be to teach BeginMainFrame to always mark the visual state update bit at the start... Thanks, giving this a shot!
,
Apr 3 2017
,
Apr 3 2017
,
Apr 3 2017
Personally I'm ok with either solution here. I think it's reasonable to have cc distinguish "request a new frame even if in the middle of a frame" and "request a new frame unless in the middle of a new frame" behavior. It already has to do this same sort of thing on the compositor thread, and I suspect ui has to deal with this too. So, I think that's a good path forward long term. If it makes more sense in the short term to just suppress more update calls from Blink, I think that can work too, although I worry longer term that folks adding more calls outside of the lifecycle update will cause similar bugs.
,
Apr 4 2017
>#c36 alan@, could you see [1]? If we find a workaround of it, we can commit my patch[2]. [1] http://crbug.com/372245 LayoutTextTrackContainer::layout() should not modify DOM tree [2] http://crrev.com/2782413002 Get rid of computeVisibleSelectionInDOMTreeDeprecated() in InputMethodController::textInputType() and textInputInfo()
,
Apr 5 2017
#41: Currently working on https://codereview.chromium.org/2791223002 as a workaround patch based on the suggestion in #35. I think it's fine if we work on these different workarounds in parallel.
,
Apr 5 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/317a8e0e26459f401312043fa99ed8e17b1b24a4 commit 317a8e0e26459f401312043fa99ed8e17b1b24a4 Author: alancutter <alancutter@chromium.org> Date: Wed Apr 05 10:09:29 2017 Notify Blink to suppress frame requests during BeginMainFrame This change fixes a performance regression where main thread frames were being continuously requested during composited animations. This is a temporary workaround that tells Blink when BeginMainFrame is active and it should not be requesting new frames for visual updates. BUG= 704763 Review-Url: https://codereview.chromium.org/2791223002 Cr-Commit-Position: refs/heads/master@{#462022} [modify] https://crrev.com/317a8e0e26459f401312043fa99ed8e17b1b24a4/content/renderer/render_widget.cc [modify] https://crrev.com/317a8e0e26459f401312043fa99ed8e17b1b24a4/third_party/WebKit/Source/core/page/PageAnimator.cpp [modify] https://crrev.com/317a8e0e26459f401312043fa99ed8e17b1b24a4/third_party/WebKit/Source/core/page/PageAnimator.h [modify] https://crrev.com/317a8e0e26459f401312043fa99ed8e17b1b24a4/third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp [modify] https://crrev.com/317a8e0e26459f401312043fa99ed8e17b1b24a4/third_party/WebKit/Source/web/WebFrameWidgetImpl.h [modify] https://crrev.com/317a8e0e26459f401312043fa99ed8e17b1b24a4/third_party/WebKit/Source/web/WebPagePopupImpl.cpp [modify] https://crrev.com/317a8e0e26459f401312043fa99ed8e17b1b24a4/third_party/WebKit/Source/web/WebPagePopupImpl.h [modify] https://crrev.com/317a8e0e26459f401312043fa99ed8e17b1b24a4/third_party/WebKit/Source/web/WebViewFrameWidget.cpp [modify] https://crrev.com/317a8e0e26459f401312043fa99ed8e17b1b24a4/third_party/WebKit/Source/web/WebViewFrameWidget.h [modify] https://crrev.com/317a8e0e26459f401312043fa99ed8e17b1b24a4/third_party/WebKit/Source/web/WebViewImpl.cpp [modify] https://crrev.com/317a8e0e26459f401312043fa99ed8e17b1b24a4/third_party/WebKit/Source/web/WebViewImpl.h [modify] https://crrev.com/317a8e0e26459f401312043fa99ed8e17b1b24a4/third_party/WebKit/public/web/WebWidget.h
,
Apr 6 2017
Requesting to merge 317a8e0e26459f401312043fa99ed8e17b1b24a4. Impact: This change fixes a ~10% CPU usage increase for composited animations. The regression has gotten some public attention: https://www.theregister.co.uk/2017/03/23/cursor_devours_cpu_cycles_ms_code_editor/
,
Apr 6 2017
Did you test with this patch to see how much better it is for CPU usage?
,
Apr 6 2017
I didn't and should have, my ~10% was from prior investigations of the bug. I've tested the repro case in #0 locally on Linux desktop looking at Chrome's task manager. With the patch I see a consistent 6-9% CPU usage from the tab. Without the patch I see 13-20% CPU usage from the tab for 0-30 seconds after which it drops down to 6-9%. From inspection the drop corresponds to main thread frames ceasing to be requested, this behaviour is not well understood.
,
Apr 6 2017
This bug requires manual review: Reverts referenced in bugdroid comments after merge request. Please contact the milestone owner if you have questions. Owners: amineer@(Android), cmasso@(iOS), bhthompson@(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Apr 6 2017
Before we approve merge to M58, could you please confirm 317a8e0e26459f401312043fa99ed8e17b1b24a4 change is well baked/verified in Canary, having enough automation coverage and safe to merge?
,
Apr 10 2017
Re #48: This has had some time to back in Canary now and I believe is ready to merge. Haven't seen any issues relating to 317a8e0e26459f401312043fa99ed8e17b1b24a4 from the wild. There were several tests that caught issues in 317a8e0e26459f401312043fa99ed8e17b1b24a4 while attempting to land it, that gives me confidence of the coverage of this change beyond its intended purpose. There was one perf regression covered by issue 709674 however the perf change was just part of undoing the effects of the regression commit and is WAI.
,
Apr 10 2017
Approving merge to M58 branch 3029 based on comment #49. Please merge ASAP. Thank you.
,
Apr 11 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/09fcbb12123dc079d446c48158ab228d8b2f5952 commit 09fcbb12123dc079d446c48158ab228d8b2f5952 Author: Alan Cutter <alancutter@chromium.org> Date: Tue Apr 11 04:17:52 2017 Notify Blink to suppress frame requests during BeginMainFrame This change fixes a performance regression where main thread frames were being continuously requested during composited animations. This is a temporary workaround that tells Blink when BeginMainFrame is active and it should not be requesting new frames for visual updates. BUG= 704763 Review-Url: https://codereview.chromium.org/2791223002 Cr-Commit-Position: refs/heads/master@{#462022} (cherry picked from commit 317a8e0e26459f401312043fa99ed8e17b1b24a4) Review-Url: https://codereview.chromium.org/2813543003 . Cr-Commit-Position: refs/branch-heads/3029@{#662} Cr-Branched-From: 939b32ee5ba05c396eef3fd992822fcca9a2e262-refs/heads/master@{#454471} [modify] https://crrev.com/09fcbb12123dc079d446c48158ab228d8b2f5952/content/renderer/render_widget.cc [modify] https://crrev.com/09fcbb12123dc079d446c48158ab228d8b2f5952/third_party/WebKit/Source/core/page/PageAnimator.cpp [modify] https://crrev.com/09fcbb12123dc079d446c48158ab228d8b2f5952/third_party/WebKit/Source/core/page/PageAnimator.h [modify] https://crrev.com/09fcbb12123dc079d446c48158ab228d8b2f5952/third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp [modify] https://crrev.com/09fcbb12123dc079d446c48158ab228d8b2f5952/third_party/WebKit/Source/web/WebFrameWidgetImpl.h [modify] https://crrev.com/09fcbb12123dc079d446c48158ab228d8b2f5952/third_party/WebKit/Source/web/WebPagePopupImpl.cpp [modify] https://crrev.com/09fcbb12123dc079d446c48158ab228d8b2f5952/third_party/WebKit/Source/web/WebPagePopupImpl.h [modify] https://crrev.com/09fcbb12123dc079d446c48158ab228d8b2f5952/third_party/WebKit/Source/web/WebViewFrameWidget.cpp [modify] https://crrev.com/09fcbb12123dc079d446c48158ab228d8b2f5952/third_party/WebKit/Source/web/WebViewFrameWidget.h [modify] https://crrev.com/09fcbb12123dc079d446c48158ab228d8b2f5952/third_party/WebKit/Source/web/WebViewImpl.cpp [modify] https://crrev.com/09fcbb12123dc079d446c48158ab228d8b2f5952/third_party/WebKit/Source/web/WebViewImpl.h [modify] https://crrev.com/09fcbb12123dc079d446c48158ab228d8b2f5952/third_party/WebKit/public/web/WebWidget.h
,
Apr 11 2017
I intend to work on the proper fix for this issue in 20% time now that the fire's out.
,
Apr 11 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/e14aacd2baf5cf9c4a5e457384e0b0f73c0163c4 commit e14aacd2baf5cf9c4a5e457384e0b0f73c0163c4 Author: yosin <yosin@chromium.org> Date: Tue Apr 11 08:33:37 2017 Get rid of computeVisibleSelectionInDOMTreeDeprecated() in InputMethodController::textInputType() and textInputInfo() This patch gets rid of |computeVisibleSelectionInDOMTreeDeprecated()| by replacing it with |rootEditableElementOf()| with non-canonicalize position from |textInputInfo()| and |textInputType()| of |InputMethodController| for improving code health. Node: Because of selection can't cross editing boundary, root editable element for non-canonicalized position and canonicalized position are same. This patch is intentionally not change all call sites of |rootEditableElement()| in |InputMethodController| to merge this patch as workaround of [1]. [1] http://crbug.com/704763 Opacity animation consuming main thread resources BUG=698633, 704763 TEST=n/a; no behavior changes Review-Url: https://codereview.chromium.org/2782413002 Cr-Original-Commit-Position: refs/heads/master@{#460705} Committed: https://chromium.googlesource.com/chromium/src/+/9744d5bffb3fbdf761455b355ebc4faa757d1aaf Review-Url: https://codereview.chromium.org/2782413002 Cr-Commit-Position: refs/heads/master@{#463575} [modify] https://crrev.com/e14aacd2baf5cf9c4a5e457384e0b0f73c0163c4/third_party/WebKit/Source/core/editing/InputMethodController.cpp
,
Apr 12 2017
,
Apr 12 2017
,
Apr 12 2017
Converted label to hotlist
,
Apr 12 2017
Tried testing the issue on Win-10 using chrome beta version #58.0.3029.68. Following are the steps followed to reproduce the issue. ------------ 1. Opened the attached caret.html in chrome and observed a black square box blinking. alancutter@ - Could you please provide manual repro steps, expected and actual result to test this issue from TE-end. Thanks...!! |
||||||||||||||||||||||||
►
Sign in to add a comment |
||||||||||||||||||||||||
Comment 1 by alancutter@chromium.org
, Mar 23 2017