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

Issue 19106007: cc: Allow the main thread to cancel commits (Closed)

Created:
7 years, 5 months ago by enne (OOO)
Modified:
7 years, 4 months ago
CC:
chromium-reviews, cc-bugs_chromium.org, boliu
Visibility:
Public.

Description

cc: Allow the main thread to cancel commits Add a new SetNeedsUpdateLayers that triggers the commit flow, but is abortable if update layers doesn't actually make any changes. This allows the main thread to abort a begin frame. This happens in the case of scroll updates from the compositor thread or invalidations. There was previously an abort begin frame call for when a visibility message and a begin frame message were posted simultaneously, but it incorrectly applied the scrolls and scales without informing the compositor thread that these had already been consumed. To fix this, the abort message passes back a boolean about whether or not the commit was aborted (and needed to be sent again) or was handled (and the scrolls and scales processed). To avoid a deluge of begin frames (in the commit sense) from the scheduler, the scheduler has been adjusted to wait until the next begin frame (in the vsync signal sense) so that these calls can be throttled. Otherwise, the scheduler will just keep trying to begin frame. R=brianderson@chromium.org, danakj@chromium.org BUG=256381 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=213338 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=214314

Patch Set 1 #

Total comments: 12

Patch Set 2 : Rebase #

Patch Set 3 : Address review comments #

Total comments: 3

Patch Set 4 : DCHECK page scale #

Patch Set 5 : cancel_commit => did_handle #

Patch Set 6 : Fix scheduler tests #

Total comments: 37

Patch Set 7 : Address danakj's review comments #

Total comments: 8

Patch Set 8 : Rebase #

Patch Set 9 : danakj review #

Patch Set 10 : Rebase #

Patch Set 11 : New test for texture state #

Total comments: 4

Patch Set 12 : Rebase #

Patch Set 13 : Fix LayerTreeHostScrollTestCaseWithChild #

Patch Set 14 : Rebase #

Patch Set 15 : Rebase #

Patch Set 16 : Move DCHECKs, add test #

Patch Set 17 : Another test #

Patch Set 18 : Rebase #

Patch Set 19 : Remove DCHECKs, set vars directly #

Patch Set 20 : Rebase #

Patch Set 21 : Rebase #

Patch Set 22 : Add more comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+659 lines, -116 lines) Patch
M cc/layers/layer.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 2 chunks +12 lines, -1 line 0 comments Download
M cc/layers/layer.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 3 chunks +17 lines, -7 lines 0 comments Download
M cc/layers/layer_impl.h View 1 1 chunk +2 lines, -0 lines 0 comments Download
M cc/layers/layer_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 2 chunks +21 lines, -2 lines 0 comments Download
M cc/layers/layer_impl_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +72 lines, -0 lines 0 comments Download
M cc/layers/layer_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 5 chunks +5 lines, -4 lines 0 comments Download
M cc/layers/texture_layer_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 2 chunks +2 lines, -1 line 0 comments Download
M cc/scheduler/scheduler.h View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +1 line, -1 line 0 comments Download
M cc/scheduler/scheduler.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +2 lines, -2 lines 0 comments Download
M cc/scheduler/scheduler_state_machine.h View 1 2 3 4 5 6 7 8 9 10 11 5 chunks +7 lines, -3 lines 0 comments Download
M cc/scheduler/scheduler_state_machine.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 11 chunks +43 lines, -16 lines 0 comments Download
M cc/scheduler/scheduler_state_machine_unittest.cc View 1 2 3 4 5 6 7 8 9 10 6 chunks +96 lines, -4 lines 0 comments Download
M cc/scheduler/scheduler_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +8 lines, -3 lines 0 comments Download
M cc/test/fake_content_layer.h View 1 2 3 4 5 6 2 chunks +5 lines, -0 lines 0 comments Download
M cc/test/fake_content_layer.cc View 1 2 3 4 5 6 2 chunks +3 lines, -2 lines 0 comments Download
M cc/test/fake_picture_layer.h View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +7 lines, -3 lines 0 comments Download
M cc/test/fake_picture_layer.cc View 1 2 3 4 5 6 2 chunks +3 lines, -2 lines 0 comments Download
M cc/test/fake_proxy.h View 1 2 3 4 5 6 7 1 chunk +1 line, -0 lines 0 comments Download
M cc/test/layer_test_common.h View 1 chunk +12 lines, -4 lines 0 comments Download
M cc/test/layer_tree_test.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +3 lines, -0 lines 0 comments Download
M cc/test/layer_tree_test.cc View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +3 lines, -3 lines 0 comments Download
M cc/trees/layer_tree_host.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 2 chunks +2 lines, -0 lines 0 comments Download
M cc/trees/layer_tree_host.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 4 chunks +23 lines, -2 lines 0 comments Download
M cc/trees/layer_tree_host_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +3 lines, -1 line 0 comments Download
M cc/trees/layer_tree_host_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 3 chunks +7 lines, -2 lines 0 comments Download
M cc/trees/layer_tree_host_unittest_context.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +1 line, -0 lines 0 comments Download
M cc/trees/layer_tree_host_unittest_damage.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +1 line, -0 lines 0 comments Download
M cc/trees/layer_tree_host_unittest_scroll.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 6 chunks +192 lines, -22 lines 0 comments Download
M cc/trees/layer_tree_impl.h View 1 2 3 4 5 6 7 1 chunk +1 line, -0 lines 0 comments Download
M cc/trees/layer_tree_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +18 lines, -0 lines 0 comments Download
M cc/trees/proxy.h View 1 2 3 4 5 6 7 1 chunk +1 line, -0 lines 0 comments Download
M cc/trees/single_thread_proxy.h View 1 2 3 4 5 6 7 1 chunk +1 line, -0 lines 0 comments Download
M cc/trees/single_thread_proxy.cc View 1 2 3 4 5 6 7 2 chunks +7 lines, -1 line 0 comments Download
M cc/trees/thread_proxy.h View 1 2 3 4 5 6 7 8 9 10 11 4 chunks +5 lines, -1 line 0 comments Download
M cc/trees/thread_proxy.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 9 chunks +72 lines, -29 lines 0 comments Download

