Remove ForceReclaimResources

Remove CompositorFrameSink::ForceReclaimResources().

This is CL is based off of http://crrev.com/2609253003. The CL was
rebased and ui/compositor/layer_unittest.cc modified to pass. Some
pieces of the original CL had already been landed in separate CLs.

Calling ForceReclaimResources() in LayerTreeHostImpl::BeginCommit()
returns all texture resources and avoid some double buffering overhead.
This optimization won't work across any thread/process boundaries, so
it's only used for the main browser UI compositor and not renderer
compositors.

We are planning on moving the display compositor to a separate process
and this browser UI compositor optimization will no longer be possible
with the mus graphics architecture. This CL removes
CompositorFrameSink::ForceReclaimResources() and associated code.

This will cause a memory regression for Win/Linux/Mac/CrOS. However,
only browser UI tiles that are actively being rastered will be double
buffered, so the regression should be small.

BUG=489515
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

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

[kylechar, 2017-04-18 17:53:27]

Description was changed from

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

This is CL is based off of http://crrev.com/2609253003.

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.


BUG=489515
==========

to

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

This is CL is based off of http://crrev.com/2609253003.

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.


BUG=489515
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[kylechar, 2017-04-18 17:54:51]

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

[commit-bot: I haz the power, 2017-04-18 17:55:15]

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-18 18:34:35]

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

[commit-bot: I haz the power, 2017-04-18 18:34:35]

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

[kylechar, 2017-04-18 19:46:55]

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

[commit-bot: I haz the power, 2017-04-18 19:47: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-18 21:46:37]

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

[commit-bot: I haz the power, 2017-04-18 21:46:38]

Dry run: This issue passed the CQ dry run.

[kylechar, 2017-04-19 14:14:31]

Patchset #3 (id:40001) has been deleted

[kylechar, 2017-04-19 19:28:51]

Description was changed from

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

This is CL is based off of http://crrev.com/2609253003.

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.


BUG=489515
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Remove ForceReclaimResources.

This is CL is based off of http://crrev.com/2609253003. The CL was
rebased and ui/compositor/layer_unittest.cc modified to pass. Some
pieces of the original CL had already been landed in separate CLs.

Calling ForceReclaimResources() in LayerTreeHostImpl::BeginCommit()
returns all textures, but will only work if everything is in the same
process / thread. As such, it's only used for the browser UI compositor.

We are planning on moving display compositor to a separate process and
this will no longer be possible. This CL removes
CompositorFrameSink::ForceReclaimResources() and associated code.

BUG=489515
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[kylechar, 2017-04-20 14:02:30]

Description was changed from

==========
Remove ForceReclaimResources.

This is CL is based off of http://crrev.com/2609253003. The CL was
rebased and ui/compositor/layer_unittest.cc modified to pass. Some
pieces of the original CL had already been landed in separate CLs.

Calling ForceReclaimResources() in LayerTreeHostImpl::BeginCommit()
returns all textures, but will only work if everything is in the same
process / thread. As such, it's only used for the browser UI compositor.

We are planning on moving display compositor to a separate process and
this will no longer be possible. This CL removes
CompositorFrameSink::ForceReclaimResources() and associated code.

BUG=489515
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Remove ForceReclaimResources.

This is CL is based off of http://crrev.com/2609253003. The CL was
rebased and ui/compositor/layer_unittest.cc modified to pass. Some
pieces of the original CL had already been landed in separate CLs.

Calling ForceReclaimResources() in LayerTreeHostImpl::BeginCommit()
returns all texture resources and avoid some double buffering overhead.
This optimization won't work across any thread/process boundaries, so
it's only used for the main browser UI compositor and not renderer
compositors.

We are planning on moving display compositor to a separate process and
this will no longer be possible with the mus graphics architecture. This
CL removes CompositorFrameSink::ForceReclaimResources() and associated
code.

This will cause a memory regression. However, only browser UI tiles that
are actively being rastered will be double buffered, so the regression
should be small.

BUG=489515
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[kylechar, 2017-04-20 14:03:27]

Description was changed from

==========
Remove ForceReclaimResources.

