Remove ForceReclaimResources

Remove ForceReclaimResources

We had previously added ForceReclaimResources to BeginCommit to throttle
frame output and prevent starving the display compositor's rasterization
tasks. Unfortunately, this introduces significant overhead as well.

With the addition of the Display Scheduler, this change no longer
appears to be needed. The scheduler has independent controls for
limiting the number of pending frames, and these should be used instead
(if further tweaking is necessary).

Removing this logic does not regress the benchmarks which it was
initially added for, and seems unneeded at this point. Removing.

R=reveman@chromium.org
BUG=676852, 489515, 617268
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2609253003
Cr-Commit-Position: refs/heads/master@{#443061}
Committed: https://chromium.googlesource.com/chromium/src/+/dc1892fc0639464d4418ccfaedfd1f95a5b947d0

[ericrk, 2017-01-03 19:46:09]

Description was changed from

==========
Remove ForceReclaimResources

We had previously added ForceReclaimResources to BeginCommit to throttle
frame output and prevent starving the display compositor's rasterization
tasks. Unfortunately, this introduces significant overhead as well.

With the addition of the Display Scheduler, this change no longer
appears to be needed. The scheduler has independent controls for
limiting the number of pending frames, and these should be used instead
(if further tweaking is necessary).

Removing this logic does not regress the benchmarks which it was
initially added for, and seems unneeded at this point. Removing.

R=reveman@chromium.org
BUG=676852,489515
==========

to

==========
Remove ForceReclaimResources

We had previously added ForceReclaimResources to BeginCommit to throttle
frame output and prevent starving the display compositor's rasterization
tasks. Unfortunately, this introduces significant overhead as well.

With the addition of the Display Scheduler, this change no longer
appears to be needed. The scheduler has independent controls for
limiting the number of pending frames, and these should be used instead
(if further tweaking is necessary).

Removing this logic does not regress the benchmarks which it was
initially added for, and seems unneeded at this point. Removing.

R=reveman@chromium.org
BUG=676852,489515
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[ericrk, 2017-01-03 19:46:23]

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

[commit-bot: I haz the power, 2017-01-03 19:46:43]

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

[danakj, 2017-01-03 19:59:07]

danakj@chromium.org changed reviewers:
+ danakj@chromium.org

[danakj, 2017-01-03 19:59:07]

This is also what allows partial raster with 1 resource instead of 2, but I know
you added stuff lately to do a good job of partial raster across frames.

Can you remove it from the CompositorFrameSink etc as well?

[commit-bot: I haz the power, 2017-01-03 20:23:25]

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

[commit-bot: I haz the power, 2017-01-03 20:23:26]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[reveman, 2017-01-03 20:35:25]

Thanks for taking a look at this so quickly. lgtm when danakj is happy.

[ericrk, 2017-01-04 18:10:48]

Updated this to fully remove ForceReclaimResources and fix up unit tests.
Partial raster still works as expected (but may need two frames before it kicks
in).

https://codereview.chromium.org/2609253003/diff/40001/cc/layers/texture_layer...
File cc/layers/texture_layer_unittest.cc (right):

https://codereview.chromium.org/2609253003/diff/40001/cc/layers/texture_layer...
cc/layers/texture_layer_unittest.cc:654:
sink->SetForceReclaimResourcesAfterSwap();
This is now a test-only method which is used for this test only.

https://codereview.chromium.org/2609253003/diff/40001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_pixeltest_tiles.cc (right):

https://codereview.chromium.org/2609253003/diff/40001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_pixeltest_tiles.cc:166:
picture_layer_->SetNeedsDisplayRect(gfx::Rect(50, 50, 100, 100));
Updated the partial raster test - we now give it two frames from which to grab
the resource to partial raster into.

[ericrk, 2017-01-04 18:10:56]

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

[commit-bot: I haz the power, 2017-01-04 18:11:05]

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-01-04 18:22:22]

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