Messages

Total messages: 35 (0 generated)
enne (OOO)
7 years, 5 months ago (2013-07-12 23:58:54 UTC) #1
enne (OOO)
Also, sorry about the gigantic patch. I tried to split off the scheduler changes into ...
7 years, 5 months ago (2013-07-13 00:04:14 UTC) #2
brianderson
https://codereview.chromium.org/19106007/diff/1/cc/scheduler/scheduler_state_machine.h File cc/scheduler/scheduler_state_machine.h (right): https://codereview.chromium.org/19106007/diff/1/cc/scheduler/scheduler_state_machine.h#newcode179 cc/scheduler/scheduler_state_machine.h:179: int last_frame_number_where_commit_was_aborted_; Can you change this to be last_frame_number_begin_frame_sent_to_main_thread_ ...
7 years, 5 months ago (2013-07-13 01:26:41 UTC) #3
enne (OOO)
PTAL https://codereview.chromium.org/19106007/diff/1/cc/scheduler/scheduler_state_machine.h File cc/scheduler/scheduler_state_machine.h (right): https://codereview.chromium.org/19106007/diff/1/cc/scheduler/scheduler_state_machine.h#newcode179 cc/scheduler/scheduler_state_machine.h:179: int last_frame_number_where_commit_was_aborted_; On 2013/07/13 01:26:42, Brian Anderson wrote: ...
7 years, 5 months ago (2013-07-15 22:23:28 UTC) #4
brianderson
I haven't taken a look at the layer changes, but the scheduler changes lgtm. https://codereview.chromium.org/19106007/diff/9001/cc/trees/single_thread_proxy.cc ...
7 years, 5 months ago (2013-07-15 23:11:06 UTC) #5
enne (OOO)
On 2013/07/15 23:11:06, Brian Anderson wrote: > I haven't taken a look at the layer ...
7 years, 5 months ago (2013-07-16 00:15:17 UTC) #6
danakj
https://codereview.chromium.org/19106007/diff/33001/cc/layers/layer.cc File cc/layers/layer.cc (right): https://codereview.chromium.org/19106007/diff/33001/cc/layers/layer.cc#newcode580 cc/layers/layer.cc:580: // Note: didScroll() could potentially change the layer structure. ...
7 years, 5 months ago (2013-07-17 20:53:24 UTC) #7
enne (OOO)
https://codereview.chromium.org/19106007/diff/33001/cc/layers/layer.cc File cc/layers/layer.cc (right): https://codereview.chromium.org/19106007/diff/33001/cc/layers/layer.cc#newcode580 cc/layers/layer.cc:580: // Note: didScroll() could potentially change the layer structure. ...
7 years, 5 months ago (2013-07-18 17:36:37 UTC) #8
brianderson
Would like to take a look at this patch a bit more today, but responding ...
7 years, 5 months ago (2013-07-18 18:30:33 UTC) #9
brianderson
lgtm
7 years, 5 months ago (2013-07-18 20:38:14 UTC) #10
danakj
A couple more questions.. https://codereview.chromium.org/19106007/diff/48001/cc/scheduler/scheduler_state_machine.cc File cc/scheduler/scheduler_state_machine.cc (right): https://codereview.chromium.org/19106007/diff/48001/cc/scheduler/scheduler_state_machine.cc#newcode447 cc/scheduler/scheduler_state_machine.cc:447: needs_forced_redraw_after_next_commit_ = false; What about ...
7 years, 5 months ago (2013-07-18 21:57:23 UTC) #11
enne (OOO)
https://codereview.chromium.org/19106007/diff/48001/cc/scheduler/scheduler_state_machine.cc File cc/scheduler/scheduler_state_machine.cc (right): https://codereview.chromium.org/19106007/diff/48001/cc/scheduler/scheduler_state_machine.cc#newcode447 cc/scheduler/scheduler_state_machine.cc:447: needs_forced_redraw_after_next_commit_ = false; On 2013/07/18 21:57:23, danakj wrote: > ...
7 years, 5 months ago (2013-07-19 01:23:50 UTC) #12
danakj
https://codereview.chromium.org/19106007/diff/48001/cc/scheduler/scheduler_state_machine.cc File cc/scheduler/scheduler_state_machine.cc (right): https://codereview.chromium.org/19106007/diff/48001/cc/scheduler/scheduler_state_machine.cc#newcode447 cc/scheduler/scheduler_state_machine.cc:447: needs_forced_redraw_after_next_commit_ = false; On 2013/07/19 01:23:51, enne wrote: > ...
7 years, 5 months ago (2013-07-22 17:16:20 UTC) #13
enne (OOO)
https://codereview.chromium.org/19106007/diff/48001/cc/scheduler/scheduler_state_machine.cc File cc/scheduler/scheduler_state_machine.cc (right): https://codereview.chromium.org/19106007/diff/48001/cc/scheduler/scheduler_state_machine.cc#newcode447 cc/scheduler/scheduler_state_machine.cc:447: needs_forced_redraw_after_next_commit_ = false; On 2013/07/22 17:16:20, danakj wrote: > ...
7 years, 5 months ago (2013-07-22 19:02:42 UTC) #14
danakj
https://codereview.chromium.org/19106007/diff/64001/cc/trees/layer_tree_host_unittest.cc File cc/trees/layer_tree_host_unittest.cc (right): https://codereview.chromium.org/19106007/diff/64001/cc/trees/layer_tree_host_unittest.cc#newcode3033 cc/trees/layer_tree_host_unittest.cc:3033: layer_->set_always_update_resources(true); Is this saying that when we switch to ...
7 years, 5 months ago (2013-07-23 15:35:27 UTC) #15
danakj
On 2013/07/23 15:35:27, danakj wrote: > https://codereview.chromium.org/19106007/diff/64001/cc/trees/layer_tree_host_unittest.cc > File cc/trees/layer_tree_host_unittest.cc (right): > > https://codereview.chromium.org/19106007/diff/64001/cc/trees/layer_tree_host_unittest.cc#newcode3033 > ...
7 years, 5 months ago (2013-07-23 16:51:07 UTC) #16
enne (OOO)
https://codereview.chromium.org/19106007/diff/64001/cc/trees/layer_tree_host_unittest.cc File cc/trees/layer_tree_host_unittest.cc (right): https://codereview.chromium.org/19106007/diff/64001/cc/trees/layer_tree_host_unittest.cc#newcode3033 cc/trees/layer_tree_host_unittest.cc:3033: layer_->set_always_update_resources(true); On 2013/07/23 15:35:27, danakj wrote: > Either way ...
7 years, 5 months ago (2013-07-23 16:58:20 UTC) #17
danakj
https://codereview.chromium.org/19106007/diff/64001/cc/trees/layer_tree_host_unittest.cc File cc/trees/layer_tree_host_unittest.cc (right): https://codereview.chromium.org/19106007/diff/64001/cc/trees/layer_tree_host_unittest.cc#newcode3033 cc/trees/layer_tree_host_unittest.cc:3033: layer_->set_always_update_resources(true); On 2013/07/23 16:58:21, enne wrote: > On 2013/07/23 ...
7 years, 5 months ago (2013-07-23 17:00:11 UTC) #18
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/enne@chromium.org/19106007/64001
7 years, 5 months ago (2013-07-23 17:11:43 UTC) #19
commit-bot: I haz the power
Failed to apply patch for cc/scheduler/scheduler_state_machine.h: While running patch -p1 --forward --force --no-backup-if-mismatch; patching file ...
7 years, 5 months ago (2013-07-23 17:11:53 UTC) #20
boliu
https://chromiumcodereview.appspot.com/19106007/diff/64001/cc/trees/layer_tree_host_unittest.cc File cc/trees/layer_tree_host_unittest.cc (right): https://chromiumcodereview.appspot.com/19106007/diff/64001/cc/trees/layer_tree_host_unittest.cc#newcode3033 cc/trees/layer_tree_host_unittest.cc:3033: layer_->set_always_update_resources(true); On 2013/07/23 16:58:21, enne wrote: > On 2013/07/23 ...
7 years, 5 months ago (2013-07-23 17:19:43 UTC) #21
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/enne@chromium.org/19106007/90001
7 years, 5 months ago (2013-07-23 17:32:48 UTC) #22
commit-bot: I haz the power
Retried try job too often on mac_rel for step(s) cc_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&number=152039
7 years, 5 months ago (2013-07-23 19:08:56 UTC) #23
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/enne@chromium.org/19106007/111001
7 years, 5 months ago (2013-07-23 23:40:40 UTC) #24
commit-bot: I haz the power
Change committed as 213338
7 years, 5 months ago (2013-07-24 04:44:56 UTC) #25
enne (OOO)
Reverted because of: http://build.chromium.org/p/chromium.mac/builders/Mac%2010.6%20Tests%20%28dbg%29%283%29/builds/39500/steps/interactive_ui_tests/logs/EditCommandsNoMenu --snip-- WebViewInteractiveTest.EditCommandsNoMenu: [1288:27139:0723/221703:FATAL:layer_impl.cc(286)] Check failed: TotalScrollOffset().y() <= max_scroll_offset_.y() (62 vs. 47) ...
7 years, 5 months ago (2013-07-25 00:21:52 UTC) #26
mkosiba (inactive)
On 2013/07/25 00:21:52, enne wrote: > Reverted because of: > http://build.chromium.org/p/chromium.mac/builders/Mac%252010.6%2520Tests%2520%2528dbg%2529%25283%2529/builds/39500/steps/interactive_ui_tests/logs/EditCommandsNoMenu > > --snip-- > ...
7 years, 5 months ago (2013-07-25 10:31:55 UTC) #27
enne (OOO)
On 2013/07/25 10:31:55, mkosiba wrote: > > This DCHECK ended up just being bogus. If ...
7 years, 5 months ago (2013-07-26 21:23:37 UTC) #28
enne (OOO)
+mkosiba
7 years, 5 months ago (2013-07-26 21:55:17 UTC) #29
mkosiba (inactive)
On 2013/07/26 21:23:37, enne wrote: > On 2013/07/25 10:31:55, mkosiba wrote: > > > This ...
7 years, 4 months ago (2013-07-29 10:44:28 UTC) #30
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/enne@chromium.org/19106007/157001
7 years, 4 months ago (2013-07-29 17:32:24 UTC) #31
commit-bot: I haz the power
Step "update" is always a major failure. Look at the try server FAQ for more ...
7 years, 4 months ago (2013-07-29 19:19:28 UTC) #32
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/enne@chromium.org/19106007/157001
7 years, 4 months ago (2013-07-29 21:53:41 UTC) #33
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/enne@chromium.org/19106007/157001
7 years, 4 months ago (2013-07-30 00:24:49 UTC) #34
commit-bot: I haz the power
7 years, 4 months ago (2013-07-30 07:17:55 UTC) #35
Message was sent while issue was closed.
Change committed as 214314

Powered by Google App Engine
This is Rietveld 408576698