This is CL is based off of http://crrev.com/2609253003. The CL was
rebased and ui/compositor/layer_unittest.cc modified to pass. Some
pieces of the original CL had already been landed in separate CLs.

Calling ForceReclaimResources() in LayerTreeHostImpl::BeginCommit()
returns all texture resources and avoid some double buffering overhead.
This optimization won't work across any thread/process boundaries, so
it's only used for the main browser UI compositor and not renderer
compositors.

We are planning on moving display compositor to a separate process and
this will no longer be possible with the mus graphics architecture. This
CL removes CompositorFrameSink::ForceReclaimResources() and associated
code.

This will cause a memory regression. However, only browser UI tiles that
are actively being rastered will be double buffered, so the regression
should be small.

BUG=489515
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Remove ForceReclaimResources.

This is CL is based off of http://crrev.com/2609253003. The CL was
rebased and ui/compositor/layer_unittest.cc modified to pass. Some
pieces of the original CL had already been landed in separate CLs.

Calling ForceReclaimResources() in LayerTreeHostImpl::BeginCommit()
returns all texture resources and avoid some double buffering overhead.
This optimization won't work across any thread/process boundaries, so
it's only used for the main browser UI compositor and not renderer
compositors.

We are planning on moving display compositor to a separate process and
this will no longer be possible with the mus graphics architecture. This
CL removes CompositorFrameSink::ForceReclaimResources() and associated
code.

This will cause a memory regression for Win/Linux/Mac/CrOS. However,
only browser UI tiles that are actively being rastered will be double
buffered, so the regression should be small.

BUG=489515
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[kylechar, 2017-04-20 14:16:36]

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

[kylechar, 2017-04-20 14:16:37]

ericrk: Sending this to you for an initial review/sanity check.

The only real code change I made is one test from patch 1 to patch 2. The test
would crash because of unreturned resources. I changed WaitForDraw() to
WaitForCommit() and it no longer crashes.

I have perf results from Linux, Windows and Mac.
reported_by_chrome:cc:effective_size doesn't seem to have changed much. It's
either up or down less than MB with the patch. If extra memory used for double
buffering tiles shows up there that seems like a good sign that regression is
smaller noise?

Some of the other memory metrics vary way more. I'm not sure which metrics are
really useful though.

win-x64:
https://build.chromium.org/p/tryserver.chromium.perf/builders/win_x64_perf_bi...

win:
https://build.chromium.org/p/tryserver.chromium.perf/builders/win_perf_bisect...

mac-air:
https://build.chromium.org/p/tryserver.chromium.perf/builders/mac_air_perf_bi...

linux:
https://build.chromium.org/p/tryserver.chromium.perf/builders/linux_perf_bise...

[kylechar, 2017-04-20 14:37:52]

Here are some (maybe) interesting metrics:

win-x64:
memory:chrome:all_processes:reported_by_chrome:cc:effective_size: +64.9KB
memory:chrome:all_processes:reported_by_chrome:peak_size: +3.2MB

win:
memory:chrome:all_processes:reported_by_chrome:cc:effective_size: -487.2KB
memory:chrome:all_processes:reported_by_chrome:peak_size: -974.5KB

mac-air:
memory:chrome:all_processes:reported_by_chrome:cc:effective_size: -69.9KB
memory:chrome:all_processes:reported_by_chrome:peak_size: -134.3KB

linux:
memory:chrome:all_processes:reported_by_chrome:cc:effective_size: -134.5KB
memory:chrome:all_processes:reported_by_chrome:peak_size: -3.2MB

I want to hope reported_by_chrome:peak_size is just noise and there is no big
win/loss. The reported_by_os metrics seem even noisier and I have no idea what
to make of them.

[ericrk, 2017-04-24 22:24:08]

LGTM - the telemetry results seem fine to me - just a bit noisy. Let's keep an
eye on win-x64 after landing to make sure the peak result is just noise and not
consistently different. Thanks!

https://codereview.chromium.org/2822143003/diff/60001/ui/compositor/layer_uni...
File ui/compositor/layer_unittest.cc (right):

https://codereview.chromium.org/2822143003/diff/60001/ui/compositor/layer_uni...
ui/compositor/layer_unittest.cc:1142: // The texture mailbox has a reference
from an in-flight texture layer.
Thanks for the fix!

