Notify Blink to suppress frame requests during BeginMainFrame

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}
Committed: https://chromium.googlesource.com/chromium/src/+/317a8e0e26459f401312043fa99ed8e17b1b24a4

[alancutter (OOO until 2018), 2017-04-03 13:11:36]

alancutter@chromium.org changed reviewers:
+ esprehn@chromium.org

[alancutter (OOO until 2018), 2017-04-03 13:11:36]

This is an implementation of the suggestion in
https://bugs.chromium.org/p/chromium/issues/detail?id=704763#c35.
This patch deviates from it slightly in that the suppression flag is unset by
RenderWidget::DidCommitCompositorFrame() instead of
PageAnimator::updateAllLifecyclePhases().
It was much harder to reason that updateAllLifecyclePhases() was always going to
execute in a BeginMainFrame than it was to reason about
DidCommitCompositorFrame().

[alancutter (OOO until 2018), 2017-04-03 23:53:22]

The CQ bit was checked by alancutter@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-03 23:53:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-04 00:57:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-04 00:57:37]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[esprehn, 2017-04-04 06:07:15]

Those test failures look real:

	blink::WebPagePopupImpl::setSuppressFrameRequestsWorkaroundFor704763Only
[0x034C58E3+19]
	content::RenderWidget::WillBeginCompositorFrame [0x03F2133F+111]
	cc::ProxyMain::BeginMainFrame [0x027E8BCF+1231]
	base::internal::Invoker<base::internal::BindState<void (__thiscall
cc::ProxyMain::*)(std::unique_ptr<cc::BeginMainFrameAndCommitState,std::default_delete<cc::BeginMainFrameAndCommitState>
>),base::WeakPtr<cc::ProxyMain>,base::internal::PassedWrapper<std::
[0x027F461C+124]
	base::internal::Invoker<base::internal::BindState<void (__thiscall
cc::ProxyMain::*)(std::unique_ptr<cc::BeginMainFrameAndCommitState,std::default_delete<cc::BeginMainFrameAndCommitState>
>),base::WeakPtr<cc::ProxyMain>,base::internal::PassedWrapper<std::
[0x027F6B76+22]
	base::debug::TaskAnnotator::RunTask [0x01C24349+409]
	blink::scheduler::TaskQueueManager::ProcessTaskFromWorkQueue [0x0342CB9E+919]
	blink::scheduler::TaskQueueManager::DoWork [0x0342B9B1+625]
	base::internal::FunctorTraits<void (__thiscall
content::WebMediaPlayerMS::*)(bool),void>::Invoke<base::WeakPtr<content::WebMediaPlayerMS>
const &,bool const &> [0x0323A531+26]
	base::internal::InvokeHelper<1,void>::MakeItSo<void (__thiscall
content::WebMediaPlayerMS::*const
&)(bool),base::WeakPtr<content::WebMediaPlayerMS> const &,bool const &>
[0x0323A5DC+34]
	base::internal::Invoker<base::internal::BindState<void (__thiscall
content::WebMediaPlayerMS::*)(bool),base::WeakPtr<content::WebMediaPlayerMS>,bool>,void
__cdecl(void)>::Run [0x04960F27+23]

[esprehn, 2017-04-04 06:13:55]

https://codereview.chromium.org/2791223002/diff/20001/content/renderer/render...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/2791223002/diff/20001/content/renderer/render...
content/renderer/render_widget.cc:916:
GetWebWidget()->setSuppressFrameRequestsWorkaroundFor704763Only(false);
I think should be in RenderWidget::UpdateVisualState() right after the call to
updateAllLifecyclePhases, I don't think it's safe to suppress updates after that
call, it'd leave unscheduled work.

Doing it between WillBeginCompositorFrame and UpdateVisualState means it's
between these lines:

https://cs.chromium.org/chromium/src/cc/trees/proxy_main.cc?type=cs&l=178

and

https://cs.chromium.org/chromium/src/cc/trees/proxy_main.cc?type=cs&l=189

which seems okay.

https://codereview.chromium.org/2791223002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebPagePopupImpl.cpp (right):