[commit-bot: I haz the power, 2017-01-04 18:22:22]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[danakj, 2017-01-04 18:43:38]

https://codereview.chromium.org/2609253003/diff/40001/cc/surfaces/direct_comp...
File cc/surfaces/direct_compositor_frame_sink.cc (left):

https://codereview.chromium.org/2609253003/diff/40001/cc/surfaces/direct_comp...
cc/surfaces/direct_compositor_frame_sink.cc:107: factory_.ClearSurface();
Good news/bad news. This is the only caller of ClearSurface() too

https://codereview.chromium.org/2609253003/diff/40001/cc/surfaces/direct_comp...
File cc/surfaces/direct_compositor_frame_sink_unittest.cc (right):

https://codereview.chromium.org/2609253003/diff/40001/cc/surfaces/direct_comp...
cc/surfaces/direct_compositor_frame_sink_unittest.cc:141:
LockingResourcesDoesNotIndirectlyCauseDamage) {
It looks like you can remove this test, it's now identical to
NoDamageDoesNotTriggerSwapBuffers?

https://codereview.chromium.org/2609253003/diff/40001/cc/test/test_compositor...
File cc/test/test_compositor_frame_sink.cc (right):

https://codereview.chromium.org/2609253003/diff/40001/cc/test/test_compositor...
cc/test/test_compositor_frame_sink.cc:192: surface_factory_->ClearSurface();
Oh I lied, this is now a test-only method. Hm. Would be nice to not have it. Can
the TextureLayerTests just swap a 2nd empty frame after to release resources? Or
if not, can you just submit an empty frame here?

https://codereview.chromium.org/2609253003/diff/40001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/2609253003/diff/40001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:5708:
SINGLE_THREAD_TEST_F(LayerTreeHostTestSynchronousCompositeSwapPromise);
I missed this because it wasn't the macro sorry, but please add a comment why
this is single threaded only when using the macro. If it helps this is because
synchronous composite is only a single-threaded feature.

[ericrk, 2017-01-05 21:25:35]

rebase

[ericrk, 2017-01-06 00:08:46]

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

[commit-bot: I haz the power, 2017-01-06 00:09:39]

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-01-06 01:09:43]

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

[commit-bot: I haz the power, 2017-01-06 01:09:44]

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

[ericrk, 2017-01-06 22:10:10]

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

[commit-bot: I haz the power, 2017-01-06 22:10:46]

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-01-06 22:59:57]

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

[commit-bot: I haz the power, 2017-01-06 22:59:58]

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

[ericrk, 2017-01-07 02:18:25]

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

[commit-bot: I haz the power, 2017-01-07 02:18:55]

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-01-07 02:56:21]

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

[commit-bot: I haz the power, 2017-01-07 02:56:21]

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

[ericrk, 2017-01-08 23:02:18]

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

[commit-bot: I haz the power, 2017-01-08 23:02:30]

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-01-08 23:37:38]

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

[commit-bot: I haz the power, 2017-01-08 23:37:40]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[ericrk, 2017-01-09 01:03:22]

The CQ bit was unchecked by ericrk@chromium.org

[ericrk, 2017-01-09 18:31:18]

Patchset #6 (id:100001) has been deleted

[ericrk, 2017-01-09 18:31:27]

Patchset #6 (id:120001) has been deleted

[ericrk, 2017-01-09 18:41:47]

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

[commit-bot: I haz the power, 2017-01-09 18:42:14]

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-01-09 20:10:58]

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

[commit-bot: I haz the power, 2017-01-09 20:10:59]

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_...)

[ericrk, 2017-01-09 21:25:06]

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

[commit-bot: I haz the power, 2017-01-09 21:25: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-01-09 23:10:46]

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

[commit-bot: I haz the power, 2017-01-09 23:10:47]

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

[ericrk, 2017-01-11 18:52:37]

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

[commit-bot: I haz the power, 2017-01-11 18:53:16]

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

[ericrk, 2017-01-11 19:17:54]