[kylechar, 2017-04-25 14:01:11]

Thanks! Will keep an eye on those.

[kylechar, 2017-04-25 14:03:44]

Description was changed from

==========
Remove ForceReclaimResources.

This is CL is based off of http://crrev.com/2609253003. The CL was
rebased and ui/compositor/layer_unittest.cc modified to pass. Some
pieces of the original CL had already been landed in separate CLs.

Calling ForceReclaimResources() in LayerTreeHostImpl::BeginCommit()
returns all texture resources and avoid some double buffering overhead.
This optimization won't work across any thread/process boundaries, so
it's only used for the main browser UI compositor and not renderer
compositors.

We are planning on moving display compositor to a separate process and
this will no longer be possible with the mus graphics architecture. This
CL removes CompositorFrameSink::ForceReclaimResources() and associated
code.

This will cause a memory regression for Win/Linux/Mac/CrOS. However,
only browser UI tiles that are actively being rastered will be double
buffered, so the regression should be small.

BUG=489515
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Remove CompositorFrameSink::ForceReclaimResources().

This is CL is based off of http://crrev.com/2609253003. The CL was
rebased and ui/compositor/layer_unittest.cc modified to pass. Some
pieces of the original CL had already been landed in separate CLs.

Calling ForceReclaimResources() in LayerTreeHostImpl::BeginCommit()
returns all texture resources and avoid some double buffering overhead.
This optimization won't work across any thread/process boundaries, so
it's only used for the main browser UI compositor and not renderer
compositors.

We are planning on moving the display compositor to a separate process
and this browser UI compositor optimization will no longer be possible
with the mus graphics architecture. This CL removes
CompositorFrameSink::ForceReclaimResources() and associated code.

This will cause a memory regression for Win/Linux/Mac/CrOS. However,
only browser UI tiles that are actively being rastered will be double
buffered, so the regression should be small.

BUG=489515
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[kylechar, 2017-04-25 14:09:04]

kylechar@chromium.org changed reviewers:
+ danakj@chromium.org, enne@chromium.org, sky@chromium.org

[kylechar, 2017-04-25 14:09:05]