https://codereview.chromium.org/2791223002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebPagePopupImpl.cpp:399: bool
suppressFrameRequests) {
I think you need to null check the page just like below?

[alancutter (OOO until 2018), 2017-04-04 09:54:31]

https://codereview.chromium.org/2791223002/diff/20001/content/renderer/render...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/2791223002/diff/20001/content/renderer/render...
content/renderer/render_widget.cc:916:
GetWebWidget()->setSuppressFrameRequestsWorkaroundFor704763Only(false);
On 2017/04/04 at 06:13:55, esprehn wrote:
> I think should be in RenderWidget::UpdateVisualState() right after the call to
updateAllLifecyclePhases, I don't think it's safe to suppress updates after that
call, it'd leave unscheduled work.

Fair call, moving the unset back to suggested place.

> Doing it between WillBeginCompositorFrame and UpdateVisualState means it's
between these lines:
> 
> https://cs.chromium.org/chromium/src/cc/trees/proxy_main.cc?type=cs&l=178
> 
> and
> 
> https://cs.chromium.org/chromium/src/cc/trees/proxy_main.cc?type=cs&l=189
> 
> which seems okay.

Keeping a DCHECK there to catch if it's ever missed.

https://codereview.chromium.org/2791223002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebPagePopupImpl.cpp (right):

https://codereview.chromium.org/2791223002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebPagePopupImpl.cpp:399: bool
suppressFrameRequests) {
On 2017/04/04 at 06:13:55, esprehn wrote:
> I think you need to null check the page just like below?

Ty, done.

[alancutter (OOO until 2018), 2017-04-04 09:54:44]

The CQ bit was checked by alancutter@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-04 09:54:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-04 10:22:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-04 10:22:25]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[esprehn, 2017-04-04 18:22:27]

Real failures, I think you want to hook RenderWidget::UpdateVisualState?

[4680:3852:0404/033359.593:9678801:FATAL:pageanimator.cpp(77)] Check failed:
!m_suppressFrameRequestsWorkaroundFor704763Only.
Backtrace:
	base::debug::StackTrace::StackTrace [0x016418E7+55]
	base::debug::StackTrace::StackTrace [0x0166540A+10]
	blink::PageAnimator::suppressFrameRequestsWorkaroundFor704763Only
[0x028E381E+76]
	content::RenderWidget::WillBeginCompositorFrame [0x02CCD0ED+109]
	cc::ProxyMain::BeginMainFrame [0x01F5492F+1231]
	base::internal::Invoker<base::internal::BindState<void (__thiscall
cc::ProxyMain::*)(std::unique_ptr<cc::BeginMainFrameAndCommitState,std::default_delete<cc::BeginMainFrameAndCommitState>
>),base::WeakPtr<cc::ProxyMain>,base::internal::PassedWrapper<std::
[0x01F9B1CC+124]
	base::internal::Invoker<base::internal::BindState<void (__thiscall
cc::ProxyMain::*)(std::unique_ptr<cc::BeginMainFrameAndCommitState,std::default_delete<cc::BeginMainFrameAndCommitState>
>),base::WeakPtr<cc::ProxyMain>,base::internal::PassedWrapper<std::
[0x01F9D6F6+22]
	base::debug::TaskAnnotator::RunTask [0x0166EF59+409]
	blink::scheduler::TaskQueueManager::ProcessTaskFromWorkQueue [0x0224336E+919]
	blink::scheduler::TaskQueueManager::DoWork [0x02242181+625]
	base::internal::FunctorTraits<void (__thiscall
content::WebMediaPlayerMS::*)(bool),void>::Invoke<base::WeakPtr<content::WebMediaPlayerMS>
const &,bool const &> [0x02CF5394+26]
	base::internal::InvokeHelper<1,void>::MakeItSo<void (__thiscall
content::WebMediaPlayerMS::*const
&)(bool),base::WeakPtr<content::WebMediaPlayerMS> const &,bool const &>
[0x02D9C9AD+34]
	base::internal::Invoker<base::internal::BindState<void (__thiscall
content::WebMediaPlayerMS::*)(bool),base::WeakPtr<content::WebMediaPlayerMS>,bool>,void
__cdecl(void)>::Run [0x0097E4B7+23]
	base::debug::TaskAnnotator::RunTask [0x0166EF59+409]
	base::MessageLoop::RunTask [0x01604201+1233]
	base::MessageLoop::DoWork [0x01603305+741]
	base::MessagePumpDefault::Run [0x0166FDCB+219]
	base::MessageLoop::RunHandler [0x01603D27+247]
	base::RunLoop::Run [0x015FA644+132]
	content::RendererMain [0x02CD4EB1+449]
	content::RunNamedProcessTypeMain [0x007AEE97+206]
	content::ContentMainRunnerImpl::Run [0x007AED98+280]
	service_manager::Main [0x020712C8+252]
	content::ContentMain [0x007AE43E+39]
	content::LaunchTests [0x013AC509+560]

[esprehn, 2017-04-04 18:30:45]

Actually what are those tests doing that they don't correctly call
updateAllLifecyclePhases?

