Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(88)

Issue 2726263003: cc::ResourcePool - Re-use larger resources for smaller requests (Closed)

Created:
3 years, 9 months ago by ericrk
Modified:
3 years, 7 months ago
CC:
cc-bugs_chromium.org, chromium-reviews
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

cc::ResourcePool - Re-use larger resources for smaller requests Allows cc::ResourcePool to re-use larger resources to fulfill a smaller request. This avoids expensive allocations, and is especially useful in cases where layer sizes are changing due to animation. Also extends the timeout for resource expiration from 1s to 5s, which will also cut down on resource allocations. CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2726263003 Cr-Original-Commit-Position: refs/heads/master@{#456202} Committed: https://chromium.googlesource.com/chromium/src/+/830cb220146684c38b9aa4da3f39e96c37a59f33 Review-Url: https://codereview.chromium.org/2726263003 Cr-Commit-Position: refs/heads/master@{#470023} Committed: https://chromium.googlesource.com/chromium/src/+/0fc488920dd2915f5596dda347681e0571bb56dc

Patch Set 1 #

Patch Set 2 : build fix #

Total comments: 3

Patch Set 3 : Base re-use threshold on pixel area, not individual dimensions. #

Total comments: 4

Patch Set 4 : improve unit tests #

Patch Set 5 : Fix one copy issues #

Patch Set 6 : cc::ResourcePool - Re-use larger resources for smaller requests #

Patch Set 7 : rebase #

Total comments: 10

Patch Set 8 : rebase #

Patch Set 9 : feedback #

Patch Set 10 : fix compile #

Unified diffs Side-by-side diffs Delta from patch set Stats (+195 lines, -64 lines) Patch
M cc/base/switches.h View 1 2 3 4 5 1 chunk +2 lines, -1 line 0 comments Download
M cc/base/switches.cc View 1 2 3 4 5 6 7 8 1 chunk +5 lines, -0 lines 0 comments Download
M cc/output/gl_renderer.cc View 1 2 3 4 5 6 7 1 chunk +2 lines, -1 line 0 comments Download
M cc/output/renderer_settings.h View 1 2 3 4 5 6 7 8 1 chunk +5 lines, -0 lines 0 comments Download
M cc/raster/one_copy_raster_buffer_provider.h View 1 2 3 4 1 chunk +1 line, -2 lines 0 comments Download
M cc/raster/one_copy_raster_buffer_provider.cc View 1 2 3 4 3 chunks +7 lines, -9 lines 0 comments Download
M cc/resources/resource_pool.h View 1 2 3 4 5 6 7 8 3 chunks +14 lines, -6 lines 0 comments Download
M cc/resources/resource_pool.cc View 1 2 3 4 5 4 chunks +40 lines, -4 lines 0 comments Download
M cc/resources/resource_pool_unittest.cc View 1 2 3 4 5 6 7 8 7 chunks +90 lines, -35 lines 0 comments Download
M cc/test/fake_picture_layer_tiling_client.cc View 1 2 3 4 5 1 chunk +2 lines, -1 line 0 comments Download
M cc/test/layer_tree_pixel_resource_test.cc View 1 2 3 4 5 6 1 chunk +1 line, -1 line 0 comments Download
M cc/trees/layer_tree_host_impl.cc View 1 2 3 4 5 6 7 8 9 4 chunks +8 lines, -4 lines 0 comments Download
M cc/trees/layer_tree_settings.h View 1 2 3 4 5 6 7 8 1 chunk +5 lines, -0 lines 0 comments Download
M content/browser/renderer_host/render_process_host_impl.cc View 1 2 3 4 5 6 7 8 9 1 chunk +1 line, -0 lines 0 comments Download
M content/renderer/gpu/render_widget_compositor.cc View 1 2 3 4 5 6 7 8 9 1 chunk +5 lines, -0 lines 0 comments Download
M content/shell/app/shell_main_delegate.cc View 1 2 3 4 5 6 1 chunk +2 lines, -0 lines 0 comments Download
M ui/compositor/compositor.cc View 1 2 3 4 5 6 7 8 9 1 chunk +5 lines, -0 lines 0 comments Download

Messages

Total messages: 89 (57 generated)
ericrk
Let me know if this makes sense. Returning larger resources seems to work fine (we ...
3 years, 9 months ago (2017-03-10 01:51:14 UTC) #12
danakj
https://codereview.chromium.org/2726263003/diff/20001/cc/resources/resource_pool.cc File cc/resources/resource_pool.cc (right): https://codereview.chromium.org/2726263003/diff/20001/cc/resources/resource_pool.cc#newcode30 cc/resources/resource_pool.cc:30: const float kReuseScaleThreshold = 2.0f; driveby constant suggestion of ...
3 years, 9 months ago (2017-03-10 16:32:12 UTC) #13
enne (OOO)
lgtm % danakj's suggestion
3 years, 9 months ago (2017-03-10 18:26:46 UTC) #14
ericrk
https://codereview.chromium.org/2726263003/diff/20001/cc/resources/resource_pool.cc File cc/resources/resource_pool.cc (right): https://codereview.chromium.org/2726263003/diff/20001/cc/resources/resource_pool.cc#newcode30 cc/resources/resource_pool.cc:30: const float kReuseScaleThreshold = 2.0f; On 2017/03/10 at 16:32:12, ...
3 years, 9 months ago (2017-03-10 18:44:21 UTC) #17
danakj
https://codereview.chromium.org/2726263003/diff/60001/cc/resources/resource_pool.cc File cc/resources/resource_pool.cc (right): https://codereview.chromium.org/2726263003/diff/60001/cc/resources/resource_pool.cc#newcode40 cc/resources/resource_pool.cc:40: float actual_area = actual_size.GetArea(); This will crash if it ...
3 years, 9 months ago (2017-03-10 18:55:40 UTC) #20
ericrk
https://codereview.chromium.org/2726263003/diff/60001/cc/resources/resource_pool.cc File cc/resources/resource_pool.cc (right): https://codereview.chromium.org/2726263003/diff/60001/cc/resources/resource_pool.cc#newcode40 cc/resources/resource_pool.cc:40: float actual_area = actual_size.GetArea(); On 2017/03/10 at 18:55:40, danakj ...
3 years, 9 months ago (2017-03-10 20:04:07 UTC) #24
danakj
Thanks! LGTM
3 years, 9 months ago (2017-03-10 20:05:15 UTC) #27
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2726263003/140001
3 years, 9 months ago (2017-03-10 20:09:13 UTC) #31
commit-bot: I haz the power
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_ng/builds/404781)
3 years, 9 months ago (2017-03-10 20:59:58 UTC) #33
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2726263003/140001
3 years, 9 months ago (2017-03-10 22:27:13 UTC) #35
commit-bot: I haz the power
Committed patchset #4 (id:140001) as https://chromium.googlesource.com/chromium/src/+/830cb220146684c38b9aa4da3f39e96c37a59f33
3 years, 9 months ago (2017-03-10 22:52:16 UTC) #38
Xianzhu
A revert of this CL (patchset #4 id:140001) has been created in https://codereview.chromium.org/2744063002/ by wangxianzhu@chromium.org. ...
3 years, 9 months ago (2017-03-11 02:16:41 UTC) #39
ericrk
On 2017/03/11 02:16:41, Xianzhu wrote: > A revert of this CL (patchset #4 id:140001) has ...
3 years, 9 months ago (2017-03-11 02:52:01 UTC) #40
ericrk
+mkwst@ for content/shell/app/shell_main_delegate.cc enne@ or danakj@, can you take another quick look at this. Basically ...
3 years, 7 months ago (2017-05-04 21:39:27 UTC) #51
danakj
https://codereview.chromium.org/2726263003/diff/200001/cc/base/switches.cc File cc/base/switches.cc (right): https://codereview.chromium.org/2726263003/diff/200001/cc/base/switches.cc#newcode121 cc/base/switches.cc:121: // Intended only for use in layout tests to ...
3 years, 7 months ago (2017-05-04 21:48:37 UTC) #52
enne (OOO)
lgtm. I don't have anything to add over danakj's comments.
3 years, 7 months ago (2017-05-05 00:52:03 UTC) #53
Mike West
//content/shell LGTM, but I generally prefer that we do relands of reverts in separate patches.
3 years, 7 months ago (2017-05-05 09:36:03 UTC) #54
enne (OOO)
On 2017/05/05 at 09:36:03, mkwst wrote: > //content/shell LGTM, but I generally prefer that we ...
3 years, 7 months ago (2017-05-05 17:03:14 UTC) #55
ericrk
+ccameron@ for render_process_host_impl.cc change (just adding a flag to be passed through). https://codereview.chromium.org/2726263003/diff/200001/cc/base/switches.cc File cc/base/switches.cc ...
3 years, 7 months ago (2017-05-05 23:18:38 UTC) #60
ccameron
On 2017/05/05 23:18:38, ericrk wrote: > +ccameron@ for render_process_host_impl.cc change (just adding a flag to ...
3 years, 7 months ago (2017-05-06 05:43:13 UTC) #65
ccameron
On 2017/05/06 05:43:13, ccameron wrote: > On 2017/05/05 23:18:38, ericrk wrote: > > +ccameron@ for ...
3 years, 7 months ago (2017-05-06 05:59:17 UTC) #66
ericrk
On 2017/05/06 05:59:17, ccameron wrote: > On 2017/05/06 05:43:13, ccameron wrote: > > On 2017/05/05 ...
3 years, 7 months ago (2017-05-07 19:45:14 UTC) #67
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2726263003/320001
3 years, 7 months ago (2017-05-07 19:53:02 UTC) #70
commit-bot: I haz the power
Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_compile_dbg/builds/263802)
3 years, 7 months ago (2017-05-07 20:32:29 UTC) #72
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2726263003/320001
3 years, 7 months ago (2017-05-07 20:35:48 UTC) #74
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_chromeos_ozone_rel_ng/builds/378270)
3 years, 7 months ago (2017-05-07 22:34:27 UTC) #76
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2726263003/320001
3 years, 7 months ago (2017-05-07 23:34:43 UTC) #78
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_chromeos_ozone_rel_ng/builds/378294) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, ...
3 years, 7 months ago (2017-05-08 00:52:40 UTC) #80
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2726263003/320001
3 years, 7 months ago (2017-05-08 01:05:48 UTC) #82
commit-bot: I haz the power
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_rel_ng/builds/447191)
3 years, 7 months ago (2017-05-08 02:27:15 UTC) #84
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2726263003/320001
3 years, 7 months ago (2017-05-08 15:53:04 UTC) #86
commit-bot: I haz the power
3 years, 7 months ago (2017-05-08 17:04:04 UTC) #89
Message was sent while issue was closed.
Committed patchset #10 (id:320001) as
https://chromium.googlesource.com/chromium/src/+/0fc488920dd2915f5596dda34768...

Powered by Google App Engine
This is Rietveld 408576698