+danakj for ui/compositor/layer_unittest.cc (and more general look at cc/* if
you feel like it)
+enne for content/renderer/gpu/render_widget_compositor.cc
+sky for content/test/layouttest_support.cc

[danakj, 2017-04-25 14:19:12]

ui/compositor LGTM

[danakj, 2017-04-25 14:20:42]

https://codereview.chromium.org/2822143003/diff/60001/ui/compositor/layer_uni...
File ui/compositor/layer_unittest.cc (right):

https://codereview.chromium.org/2822143003/diff/60001/ui/compositor/layer_uni...
ui/compositor/layer_unittest.cc:1145: // frame to ensure that the mailbox's
release callback has time to run.
Oh but you could also do fwiw..

make a RunLoop, and bind the QuitClosure into the mailbox release callback which
runs it, then RunLoop::Run() instead of drawing again which is kinda a heavy and
indirect way to wait for the callback?

[kylechar, 2017-04-25 16:25:01]

https://codereview.chromium.org/2822143003/diff/60001/ui/compositor/layer_uni...
File ui/compositor/layer_unittest.cc (right):

https://codereview.chromium.org/2822143003/diff/60001/ui/compositor/layer_uni...
ui/compositor/layer_unittest.cc:1145: // frame to ensure that the mailbox's
release callback has time to run.
On 2017/04/25 14:20:42, danakj wrote:
> Oh but you could also do fwiw..
> 
> make a RunLoop, and bind the QuitClosure into the mailbox release callback
which
> runs it, then RunLoop::Run() instead of drawing again which is kinda a heavy
and
> indirect way to wait for the callback?

How does this look?

[danakj, 2017-04-25 16:58:18]

Yeah, thanks :) LGTM

[kylechar, 2017-04-25 17:35:52]

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

[commit-bot: I haz the power, 2017-04-25 17:36:16]

Dry run: CQ is trying da patch.

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

[sky, 2017-04-25 17:40:45]

LGTM

[commit-bot: I haz the power, 2017-04-25 18:44:26]

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

[commit-bot: I haz the power, 2017-04-25 18:44:29]

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

[kylechar, 2017-04-25 18:46:56]

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

[commit-bot: I haz the power, 2017-04-25 18:47:23]

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-25 19:13:51]

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

[commit-bot: I haz the power, 2017-04-25 19:13:52]

Dry run: This issue passed the CQ dry run.

[enne (OOO), 2017-04-26 19:48:20]

lgtm

[kylechar, 2017-04-26 19:48:45]

Thanks!

[kylechar, 2017-04-26 19:48:50]

The CQ bit was checked by kylechar@chromium.org

[kylechar, 2017-04-26 19:48:51]

The patchset sent to the CQ was uploaded after l-g-t-m from ericrk@chromium.org
Link to the patchset: https://codereview.chromium.org/2822143003/#ps100001
(title: "Change to RunLoop.")

[kylechar, 2017-04-26 19:49:12]

The CQ bit was unchecked by kylechar@chromium.org

[kylechar, 2017-04-26 19:50:02]

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

[commit-bot: I haz the power, 2017-04-26 19:50:53]

Dry run: CQ is trying da patch.

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

[kylechar, 2017-04-26 20:42:44]

The CQ bit was unchecked by kylechar@chromium.org

[kylechar, 2017-04-26 20:42:48]

The CQ bit was checked by kylechar@chromium.org

[commit-bot: I haz the power, 2017-04-26 20:43:25]

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-26 21:09:12]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1493239368205360,
"parent_rev": "71c5d05be29f65673640731f6c0fd442e50f6f8d", "commit_rev":
"cf88963a80844b8ed00484ec191e5a952e2b9b07"}

[commit-bot: I haz the power, 2017-04-26 21:09:21]

Description was changed from

==========
Remove CompositorFrameSink::ForceReclaimResources().

This is CL is based off of http://crrev.com/2609253003. The CL was
rebased and ui/compositor/layer_unittest.cc modified to pass. Some
pieces of the original CL had already been landed in separate CLs.

Calling ForceReclaimResources() in LayerTreeHostImpl::BeginCommit()
returns all texture resources and avoid some double buffering overhead.
This optimization won't work across any thread/process boundaries, so
it's only used for the main browser UI compositor and not renderer
compositors.

We are planning on moving the display compositor to a separate process
and this browser UI compositor optimization will no longer be possible
with the mus graphics architecture. This CL removes
CompositorFrameSink::ForceReclaimResources() and associated code.

This will cause a memory regression for Win/Linux/Mac/CrOS. However,
only browser UI tiles that are actively being rastered will be double
buffered, so the regression should be small.

BUG=489515
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Remove CompositorFrameSink::ForceReclaimResources().

This is CL is based off of http://crrev.com/2609253003. The CL was
rebased and ui/compositor/layer_unittest.cc modified to pass. Some
pieces of the original CL had already been landed in separate CLs.

Calling ForceReclaimResources() in LayerTreeHostImpl::BeginCommit()
returns all texture resources and avoid some double buffering overhead.
This optimization won't work across any thread/process boundaries, so
it's only used for the main browser UI compositor and not renderer
compositors.

We are planning on moving the display compositor to a separate process
and this browser UI compositor optimization will no longer be possible
with the mus graphics architecture. This CL removes
CompositorFrameSink::ForceReclaimResources() and associated code.

This will cause a memory regression for Win/Linux/Mac/CrOS. However,
only browser UI tiles that are actively being rastered will be double
buffered, so the regression should be small.

BUG=489515
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

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

[commit-bot: I haz the power, 2017-04-26 21:09:22]

Committed patchset #5 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/cf88963a80844b8ed00484ec191e...

[kylechar, 2017-04-28 14:10:15]

On 2017/04/26 21:09:22, commit-bot: I haz the power wrote:
> Committed patchset #5 (id:100001) as
>
https://chromium.googlesource.com/chromium/src/+/cf88963a80844b8ed00484ec191e...

This produced the same ~500KB memory regression that the previous reverted CL
also introduced. This is expected (unfortunately).

https://chromeperf.appspot.com/report?sid=a82a20f2563b7313176f76d2e7d91ed7d8e...