Trigger deadline immediately after an aborted main thread commit

Trigger deadline immediately after an aborted main thread commit

If the main thread aborts a commit, check if we should trigger the begin frame
deadline immediately. This fixes a problem where the first frame in response to
a scroll gesture is delayed to the next frame. The sequence of events is:

1. Impl thread receives ScrollBegin event.
2. Impl thread receives BeginFrame event.  3. Impl thread receives ScrollBy
   event. It applies the new scroll offset to the active tree and requests both
   a commit (to tell the main thread about the new scroll offset) and a draw.
3. Commit begins, but the main thread aborts it because only the scroll offset
   changed.
4. Impl thread is told about aborted commit, but instead of drawing the new
   frame it does nothing.

BUG=306958
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=229115

[Sami, 2013-10-14 16:44:14]

N.B. this patch is mainly for discussion since we're not sure how to best solve
this yet.

[brianderson, 2013-10-15 10:00:22]

I think this solution will actually work very well. It prioritizes impl-thread
latency when the main thread isn't producing anything.

If the main thread requests a commit before the BeginImplFrame, the commit state
wont be idle and the deadline won't trigger early. This should be the case for
RAF most of the time, so we wont be penalizing RAF.

If the main thread requests a commit after the BeginImplFrame, the impl thread
will trigger the deadline early, which will penalize the main thread; but I
think that's the only safe thing to do if our deadline estimation isn't
accurate.

This patch probably wouldn't be needed if the deadline estimations were more
accurate, so we should revisit this logic once they are.

https://codereview.chromium.org/27200003/diff/1/cc/scheduler/scheduler_state_...
File cc/scheduler/scheduler_state_machine.cc (right):

https://codereview.chromium.org/27200003/diff/1/cc/scheduler/scheduler_state_...
cc/scheduler/scheduler_state_machine.cc:883: if (commit_state_ ==
COMMIT_STATE_IDLE && needs_redraw_)
Can you reorganize this method so that it checks the begin_frame_state before
this point to early out and returns false at the end?

In this if statement, can you also add "&& has_pending_tree". At ToT, the
commit_state_ will not be idle when has_pending_tree is true, but that will
change after https://codereview.chromium.org/23907006.

[Sami, 2013-10-15 14:33:15]

Thanks, Brian. I've updated the patch accordingly (N.B. !has_pending_tree
instead has_pending_tree) and added a test. PTAL.

[brianderson, 2013-10-15 15:02:31]

https://codereview.chromium.org/27200003/diff/6001/cc/scheduler/scheduler_sta...
File cc/scheduler/scheduler_state_machine.cc (right):

https://codereview.chromium.org/27200003/diff/6001/cc/scheduler/scheduler_sta...
cc/scheduler/scheduler_state_machine.cc:886: return true;
Sorry, I meant to split this into two conditions:

if (begin_frame_state_ != BEGIN_FRAME_STATE_INSIDE_BEGIN_FRAME)
  return false;

if (active_tree_needs_first_draw_)
  return true;

That way, the whole method will not return true if there isn't a deadline
already pending.

[Sami, 2013-10-15 15:12:51]

https://codereview.chromium.org/27200003/diff/6001/cc/scheduler/scheduler_sta...
File cc/scheduler/scheduler_state_machine.cc (right):

https://codereview.chromium.org/27200003/diff/6001/cc/scheduler/scheduler_sta...
cc/scheduler/scheduler_state_machine.cc:886: return true;
On 2013/10/15 15:02:32, Brian Anderson wrote:
> Sorry, I meant to split this into two conditions:
> 
> if (begin_frame_state_ != BEGIN_FRAME_STATE_INSIDE_BEGIN_FRAME)
>   return false;
> 
> if (active_tree_needs_first_draw_)
>   return true;
> 
> That way, the whole method will not return true if there isn't a deadline
> already pending.

Done.

[brianderson, 2013-10-15 15:54:02]

lgtm. Thanks for the test and thanks for catching the regression.

[commit-bot: I haz the power, 2013-10-15 16:02:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skyostil@chromium.org/27200003/15001

[commit-bot: I haz the power, 2013-10-15 16:57:48]

Retried try job too often on linux_rel for step(s) cc_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-10-16 10:38:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skyostil@chromium.org/27200003/36001

[commit-bot: I haz the power, 2013-10-16 19:46:43]

Retried try job too often on win7_aura for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...

[commit-bot: I haz the power, 2013-10-17 09:07:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skyostil@chromium.org/27200003/36001

[commit-bot: I haz the power, 2013-10-17 13:13:01]

Change committed as 229115