[alancutter (OOO until 2018), 2017-04-05 01:28:46]

On 2017/04/04 at 18:30:45, esprehn wrote:
> Actually what are those tests doing that they don't correctly call
updateAllLifecyclePhases?

WebViewImpl::updateAllLifecyclePhases() early exits without calling
PageWidgetDelegate::updateAllLifecyclePhases(), looks like it's swapping between
two different Pages.
Putting the flag clear in RenderWidget::UpdateVisualState() fixes that issue.

[alancutter (OOO until 2018), 2017-04-05 01:28:53]

The CQ bit was checked by alancutter@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-05 01:29:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[alancutter (OOO until 2018), 2017-04-05 01:42:22]

On 2017/04/04 at 10:22:25, commit-bot wrote:
> Dry run: Try jobs failed on following builders:
>   linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

On the bright side these failing tests are showing me plausible test framework
candidates for actually testing this bug.

[commit-bot: I haz the power, 2017-04-05 02:15:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-05 02:15:21]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[alancutter (OOO until 2018), 2017-04-05 02:59:56]

The CQ bit was checked by alancutter@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-05 03:00:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-05 03:47:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-05 03:47:28]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[alancutter (OOO until 2018), 2017-04-05 03:56:55]

On 2017/04/05 at 03:47:28, commit-bot wrote:
> Dry run: Try jobs failed on following builders:
>   win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

This is failing on a test helper method testRunner.layoutAndPaintAsyncThen().

STDERR: [1:1:0405/133504.623284:716244612454:FATAL:PageAnimator.cpp(79)] Check
failed: m_suppressFrameRequestsWorkaroundFor704763Only != suppressFrameRequests
(0 vs. 0)
STDERR: #0 0x7f5ec014f9a7 base::debug::StackTrace::StackTrace()
STDERR: #1 0x7f5ec017306b logging::LogMessage::~LogMessage()
STDERR: #2 0x7f5ebbeca8ec
blink::PageAnimator::setSuppressFrameRequestsWorkaroundFor704763Only()
STDERR: #3 0x7f5ec130f69b content::RenderWidget::UpdateVisualState()
STDERR: #4 0x7f5ebe6d47e8 cc::LayerTreeHost::LayoutAndUpdateLayers()
STDERR: #5 0x7f5ec12566bf
content::RenderWidgetCompositor::LayoutAndUpdateLayers()

I've relaxed the DCHECK in PageAnimator.cpp to allow multiple clears of the
suppression in a row with an extra comment.

[alancutter (OOO until 2018), 2017-04-05 03:57:17]

The CQ bit was checked by alancutter@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-05 03:57:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-05 05:19:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-05 05:19:28]

Dry run: This issue passed the CQ dry run.

[alancutter (OOO until 2018), 2017-04-05 05:20:07]

On 2017/04/05 at 05:19:28, commit-bot wrote:
> Dry run: This issue passed the CQ dry run.

Finally. :D

[esprehn, 2017-04-05 08:33:02]

lgtm

https://codereview.chromium.org/2791223002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/PageAnimator.cpp (right):

https://codereview.chromium.org/2791223002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/PageAnimator.cpp:80:
DCHECK(!m_suppressFrameRequestsWorkaroundFor704763Only ||
This could be != Instead of the or I think?

[alancutter (OOO until 2018), 2017-04-05 10:04:18]

https://codereview.chromium.org/2791223002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/PageAnimator.cpp (right):

https://codereview.chromium.org/2791223002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/PageAnimator.cpp:80:
DCHECK(!m_suppressFrameRequestsWorkaroundFor704763Only ||
On 2017/04/05 at 08:33:02, esprehn wrote:
> This could be != Instead of the or I think?

It used to be but some tests call UpdateVisualState() twice in a row inside
paint test helpers. See above comments.

[alancutter (OOO until 2018), 2017-04-05 10:04:43]

The CQ bit was checked by alancutter@chromium.org

[commit-bot: I haz the power, 2017-04-05 10:04:52]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-05 10:09:36]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1491386683620080,
"parent_rev": "dffcf95adcedc8a1e1e59a17695a2610664b4564", "commit_rev":
"317a8e0e26459f401312043fa99ed8e17b1b24a4"}

[commit-bot: I haz the power, 2017-04-05 10:10:21]

Description was changed from

==========
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
==========

to

==========
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}
Committed:
https://chromium.googlesource.com/chromium/src/+/317a8e0e26459f401312043fa99e...
==========

[commit-bot: I haz the power, 2017-04-05 10:10:23]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/317a8e0e26459f401312043fa99e...