Ok, I think this CL is in good shape now. Delaying the more problematic part
(allowing browser LTHI to skip frames with no damage) to a subsequent CL. Filed
a bug for this.

https://codereview.chromium.org/2609253003/diff/40001/cc/surfaces/direct_comp...
File cc/surfaces/direct_compositor_frame_sink.cc (left):

https://codereview.chromium.org/2609253003/diff/40001/cc/surfaces/direct_comp...
cc/surfaces/direct_compositor_frame_sink.cc:107: factory_.ClearSurface();
On 2017/01/04 18:43:38, danakj_OOO_and_slow wrote:
> Good news/bad news. This is the only caller of ClearSurface() too

Removed this and the now unused "EvictFrame" I found in the process :D

https://codereview.chromium.org/2609253003/diff/40001/cc/surfaces/direct_comp...
File cc/surfaces/direct_compositor_frame_sink_unittest.cc (right):

https://codereview.chromium.org/2609253003/diff/40001/cc/surfaces/direct_comp...
cc/surfaces/direct_compositor_frame_sink_unittest.cc:141:
LockingResourcesDoesNotIndirectlyCauseDamage) {
On 2017/01/04 18:43:38, danakj_OOO_and_slow wrote:
> It looks like you can remove this test, it's now identical to
> NoDamageDoesNotTriggerSwapBuffers?

Ah yeah, looks like it. That first part isn't really doing much.

https://codereview.chromium.org/2609253003/diff/40001/cc/test/test_compositor...
File cc/test/test_compositor_frame_sink.cc (right):

https://codereview.chromium.org/2609253003/diff/40001/cc/test/test_compositor...
cc/test/test_compositor_frame_sink.cc:192: surface_factory_->ClearSurface();
On 2017/01/04 18:43:38, danakj_OOO_and_slow wrote:
> Oh I lied, this is now a test-only method. Hm. Would be nice to not have it.
Can
> the TextureLayerTests just swap a 2nd empty frame after to release resources?
Or
> if not, can you just submit an empty frame here?

Removed this and added an extra dummy swap in the TextureLayerTest.

https://codereview.chromium.org/2609253003/diff/40001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/2609253003/diff/40001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:5708:
SINGLE_THREAD_TEST_F(LayerTreeHostTestSynchronousCompositeSwapPromise);
On 2017/01/04 18:43:38, danakj_OOO_and_slow wrote:
> I missed this because it wasn't the macro sorry, but please add a comment why
> this is single threaded only when using the macro. If it helps this is because
> synchronous composite is only a single-threaded feature.

Done.

[danakj, 2017-01-11 19:28:24]

LGTM

https://codereview.chromium.org/2609253003/diff/160001/cc/layers/texture_laye...
File cc/layers/texture_layer_unittest.cc (right):

https://codereview.chromium.org/2609253003/diff/160001/cc/layers/texture_laye...
cc/layers/texture_layer_unittest.cc:706: // resources.
^_^b

https://codereview.chromium.org/2609253003/diff/160001/content/test/layouttes...
File content/test/layouttest_support.cc (right):

https://codereview.chromium.org/2609253003/diff/160001/content/test/layouttes...
content/test/layouttest_support.cc:361: compositor_frame_sink->SetAlwaysSwap();
Instead we could have the code that inserts a copy request also set damage on
the viewport?

[commit-bot: I haz the power, 2017-01-11 19:57:49]

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

[commit-bot: I haz the power, 2017-01-11 19:57:50]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[ericrk, 2017-01-11 20:56:59]

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

[commit-bot: I haz the power, 2017-01-11 20:57: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-01-11 22:04:26]

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

[commit-bot: I haz the power, 2017-01-11 22:04:27]

Dry run: This issue passed the CQ dry run.

[ericrk, 2017-01-11 23:10:50]

https://codereview.chromium.org/2609253003/diff/160001/content/test/layouttes...
File content/test/layouttest_support.cc (right):

https://codereview.chromium.org/2609253003/diff/160001/content/test/layouttes...
content/test/layouttest_support.cc:361: compositor_frame_sink->SetAlwaysSwap();
On 2017/01/11 19:28:24, danakj (slow) wrote:
> Instead we could have the code that inserts a copy request also set damage on
> the viewport?

Added the invalidation to the code that triggers the copy request in
RenderWidgetCompositor.

[danakj, 2017-01-11 23:12:59]

LGTM

[ericrk, 2017-01-11 23:13:16]

ericrk@chromium.org changed reviewers:
+ enne@chromium.org, tommi@chromium.org

[ericrk, 2017-01-11 23:13:17]

+enne for content/renderer/gpu/render_widget_compositor.cc
+tommi for content/test/layouttest_support.cc

[tommi (sloooow) - chröme, 2017-01-11 23:19:54]

rs lgtm

[enne (OOO), 2017-01-11 23:24:02]

rwc lgtm (and lgtm in general)

[ericrk, 2017-01-11 23:26:46]

Description was changed from

==========
Remove ForceReclaimResources

We had previously added ForceReclaimResources to BeginCommit to throttle
frame output and prevent starving the display compositor's rasterization
tasks. Unfortunately, this introduces significant overhead as well.

With the addition of the Display Scheduler, this change no longer
appears to be needed. The scheduler has independent controls for
limiting the number of pending frames, and these should be used instead
(if further tweaking is necessary).

Removing this logic does not regress the benchmarks which it was
initially added for, and seems unneeded at this point. Removing.

R=reveman@chromium.org
BUG=676852,489515
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Remove ForceReclaimResources

We had previously added ForceReclaimResources to BeginCommit to throttle
frame output and prevent starving the display compositor's rasterization
tasks. Unfortunately, this introduces significant overhead as well.

With the addition of the Display Scheduler, this change no longer
appears to be needed. The scheduler has independent controls for
limiting the number of pending frames, and these should be used instead
(if further tweaking is necessary).

Removing this logic does not regress the benchmarks which it was
initially added for, and seems unneeded at this point. Removing.

R=reveman@chromium.org
BUG=676852,489515,617268
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[ericrk, 2017-01-11 23:39:42]

The CQ bit was checked by ericrk@chromium.org

[ericrk, 2017-01-11 23:39:43]

The patchset sent to the CQ was uploaded after l-g-t-m from reveman@chromium.org
Link to the patchset: https://codereview.chromium.org/2609253003/#ps170001
(title: "Damage frame for layout tests rather than forcing us to always swap")

[commit-bot: I haz the power, 2017-01-11 23:41:07]

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-01-11 23:54:18]

