Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2734783006:
CompositorFrameSinkSupport should return resources before sending an ack (Closed)
|
Created:
3 years, 9 months ago by Saman Sami Modified:
3 years, 9 months ago CC:
chromium-reviews, cc-bugs_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionCompositorFrameSinkSupport should return resources before sending an ack
Apparently after we switched to CompositorFrameSinkSupport in
DelegatedFrameHost, we started using more memory in some performance tests.
We hypothesized that this could be because we send an ack back to the
client before returning the resources and therefore the client cannot reuse
the old resources and has to allocate new ones. I ran the performance tests
that regressed on this CL and I can see clear improvements in memory usage.
Examples of improved metrics (from win_x64_perf_bisect Build #1626):
vm_working_set_delta_size.smpte_3840x2160_60fps_vp9.webm_total from 297.5 MiB to 258.6 MiB
vm_working_set_delta_size.crowd2160.webm_total from 301.1 MiB to 261.3 MiB
BUG=696850
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
Review-Url: https://codereview.chromium.org/2734783006
Cr-Commit-Position: refs/heads/master@{#455646}
Committed: https://chromium.googlesource.com/chromium/src/+/f0f14b715fc62c3e8d9888970ebda21ea7a6e399
Patch Set 1 #Patch Set 2 : Added comment and test #
Total comments: 1
Patch Set 3 : Use MakeCompositorFrameWithResources #
Messages
Total messages: 28 (18 generated)
Description was changed from ========== CompositorFrameSinkSupport should return resources before sending an ack BUG=696850 ========== to ========== CompositorFrameSinkSupport should return resources before sending an ack BUG=696850 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
The CQ bit was checked by samans@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== CompositorFrameSinkSupport should return resources before sending an ack BUG=696850 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== CompositorFrameSinkSupport should return resources before sending an ack Apparently after we switched to CompositorFrameSinkSupport in DelegatedFrameHost, we started using more memory in some performance tests. We hypothesized that this could be because we send an ack back to the client before returning the resources and therefore the client cannot reuse the old resources and has to allocate new ones. I ran the performance tests that regressed on this CL and I can see clear improvements in memory usage. BUG=696850 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
samans@chromium.org changed reviewers: + fsamuel@chromium.org
samans@chromium.org changed reviewers: + danakj@chromium.org
danakj@ and fsamuel@ PTAL
This needs a test :-) Thanks!
On 2017/03/08 18:22:18, Fady Samuel wrote: > This needs a test :-) Thanks! Also, could we please have have a comment? Thanks!
The CQ bit was checked by samans@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL I added comments and a unit test.
lgtm
samans@chromium.org changed reviewers: + jbauman@chromium.org
jbauman@ PTAL
Description was changed from ========== CompositorFrameSinkSupport should return resources before sending an ack Apparently after we switched to CompositorFrameSinkSupport in DelegatedFrameHost, we started using more memory in some performance tests. We hypothesized that this could be because we send an ack back to the client before returning the resources and therefore the client cannot reuse the old resources and has to allocate new ones. I ran the performance tests that regressed on this CL and I can see clear improvements in memory usage. BUG=696850 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== CompositorFrameSinkSupport should return resources before sending an ack Apparently after we switched to CompositorFrameSinkSupport in DelegatedFrameHost, we started using more memory in some performance tests. We hypothesized that this could be because we send an ack back to the client before returning the resources and therefore the client cannot reuse the old resources and has to allocate new ones. I ran the performance tests that regressed on this CL and I can see clear improvements in memory usage. Examples of improved metrics (from win_x64_perf_bisect Build #1626): vm_working_set_delta_size.smpte_3840x2160_60fps_vp9.webm_total from 297.5 MiB to 258.6 MiB vm_working_set_delta_size.crowd2160.webm_total from 301.1 MiB to 261.3 MiB BUG=696850 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
lgtm
Added a nit. https://codereview.chromium.org/2734783006/diff/20001/cc/surfaces/compositor_... File cc/surfaces/compositor_frame_sink_support_unittest.cc (right): https://codereview.chromium.org/2734783006/diff/20001/cc/surfaces/compositor_... cc/surfaces/compositor_frame_sink_support_unittest.cc:976: frame.resource_list.push_back(resource); You could use MakeCompositorFrameWithResources?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by samans@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from fsamuel@chromium.org, jbauman@chromium.org Link to the patchset: https://codereview.chromium.org/2734783006/#ps40001 (title: "Use MakeCompositorFrameWithResources")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1489017245162870,
"parent_rev": "013ac5eaf30c02dfa356ba5399da43eb9f368040", "commit_rev":
"f0f14b715fc62c3e8d9888970ebda21ea7a6e399"}
Message was sent while issue was closed.
Description was changed from ========== CompositorFrameSinkSupport should return resources before sending an ack Apparently after we switched to CompositorFrameSinkSupport in DelegatedFrameHost, we started using more memory in some performance tests. We hypothesized that this could be because we send an ack back to the client before returning the resources and therefore the client cannot reuse the old resources and has to allocate new ones. I ran the performance tests that regressed on this CL and I can see clear improvements in memory usage. Examples of improved metrics (from win_x64_perf_bisect Build #1626): vm_working_set_delta_size.smpte_3840x2160_60fps_vp9.webm_total from 297.5 MiB to 258.6 MiB vm_working_set_delta_size.crowd2160.webm_total from 301.1 MiB to 261.3 MiB BUG=696850 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== CompositorFrameSinkSupport should return resources before sending an ack Apparently after we switched to CompositorFrameSinkSupport in DelegatedFrameHost, we started using more memory in some performance tests. We hypothesized that this could be because we send an ack back to the client before returning the resources and therefore the client cannot reuse the old resources and has to allocate new ones. I ran the performance tests that regressed on this CL and I can see clear improvements in memory usage. Examples of improved metrics (from win_x64_perf_bisect Build #1626): vm_working_set_delta_size.smpte_3840x2160_60fps_vp9.webm_total from 297.5 MiB to 258.6 MiB vm_working_set_delta_size.crowd2160.webm_total from 301.1 MiB to 261.3 MiB BUG=696850 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2734783006 Cr-Commit-Position: refs/heads/master@{#455646} Committed: https://chromium.googlesource.com/chromium/src/+/f0f14b715fc62c3e8d9888970ebd... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/f0f14b715fc62c3e8d9888970ebd... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