CQ is committing da patch.

Bot data: {"patchset_id": 170001, "attempt_start_ts": 1484177982631210,
"parent_rev": "0b469432cc5bd5db8422ce5a8100f8fcae148a2c", "commit_rev":
"dc1892fc0639464d4418ccfaedfd1f95a5b947d0"}

[commit-bot: I haz the power, 2017-01-11 23:55:09]

Description was changed from

==========
Remove ForceReclaimResources

We had previously added ForceReclaimResources to BeginCommit to throttle
frame output and prevent starving the display compositor's rasterization
tasks. Unfortunately, this introduces significant overhead as well.

With the addition of the Display Scheduler, this change no longer
appears to be needed. The scheduler has independent controls for
limiting the number of pending frames, and these should be used instead
(if further tweaking is necessary).

Removing this logic does not regress the benchmarks which it was
initially added for, and seems unneeded at this point. Removing.

R=reveman@chromium.org
BUG=676852,489515,617268
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Remove ForceReclaimResources

We had previously added ForceReclaimResources to BeginCommit to throttle
frame output and prevent starving the display compositor's rasterization
tasks. Unfortunately, this introduces significant overhead as well.

With the addition of the Display Scheduler, this change no longer
appears to be needed. The scheduler has independent controls for
limiting the number of pending frames, and these should be used instead
(if further tweaking is necessary).

Removing this logic does not regress the benchmarks which it was
initially added for, and seems unneeded at this point. Removing.

R=reveman@chromium.org
BUG=676852,489515,617268
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2609253003
Cr-Commit-Position: refs/heads/master@{#443061}
Committed:
https://chromium.googlesource.com/chromium/src/+/dc1892fc0639464d4418ccfaedfd...
==========

[commit-bot: I haz the power, 2017-01-11 23:55:15]

Committed patchset #8 (id:170001) as
https://chromium.googlesource.com/chromium/src/+/dc1892fc0639464d4418ccfaedfd...

[brianderson, 2017-01-12 00:38:15]

brianderson@chromium.org changed reviewers:
+ brianderson@chromium.org

[brianderson, 2017-01-12 00:38:16]

https://codereview.chromium.org/2609253003/diff/170001/cc/surfaces/surface_fa...
File cc/surfaces/surface_factory.cc (left):

https://codereview.chromium.org/2609253003/diff/170001/cc/surfaces/surface_fa...
cc/surfaces/surface_factory.cc:101:
manager_->SurfaceModified(current_surface_->surface_id());
Manager::SurfaceModified calls  Display::UpdateRootSurfaceResourcesLocked, which
would become true after the frame is evicted. Since the frame isn't evicted
anymore, is that essentially dead code now? Can all "ResourcesLocked" related
logic be removed too?

[ericrk, 2017-01-13 20:41:15]

A revert of this CL (patchset #8 id:170001) has been created in
https://codereview.chromium.org/2633563003/ by ericrk@chromium.org.

The reason for reverting is: This has introduced flakiness into
TextureLayerImplWithMailboxThreadedCallback.RunMultiThread_DelegatingRenderer,
will address this and re-land..

[ericrk, 2017-01-17 18:16:26]

Patchset #8 (id:170001) has been deleted

[ericrk, 2017-01-17 18:26:27]

Patchset #8 (id:190001) has been deleted

[ericrk, 2017-01-17 18:27:03]

Description was changed from

==========
Remove ForceReclaimResources

We had previously added ForceReclaimResources to BeginCommit to throttle
frame output and prevent starving the display compositor's rasterization
tasks. Unfortunately, this introduces significant overhead as well.

With the addition of the Display Scheduler, this change no longer
appears to be needed. The scheduler has independent controls for
limiting the number of pending frames, and these should be used instead
(if further tweaking is necessary).

Removing this logic does not regress the benchmarks which it was
initially added for, and seems unneeded at this point. Removing.

R=reveman@chromium.org
BUG=676852,489515,617268
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2609253003
Cr-Commit-Position: refs/heads/master@{#443061}
Committed:
https://chromium.googlesource.com/chromium/src/+/dc1892fc0639464d4418ccfaedfd...
==========

to

==========
Remove ForceReclaimResources

We had previously added ForceReclaimResources to BeginCommit to throttle
frame output and prevent starving the display compositor's rasterization
tasks. Unfortunately, this introduces significant overhead as well.

With the addition of the Display Scheduler, this change no longer
appears to be needed. The scheduler has independent controls for
limiting the number of pending frames, and these should be used instead
(if further tweaking is necessary).

Removing this logic does not regress the benchmarks which it was
initially added for, and seems unneeded at this point. Removing.

R=reveman@chromium.org
BUG=676852,489515,617268
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2609253003
Cr-Commit-Position: refs/heads/master@{#443061}
Committed:
https://chromium.googlesource.com/chromium/src/+/dc1892fc0639464d4418ccfaedfd...
==========

[ericrk, 2017-01-17 18:27:12]

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

[commit-bot: I haz the power, 2017-01-17 18:27:41]

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-01-17 19:27:28]

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

[commit-bot: I haz the power, 2017-01-17 19:27:29]

Dry run: This issue passed the CQ dry run.

[ericrk, 2017-01-18 18:44:48]

danakj@, can you take a look at the updated unit test in
texture_layer_unittest.cc? I had to tweak the logic to prevent flakiness.

Unfortunately, adding another swap didn't quite work. It ensures that the
resource is returned, but doesn't ensure that the resource released callback has
time to run - that callback is just added to the task runner when the resource
is returned, nothing about presenting a single frame guarantees it will be
processed - this is why the test was actually always flaky (my change just made
it flake a bit more).

I've added some logic to flip up to 10 frames while waiting for the callback...
we could also manually pump the task runner, but that seemed a bit messier?
WDYT?

[danakj, 2017-01-18 19:30:39]

On 2017/01/18 18:44:48, ericrk wrote:
> danakj@, can you take a look at the updated unit test in
> texture_layer_unittest.cc? I had to tweak the logic to prevent flakiness.
> 
> Unfortunately, adding another swap didn't quite work. It ensures that the
> resource is returned, but doesn't ensure that the resource released callback
has
> time to run - that callback is just added to the task runner when the resource
> is returned, nothing about presenting a single frame guarantees it will be
> processed - this is why the test was actually always flaky (my change just
made
> it flake a bit more).
> 
> I've added some logic to flip up to 10 frames while waiting for the
callback...
> we could also manually pump the task runner, but that seemed a bit messier?
> WDYT?

Hm, what if the release callback is what tells the test to go to the next step?

[ericrk, 2017-01-18 20:12:20]

On 2017/01/18 19:30:39, danakj (slow) wrote:
> On 2017/01/18 18:44:48, ericrk wrote:
> > danakj@, can you take a look at the updated unit test in
> > texture_layer_unittest.cc? I had to tweak the logic to prevent flakiness.
> > 
> > Unfortunately, adding another swap didn't quite work. It ensures that the
> > resource is returned, but doesn't ensure that the resource released callback
> has
> > time to run - that callback is just added to the task runner when the
resource
> > is returned, nothing about presenting a single frame guarantees it will be
> > processed - this is why the test was actually always flaky (my change just
> made
> > it flake a bit more).
> > 
> > I've added some logic to flip up to 10 frames while waiting for the
> callback...
> > we could also manually pump the task runner, but that seemed a bit messier?
> > WDYT?
> 
> Hm, what if the release callback is what tells the test to go to the next
step?

Ok, I think that is a better approach. Not all steps/phases need a callback to
go
to the next, so there's a bit of annoying modality, but I still think this is
better
and should be not-flaky (nothing relies on timing). Take a look - it's easiest
to
diff against unmodified (not the previous patch) at this point. Thanks!

[danakj, 2017-01-18 20:35:03]

That's great thanks. LGTM

https://codereview.chromium.org/2609253003/diff/250001/cc/layers/texture_laye...
File cc/layers/texture_layer_unittest.cc (right):

https://codereview.chromium.org/2609253003/diff/250001/cc/layers/texture_laye...
cc/layers/texture_layer_unittest.cc:657: // Case #2: change mailbox after the
commit (and draw), where the
Uh, it looks like Case #1 didn't make it, maybe u can decrement all these Case
numbers?

[ericrk, 2017-01-18 23:18:24]

https://codereview.chromium.org/2609253003/diff/250001/cc/layers/texture_laye...
File cc/layers/texture_layer_unittest.cc (right):

https://codereview.chromium.org/2609253003/diff/250001/cc/layers/texture_laye...
cc/layers/texture_layer_unittest.cc:657: // Case #2: change mailbox after the
commit (and draw), where the
On 2017/01/18 20:35:03, danakj (slow) wrote:
> Uh, it looks like Case #1 didn't make it, maybe u can decrement all these Case
> numbers?

Case 1 was in BeginTest - Moved it here so they're all in one place. Also
stopped calling things "phases", everything is now called "test case".

[ericrk, 2017-01-18 23:18:31]

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

[commit-bot: I haz the power, 2017-01-18 23:19:21]

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

[danakj, 2017-01-18 23:20:08]

That is very nice!

[commit-bot: I haz the power, 2017-01-18 23:22:27]

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

[commit-bot: I haz the power, 2017-01-18 23:22:29]

Dry run: Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[ericrk, 2017-04-12 18:01:41]

ericrk@chromium.org changed reviewers:
+ jbauman@chromium.org, kylechar@chromium.org

[ericrk, 2017-04-12 18:01:42]

On 2017/01/18 23:20:08, danakj (out sick) wrote:
> That is very nice!

===============================================================================
So, I think we're re-opening this, as it's a blocker for some MUS work. To give
a summary of why we never landed this:

  Right now, when rasterizing resources for the Browser UI (which happens in
  the browser proc, along with compositing), we force reclaim resources before
  raster, allowing us to immediately raster into them again. To the best of my
  understanding, this only works because raster/compositing happen in the same
  proc, which makes the reclaim super fast and efficient.

  With this change, we no longer force reclaim, so when we go to raster the
  browser UI, the existing resources aren't available, and we need to raster
  into new resources, which are then swapped with the existing ones. This
  forces a sort of "double buffering", driving up memory cost during
  rasterization. You can see the impact here:
 
https://chromeperf.appspot.com/report?sid=a82a20f2563b7313176f76d2e7d91ed7d8e...

  That said, this double-buffering cost should only be paid for those tiles
  which are currently being re-rastered, so I imagine the impact won't be
  huge.

This was enough to delay landing this, as the benefits were limited, and it
turned out this wasn't needed to address a larger perf problem I was
investigating.

However, now that we are working on moving browser compositing to a MUS
process, this has become a blocker. To the best of my understanding,
moving compositing to the MUS process makes browser UI rasterization
much like renderer rasterization - not suitable for force reclaim, as
this now involves cross process IPCs and more delays. Given this, I think
these memory regressions are fairly unavoidable.

Given that this double-buffering should only be for tiles that are actively
being rasterized, I hope the regression is small enough to ignore. I propose
re-landing this and watching to make sure no large regressions appear via
UMA or telemetry? Does this plan sound OK to everyone?

kylechar@ has offered to take over this effort, and will probably re-base
and re-land this in a new CL.

[danakj, 2017-04-12 19:01:48]

SGTM

On Wed, Apr 12, 2017 at 2:01 PM, <ericrk@chromium.org> wrote:

> On 2017/01/18 23:20:08, danakj (out sick) wrote:
> > That is very nice!
>
> ============================================================
> ===================
> So, I think we're re-opening this, as it's a blocker for some MUS work. To
> give
> a summary of why we never landed this:
>
> Right now, when rasterizing resources for the Browser UI (which happens in
> the browser proc, along with compositing), we force reclaim resources
> before
> raster, allowing us to immediately raster into them again. To the best of
> my
> understanding, this only works because raster/compositing happen in the
> same
> proc, which makes the reclaim super fast and efficient.
>
> With this change, we no longer force reclaim, so when we go to raster the
> browser UI, the existing resources aren't available, and we need to raster
> into new resources, which are then swapped with the existing ones. This
> forces a sort of "double buffering", driving up memory cost during
> rasterization. You can see the impact here:
>
> https://chromeperf.appspot.com/report?sid=a82a20f2563b7313176f76d2e7d91e
> d7d8e016283f3909b258983d2c85c59024&start_rev=442183&end_rev=443906
>
> That said, this double-buffering cost should only be paid for those tiles
> which are currently being re-rastered, so I imagine the impact won't be
> huge.
>
> This was enough to delay landing this, as the benefits were limited, and it
> turned out this wasn't needed to address a larger perf problem I was
> investigating.
>
> However, now that we are working on moving browser compositing to a MUS
> process, this has become a blocker. To the best of my understanding,
> moving compositing to the MUS process makes browser UI rasterization
> much like renderer rasterization - not suitable for force reclaim, as
> this now involves cross process IPCs and more delays. Given this, I think
> these memory regressions are fairly unavoidable.
>
> Given that this double-buffering should only be for tiles that are actively
> being rasterized, I hope the regression is small enough to ignore. I
> propose
> re-landing this and watching to make sure no large regressions appear via
> UMA or telemetry? Does this plan sound OK to everyone?
>
> kylechar@ has offered to take over this effort, and will probably re-base
> and re-land this in a new CL.
>
>
>
> https://codereview.chromium.org/2609253003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[brianderson, 2017-04-12 19:32:18]

brianderson@chromium.org changed reviewers:
+ mariakhomenko@chromium.org

[brianderson, 2017-04-12 19:32:19]

It would simplify DisplayScheduler, so +1 there.

Considering recent efforts targeting low memory devices though, I wonder if we
should coordinate when to regress memory. Would it make sense to defer it until
other memory improvements bring OOM crash rates to am acceptable level?

+mariakhomenko

[Maria, 2017-04-12 21:41:47]

On 2017/04/12 19:32:19, brianderson wrote:
> It would simplify DisplayScheduler, so +1 there.
> 
> Considering recent efforts targeting low memory devices though, I wonder if we
> should coordinate when to regress memory. Would it make sense to defer it
until
> other memory improvements bring OOM crash rates to am acceptable level?
> 
> +mariakhomenko

I think this depends on the size of the regression to Android. I am seeing a
500KiB regression on the chromeperf link above. I tried loading the graph from
the same revision for Android and didn't see anything on it. I think we should
measure it -- this should help decide how to proceed. Low memory device work is
targeted to M-61.

[danakj, 2017-04-13 15:16:16]

This does not affect android, as android does not use ui::Compositor or
raster in the browser compositor.

On Wed, Apr 12, 2017 at 5:41 PM, <mariakhomenko@chromium.org> wrote:

> On 2017/04/12 19:32:19, brianderson wrote:
> > It would simplify DisplayScheduler, so +1 there.
> >
> > Considering recent efforts targeting low memory devices though, I wonder
> if we
> > should coordinate when to regress memory. Would it make sense to defer it
> until
> > other memory improvements bring OOM crash rates to am acceptable level?
> >
> > +mariakhomenko
>
> I think this depends on the size of the regression to Android. I am seeing
> a
> 500KiB regression on the chromeperf link above. I tried loading the graph
> from
> the same revision for Android and didn't see anything on it. I think we
> should
> measure it -- this should help decide how to proceed. Low memory device
> work is
> targeted to M-61.
>
> https://codereview.chromium.org/2609253003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[kylechar, 2017-04-19 14:23:43]

On 2017/04/13 15:16:16, danakj wrote:
> This does not affect android, as android does not use ui::Compositor or
> raster in the browser compositor.
> 
> On Wed, Apr 12, 2017 at 5:41 PM, <mailto:mariakhomenko@chromium.org> wrote:
> 
> > On 2017/04/12 19:32:19, brianderson wrote:
> > > It would simplify DisplayScheduler, so +1 there.
> > >
> > > Considering recent efforts targeting low memory devices though, I wonder
> > if we
> > > should coordinate when to regress memory. Would it make sense to defer it
> > until
> > > other memory improvements bring OOM crash rates to am acceptable level?
> > >
> > > +mariakhomenko
> >
> > I think this depends on the size of the regression to Android. I am seeing
> > a
> > 500KiB regression on the chromeperf link above. I tried loading the graph
> > from
> > the same revision for Android and didn't see anything on it. I think we
> > should
> > measure it -- this should help decide how to proceed. Low memory device
> > work is
> > targeted to M-61.
> >
> > https://codereview.chromium.org/2609253003/
> >
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

I've rebased this and uploaded in https://codereview.chromium.org/2822143003.
I'm just trying to run some perf tests and then I'll send it out for review.