Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1133673004:
cc: Heuristic for Renderer latency recovery (Closed)
|
Created:
5 years, 7 months ago by brianderson Modified:
5 years, 5 months ago CC:
cc-bugs_chromium.org, chromium-reviews, jdduke (slow), scheduler-bugs_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptioncc: Heuristic for Renderer latency recovery
If the Renderer impl thread can draw before the
deadline and we are getting swap acks from the
Browser into the next BeginFrame, there's
a very high probability that the Renderer is
in a high latency mode but could operate in a
low latency mode.
To recover latency in that case, we skip a
BeginImplFrame.
BUG=406158
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
Committed: https://crrev.com/6d50e7a7df5b33048b9bcd641171b31642b58850
Cr-Commit-Position: refs/heads/master@{#338205}
Patch Set 1 #
Total comments: 5
Patch Set 2 : remove unnecessary BeginImplFrameDeadlinePending reorg #Patch Set 3 : Fix MainFrameInHighLatencyMode #Patch Set 4 : add more unittests #Patch Set 5 : rebase #
Total comments: 11
Patch Set 6 : fix lots of issues; improve test coverage; #
Total comments: 12
Patch Set 7 : rebase; really simplify heuristic #
Total comments: 43
Patch Set 8 : Add SchedulerStateMachine::SwapThrottled #
Total comments: 31
Patch Set 9 : rebase, some tests need fixing #Patch Set 10 : rebase, address comments #
Total comments: 7
Patch Set 11 : rebase, address Tim's comments #
Total comments: 8
Patch Set 12 : improve test readability and coverage #
Total comments: 6
Patch Set 13 : Better names and durations #
Total comments: 30
Patch Set 14 : Sunny's review #Patch Set 15 : So many tests #
Total comments: 2
Patch Set 16 : remove active_tree_needs_first_draw condidtion #
Total comments: 17
Patch Set 17 : Remove unused test method declaration #Patch Set 18 : Sunny's comments #Patch Set 19 : task_runner().RunUntilTime(now_src_->NowTicks());3. #Patch Set 20 : rebaswe #
Messages
Total messages: 56 (12 generated)
brianderson@chromium.org changed reviewers: + mithro@mithis.com, sunnyps@chromium.org
Split this out from the DisplayScheduler patch and added unit tests. This patch should land after the DisplayScheduler patch though, which will make this heuristic more reliable. It might work at well at ToT, but I'm not sure. I'm hoping this will reduce run-to-run latency noise on the bots.
Some comments to make review easier inline. https://codereview.chromium.org/1133673004/diff/1/cc/scheduler/scheduler.cc File cc/scheduler/scheduler.cc (right): https://codereview.chromium.org/1133673004/diff/1/cc/scheduler/scheduler.cc#n... cc/scheduler/scheduler.cc:132: void Scheduler::SetAuthoritativeVSyncInterval(const base::TimeDelta& interval) { SetAuthoritativeVSyncInterval changes are unrelated. I will take them out of this patch. https://codereview.chromium.org/1133673004/diff/1/cc/scheduler/scheduler_unit... File cc/scheduler/scheduler_unittest.cc (right): https://codereview.chromium.org/1133673004/diff/1/cc/scheduler/scheduler_unit... cc/scheduler/scheduler_unittest.cc:1347: scheduler_->DidSwapBuffersComplete(); Needed to add these calls to DidSwapBuffersComplete to avoid ImplThreadIsLikelyHighLatencyMode becoming true. https://codereview.chromium.org/1133673004/diff/1/cc/scheduler/scheduler_unit... cc/scheduler/scheduler_unittest.cc:1393: void SchedulerTest::ImplFrameInHighLatencyMode( Hmm. I need to make a version of this test that tests impl latency recovery when both main and impl are in high latency but we don't recover main thread latency. https://codereview.chromium.org/1133673004/diff/1/cc/test/begin_frame_args_te... File cc/test/begin_frame_args_test.cc (right): https://codereview.chromium.org/1133673004/diff/1/cc/test/begin_frame_args_te... cc/test/begin_frame_args_test.cc:24: BeginFrameArgs::DefaultEstimatedParentDrawTime(), I changed this because the non-zero threshold in Scheduler::CanDrawBeforeDeadline made it difficult for my unittests to make CanDrawBeforeDeadline true if the deadline was half way through the frame. I could have made it work in the tests with really small duration estimates, but decided to change this instead since it better reflects the BeginFrames we'd get in production with hard coded deadlines. https://codereview.chromium.org/1133673004/diff/1/cc/test/scheduler_test_comm... File cc/test/scheduler_test_common.h (right): https://codereview.chromium.org/1133673004/diff/1/cc/test/scheduler_test_comm... cc/test/scheduler_test_common.h:197: bool BeginImplFrameDeadlinePendingForTest() const { I guess this change wasn't strictly necessary in this patch. I was planning to do something with it, but it seems completely unrelated now. I will move it to a separate cleanup patch.
After thinking about it a bit, I think this can land before the DisplayScheduler. It won't detect that the impl thread is in a high latency mode as often as it would with the DisplaySchuler since Surfaces currently draws ASAP. PTAL, at least to comment on the high-level approach.
Left a few comments. I haven't looked at the tests yet. I think a better approach to solving this problem would be to skip renderer frames in the display scheduler where we can surely tell if a renderer is in a high latency mode. https://codereview.chromium.org/1133673004/diff/80001/cc/scheduler/scheduler.cc File cc/scheduler/scheduler.cc (right): https://codereview.chromium.org/1133673004/diff/80001/cc/scheduler/scheduler.... cc/scheduler/scheduler.cc:518: return; BeginFrameSource::DidFinishFrame needs to be called if we're finishing the frame here. https://codereview.chromium.org/1133673004/diff/80001/cc/scheduler/scheduler.... cc/scheduler/scheduler.cc:844: if (!main_thread_is_in_high_latency_mode) { This should depend on if we care about main thread latency i.e. some combination of impl_latency_takes_priority and needs_commit. https://codereview.chromium.org/1133673004/diff/80001/cc/scheduler/scheduler.... cc/scheduler/scheduler.cc:860: base::TimeDelta threshold = BeginFrameArgs::DefaultEstimatedParentDrawTime(); begin_impl_frame_args_.deadline already has the parent draw time adjusted. https://codereview.chromium.org/1133673004/diff/80001/cc/scheduler/scheduler_... File cc/scheduler/scheduler_state_machine.cc (right): https://codereview.chromium.org/1133673004/diff/80001/cc/scheduler/scheduler_... cc/scheduler/scheduler_state_machine.cc:1010: begin_impl_frame_state_ == BEGIN_IMPL_FRAME_STATE_INSIDE_BEGIN_FRAME || I don't understand what the frame state has to do with the renderer being in high latency mode. Consider a renderer that always goes for an immediate deadline at the beginning of the frame. In that case DidSwapBuffersComplete will almost always come in while the state is IDLE. That doesn't imply that we're not in high latency mode. https://codereview.chromium.org/1133673004/diff/80001/cc/scheduler/scheduler_... File cc/scheduler/scheduler_state_machine.h (right): https://codereview.chromium.org/1133673004/diff/80001/cc/scheduler/scheduler_... cc/scheduler/scheduler_state_machine.h:204: bool NeedsCommitForTesting() const { return needs_commit_; } nit: This could be renamed to needs_commit. That way it follows the naming convention for inline methods. Given that this is a const accessor I see no harm in exposing this.
https://codereview.chromium.org/1133673004/diff/80001/cc/scheduler/scheduler.cc File cc/scheduler/scheduler.cc (right): https://codereview.chromium.org/1133673004/diff/80001/cc/scheduler/scheduler.... cc/scheduler/scheduler.cc:518: return; On 2015/05/12 at 00:36:25, sunnyps wrote: > BeginFrameSource::DidFinishFrame needs to be called if we're finishing the frame here. Ah, good catch! I should make sure this logic interacts well with the BackToBackBeginFrameSource. https://codereview.chromium.org/1133673004/diff/80001/cc/scheduler/scheduler.... cc/scheduler/scheduler.cc:844: if (!main_thread_is_in_high_latency_mode) { On 2015/05/12 at 00:36:25, sunnyps wrote: > This should depend on if we care about main thread latency i.e. some combination of impl_latency_takes_priority and needs_commit. main_thread_is_in_high_latency_mode will become true if impl_latency_takes_priority is true, so that part is covered already. I definitely should take into account needs_commit / commit state / and whether there is a pending tree though! Thanks for catching that. https://codereview.chromium.org/1133673004/diff/80001/cc/scheduler/scheduler.... cc/scheduler/scheduler.cc:860: base::TimeDelta threshold = BeginFrameArgs::DefaultEstimatedParentDrawTime(); On 2015/05/12 at 00:36:25, sunnyps wrote: > begin_impl_frame_args_.deadline already has the parent draw time adjusted. I should probably rename CanDrawBeforeDeadline to CanRecoverImplLatency. I tried explaining why I didn't use a threshold of 0 in the comment above. The intent is to only attempt latency recovery when BeginImplFrame->draw takes less than 1/3 the vsync (assuming our fixed deadline estimates). If that happens, and we don't get a swap ack until after > 2/3 of the remaining vsync later (the next BeginFrame), we are almost certainly in a high latency mode. If we use a threshold of 0, I'm worried about races where we think we can draw before the deadline, but can't actually make it. https://codereview.chromium.org/1133673004/diff/80001/cc/scheduler/scheduler_... File cc/scheduler/scheduler_state_machine.cc (right): https://codereview.chromium.org/1133673004/diff/80001/cc/scheduler/scheduler_... cc/scheduler/scheduler_state_machine.cc:1010: begin_impl_frame_state_ == BEGIN_IMPL_FRAME_STATE_INSIDE_BEGIN_FRAME || On 2015/05/12 at 00:36:25, sunnyps wrote: > I don't understand what the frame state has to do with the renderer being in high latency mode. Consider a renderer that always goes for an immediate deadline at the beginning of the frame. In that case DidSwapBuffersComplete will almost always come in while the state is IDLE. That doesn't imply that we're not in high latency mode. Regarding frame state: The code is hard to follow. I think I can use an alternative approach that is easier to follow by using a swap_throttled_at_last_on_begin_impl_frame_ instead of a swap_throttled_in_last_deadline like the current code does. That would remove the need to look at the begin_impl_frame_state here. Regarding your specific concern: The immediate deadline case is covered by the swap_throttled_in_last_deadline logic above, which is used to differentiate between a DidSwapBuffersComplete that came back quickly vs. one that came after the next BeginFrame's deadline since the state is IDLE in both cases. swap_throttled_at_last_on_begin_impl_frame_ could serve the same purpose. https://codereview.chromium.org/1133673004/diff/80001/cc/scheduler/scheduler_... File cc/scheduler/scheduler_state_machine.h (right): https://codereview.chromium.org/1133673004/diff/80001/cc/scheduler/scheduler_... cc/scheduler/scheduler_state_machine.h:204: bool NeedsCommitForTesting() const { return needs_commit_; } On 2015/05/12 at 00:36:25, sunnyps wrote: > nit: This could be renamed to needs_commit. That way it follows the naming convention for inline methods. Given that this is a const accessor I see no harm in exposing this. Done.
> I think a better approach to solving this problem would be to skip renderer frames in the display scheduler where we can surely tell if a renderer is in a high latency mode. Although I think the Browser is the better place to *detect* if the Renderer is in a high latency mode, I'm not sure the logic that determines how to *respond* should also be there. The Renderer has more flexibility to play with outside of simply skipping a BeginFrame. For example, the logic that decides to skip a BeginMainFrame instead of a BeginImplFrame would be more difficult to control from the Browser. https://codereview.chromium.org/1133673004/diff/80001/cc/scheduler/scheduler.cc File cc/scheduler/scheduler.cc (right): https://codereview.chromium.org/1133673004/diff/80001/cc/scheduler/scheduler.... cc/scheduler/scheduler.cc:844: if (!main_thread_is_in_high_latency_mode) { On 2015/05/12 at 19:25:17, brianderson wrote: > On 2015/05/12 at 00:36:25, sunnyps wrote: > > This should depend on if we care about main thread latency i.e. some combination of impl_latency_takes_priority and needs_commit. > > main_thread_is_in_high_latency_mode will become true if impl_latency_takes_priority is true, so that part is covered already. > > I definitely should take into account needs_commit / commit state / and whether there is a pending tree though! Thanks for catching that. Actually, I think I do need to look into impl_latency_takes_priority like you say. I'm going to reuse the begin_impl_frame_deadline_mode_ you added a while back to help me here.
Changed a lot in the latest patch: 1) I got rid of the arbitrary DefaultEstimatedParentDrawTime and instead increased the kDrawDurationEstimatePaddingInMicroseconds from 0 to 1ms. 2) Swaps are skipped within the same frame that we detect that we enter a high latency mode now, the previous one had a delay. 3) The previous patch had a bug where we would skip two frames in a row. This one doesn't do that anymore.
I haven't looked at the test yet and I'm still trying to wrap my head around swaps_are_likely_high_latency. https://codereview.chromium.org/1133673004/diff/100001/cc/scheduler/scheduler.cc File cc/scheduler/scheduler.cc (right): https://codereview.chromium.org/1133673004/diff/100001/cc/scheduler/scheduler... cc/scheduler/scheduler.cc:520: state_machine_.SetSkipNextSwapToReduceLatency(); Can we call FinishImplFrame here instead of going through the BeginImplFrame cycle? That will allows us to get rid of the state tracking code in the state machine. https://codereview.chromium.org/1133673004/diff/100001/cc/scheduler/scheduler... cc/scheduler/scheduler.cc:522: ProcessScheduledActions(); Any particular reason for splitting ProcessScheduledActions out of BeginImplFrame? https://codereview.chromium.org/1133673004/diff/100001/cc/scheduler/scheduler... cc/scheduler/scheduler.cc:838: if (!state_machine_.swaps_are_likely_high_latency()) When is swaps_are_likely_high_latency != pending_swaps >= max_pending_swaps? https://codereview.chromium.org/1133673004/diff/100001/cc/scheduler/scheduler... cc/scheduler/scheduler.cc:841: bool frame_time_is_before_deadline = Maybe we shouldn't adjust the deadline in begin_impl_frame_args_? So that we can compare DrawDurationEstimate with the real deadline here? It's a little unclear what's happening here because the deadline already has DrawDurationEstimate adjusted. https://codereview.chromium.org/1133673004/diff/100001/cc/scheduler/scheduler... cc/scheduler/scheduler.cc:844: auto next_begin_impl_frame_deadline_mode = nit: Let's not use auto here. https://codereview.chromium.org/1133673004/diff/100001/cc/scheduler/scheduler... File cc/scheduler/scheduler_state_machine.cc (right): https://codereview.chromium.org/1133673004/diff/100001/cc/scheduler/scheduler... cc/scheduler/scheduler_state_machine.cc:745: if (!pending_swaps_) nit: Don't use boolean operators on numeric types.
https://codereview.chromium.org/1133673004/diff/100001/cc/scheduler/scheduler.cc File cc/scheduler/scheduler.cc (right): https://codereview.chromium.org/1133673004/diff/100001/cc/scheduler/scheduler... cc/scheduler/scheduler.cc:520: state_machine_.SetSkipNextSwapToReduceLatency(); On 2015/05/21 23:42:23, sunnyps wrote: > Can we call FinishImplFrame here instead of going through the BeginImplFrame > cycle? That will allows us to get rid of the state tracking code in the state > machine. That's a good idea. I'll give it a try! https://codereview.chromium.org/1133673004/diff/100001/cc/scheduler/scheduler... cc/scheduler/scheduler.cc:522: ProcessScheduledActions(); On 2015/05/21 23:42:23, sunnyps wrote: > Any particular reason for splitting ProcessScheduledActions out of > BeginImplFrame? SchedulerStateMachine::OnBeginImplFrame resets the skip_next_... variables. Let me see if there's another way of doing this - it may not be needed if calling FinishImplFrame works. https://codereview.chromium.org/1133673004/diff/100001/cc/scheduler/scheduler... cc/scheduler/scheduler.cc:838: if (!state_machine_.swaps_are_likely_high_latency()) On 2015/05/21 23:42:22, sunnyps wrote: > When is swaps_are_likely_high_latency != pending_swaps >= max_pending_swaps? When the swap ack for a BeginImplFrame(1) comes back after the deadline of the next BeginImplFrame(2). In that case though, we wouldn't have done anything in the BeginImplFrame(2) and shouldn't need to skip the next swap in BeginImplFrame(3). I'll try to simplify the logic. https://codereview.chromium.org/1133673004/diff/100001/cc/scheduler/scheduler... cc/scheduler/scheduler.cc:841: bool frame_time_is_before_deadline = On 2015/05/21 23:42:23, sunnyps wrote: > Maybe we shouldn't adjust the deadline in begin_impl_frame_args_? So that we can > compare DrawDurationEstimate with the real deadline here? It's a little unclear > what's happening here because the deadline already has DrawDurationEstimate > adjusted. Are you saying the logic would be easier to follow that way? Or is there also something wrong with this comparison? I like how the deadline is adjusted by DrawDurationEstimate for tracing purposes - it's easy to see how much time is left before the deadline. https://codereview.chromium.org/1133673004/diff/100001/cc/scheduler/scheduler... cc/scheduler/scheduler.cc:844: auto next_begin_impl_frame_deadline_mode = On 2015/05/21 23:42:23, sunnyps wrote: > nit: Let's not use auto here. Done. https://codereview.chromium.org/1133673004/diff/100001/cc/scheduler/scheduler... File cc/scheduler/scheduler_state_machine.cc (right): https://codereview.chromium.org/1133673004/diff/100001/cc/scheduler/scheduler... cc/scheduler/scheduler_state_machine.cc:745: if (!pending_swaps_) On 2015/05/21 23:42:23, sunnyps wrote: > nit: Don't use boolean operators on numeric types. Done.
The patch is suuper simple now. Thanks for the feedback Sunny. I think the complexity from the previous patch set stemmed from earlier versions I had where the latency recovery was delayed by a frame. This version skips BeginImplFrames entirely and immediately, so we won't PrepareTiles like previous iterations of this patch did, but I think that's okay.
Went through the tests this time. Left a few comments. https://codereview.chromium.org/1133673004/diff/120001/cc/scheduler/scheduler.cc File cc/scheduler/scheduler.cc (right): https://codereview.chromium.org/1133673004/diff/120001/cc/scheduler/scheduler... cc/scheduler/scheduler.cc:509: } else if (ShouldRecoverImplLatency()) { Does it make sense to prioritize skipping impl frame first? I.e. if both impl thread and main thread are in high latency and we know we cannot draw in time we should skip the impl frame but not skip the main frame. https://codereview.chromium.org/1133673004/diff/120001/cc/scheduler/scheduler... cc/scheduler/scheduler.cc:513: return; FinishImplFrame should be called here, right? https://codereview.chromium.org/1133673004/diff/120001/cc/scheduler/scheduler... File cc/scheduler/scheduler_state_machine.h (right): https://codereview.chromium.org/1133673004/diff/120001/cc/scheduler/scheduler... cc/scheduler/scheduler_state_machine.h:148: bool ImplThreadIsLikelyInHighLatencyMode() const { Rename this to IsSwapThrottled or similar. We are only likely in high latency mode if this is true at the start of the frame. https://codereview.chromium.org/1133673004/diff/120001/cc/scheduler/scheduler... File cc/scheduler/scheduler_unittest.cc (right): https://codereview.chromium.org/1133673004/diff/120001/cc/scheduler/scheduler... cc/scheduler/scheduler_unittest.cc:157: return base::TimeDelta::FromHours(1); nit: base::TimeDelta::Max() https://codereview.chromium.org/1133673004/diff/120001/cc/scheduler/scheduler... cc/scheduler/scheduler_unittest.cc:230: public: Why public? https://codereview.chromium.org/1133673004/diff/120001/cc/scheduler/scheduler... cc/scheduler/scheduler_unittest.cc:1394: void SchedulerTest::SwapIsHighLatency( nit: ImplFrameInHighLatencyMode or similar? feel free to ignore. https://codereview.chromium.org/1133673004/diff/120001/cc/scheduler/scheduler... cc/scheduler/scheduler_unittest.cc:1410: SetUpScheduler(make_scoped_ptr(client).Pass(), true); No need to call Pass() here. https://codereview.chromium.org/1133673004/diff/120001/cc/scheduler/scheduler... cc/scheduler/scheduler_unittest.cc:1414: client_->SetAutomaticSwapAck(false); Use either client_ or client consistently. I would prefer client_ and maybe the client can be created inline in SetUpScheduler so that we have no pointers other than client_ to it but that's optional. https://codereview.chromium.org/1133673004/diff/120001/cc/scheduler/scheduler... cc/scheduler/scheduler_unittest.cc:1415: Please make these changes to the MainFrameInHighLatencyMode test too. https://codereview.chromium.org/1133673004/diff/120001/cc/scheduler/scheduler... cc/scheduler/scheduler_unittest.cc:1426: task_runner().RunPendingTasks(); // Run posted deadline. nit: RunTasksWhile(deadline) https://codereview.chromium.org/1133673004/diff/120001/cc/scheduler/scheduler... cc/scheduler/scheduler_unittest.cc:1427: EXPECT_TRUE(client->HasAction("ScheduledActionDrawAndSwapIfPossible")); nit: consider using EXPECT_ACTION https://codereview.chromium.org/1133673004/diff/120001/cc/scheduler/scheduler... cc/scheduler/scheduler_unittest.cc:1435: EXPECT_FALSE(scheduler_->MainThreadIsInHighLatencyMode()); Use SendNextBeginFrame in both cases. Use different expectations (WillBeginImplFrame and deadline) in each case. https://codereview.chromium.org/1133673004/diff/120001/cc/scheduler/scheduler... cc/scheduler/scheduler_unittest.cc:1462: EXPECT_EQ(should_draw_and_swap_in_high_latency, Is this meant to be an implication or equality? If it's implies, change the expect condition. https://codereview.chromium.org/1133673004/diff/120001/cc/scheduler/scheduler... cc/scheduler/scheduler_unittest.cc:1464: if (should_draw_and_swap_in_high_latency) nit: Consider combining this and the above expect using and if - else statement. https://codereview.chromium.org/1133673004/diff/120001/cc/scheduler/scheduler... cc/scheduler/scheduler_unittest.cc:1472: EXPECT_FALSE(scheduler_->MainThreadIsInHighLatencyMode()); There are a bunch of expectations about MainThreadIsInHighLatencyMode and none about SwapIsHighLatency, is that intended? https://codereview.chromium.org/1133673004/diff/120001/cc/scheduler/scheduler... cc/scheduler/scheduler_unittest.cc:1498: // Set up client so that estimates indicate that the commit cannot finish the draw cannot finish before the deadline. https://codereview.chromium.org/1133673004/diff/120001/cc/scheduler/scheduler... cc/scheduler/scheduler_unittest.cc:1504: // Set up client so that estimates indicate that the activate cannot finish commit and activation cannot finish before the deadline. https://codereview.chromium.org/1133673004/diff/120001/cc/scheduler/scheduler... cc/scheduler/scheduler_unittest.cc:1515: auto slow_duration = base::TimeDelta::FromMilliseconds(10); nit: base::TimeDelta instead of auto. https://codereview.chromium.org/1133673004/diff/120001/cc/scheduler/scheduler... cc/scheduler/scheduler_unittest.cc:1534: task_runner().RunPendingTasks(); // Run posted deadline. nit: RunTasksWhile(deadline) instead of RunPendingTasks https://codereview.chromium.org/1133673004/diff/120001/cc/scheduler/scheduler... cc/scheduler/scheduler_unittest.cc:1539: EXPECT_TRUE(client->HasAction("WillBeginImplFrame")); nit: EXPECT_ACTION https://codereview.chromium.org/1133673004/diff/120001/cc/scheduler/scheduler... cc/scheduler/scheduler_unittest.cc:1565: // Note: BeginMainFrame and BeginImplFrame are skipped here because You mean Draw and not BeginImplFrame right? https://codereview.chromium.org/1133673004/diff/120001/cc/scheduler/scheduler... cc/scheduler/scheduler_unittest.cc:1571: client->begin_main_frame_to_commit_duration_ = fast_duration; nit: Use setters instead of accessing member vars directly. Or remove the trailing underscores from the names. https://codereview.chromium.org/1133673004/diff/120001/cc/scheduler/scheduler... cc/scheduler/scheduler_unittest.cc:1581: EXPECT_TRUE(scheduler_->NeedsCommit()); // Previous commit still outstanding. NeedsCommit doesn't mean that previous commit is outstanding - it's also true when we haven't sent a BeginMainFrame yet.
Thanks for the feedback. I didn't get to respond to everything yet, but wanted to post what I have. https://codereview.chromium.org/1133673004/diff/120001/cc/scheduler/scheduler.cc File cc/scheduler/scheduler.cc (right): https://codereview.chromium.org/1133673004/diff/120001/cc/scheduler/scheduler... cc/scheduler/scheduler.cc:513: return; On 2015/05/22 21:15:05, sunnyps wrote: > FinishImplFrame should be called here, right? Ah! I am missing a call to frame_source_->DidFinishFrame() - that would explain why this didn't work well with the --disabe-gpu-vsync. This completely skips calling the BeginImplFrame though - so I don't think I need to do all of FinishImplFrame. https://codereview.chromium.org/1133673004/diff/120001/cc/scheduler/scheduler... File cc/scheduler/scheduler_unittest.cc (right): https://codereview.chromium.org/1133673004/diff/120001/cc/scheduler/scheduler... cc/scheduler/scheduler_unittest.cc:157: return base::TimeDelta::FromHours(1); On 2015/05/22 21:15:05, sunnyps wrote: > nit: base::TimeDelta::Max() Tried this and ended up with test failures - I think because we do math with them and get overflows. https://codereview.chromium.org/1133673004/diff/120001/cc/scheduler/scheduler... cc/scheduler/scheduler_unittest.cc:230: public: On 2015/05/22 21:15:05, sunnyps wrote: > Why public? One of the tests changes the settings dynamically and I was lazy. I'll add setters. https://codereview.chromium.org/1133673004/diff/120001/cc/scheduler/scheduler... cc/scheduler/scheduler_unittest.cc:1472: EXPECT_FALSE(scheduler_->MainThreadIsInHighLatencyMode()); On 2015/05/22 21:15:06, sunnyps wrote: > There are a bunch of expectations about MainThreadIsInHighLatencyMode and none > about SwapIsHighLatency, is that intended? I was able to check expectations in previous patches regarding SwapIsHighLatency because the logic was more complicated to have it always be correct. SwapIsHighLatency only has meaning at the beginning of the frame now - so I can't test the test using that any more. The test still tests the scheduler though.
Hi Brian, Sorry for being so slow to review this CL. Please do ping me if things are blocking you so I make sure to prioritize them. Generally things look pretty good at the moment, most of my comments are around trying to make the tests easier to understand. I'll then be able to make better comments on if Should there also be some sort of change to scheduler_state_machine_unittest.cc too? Tim 'mithro' Ansell PS Missing cc: in the subject. https://codereview.chromium.org/1133673004/diff/140001/cc/scheduler/scheduler.cc File cc/scheduler/scheduler.cc (right): https://codereview.chromium.org/1133673004/diff/140001/cc/scheduler/scheduler... cc/scheduler/scheduler.cc:132: void Scheduler::SetAuthoritativeVSyncInterval(const base::TimeDelta& interval) { Moving this SetAuthoritativeVSyncInterval seems unrelated to the rest of this change? Should it be split out into a separate CL? https://codereview.chromium.org/1133673004/diff/140001/cc/scheduler/scheduler... cc/scheduler/scheduler.cc:133: authoritative_vsync_interval_ = interval; Would it be useful to do a TRACE_EVENT here too? https://codereview.chromium.org/1133673004/diff/140001/cc/scheduler/scheduler... cc/scheduler/scheduler.cc:141: (timebase - base::TimeTicks()).InSecondsF(), "interval", TRACE_EVENT should have native support for base::TimeTicks (see lines 1327-1341 of base/trace_event/trace_event.h), you shouldn't need to do the conversion here. This log should probably be in the separate CL referenced above. https://codereview.chromium.org/1133673004/diff/140001/cc/scheduler/scheduler... cc/scheduler/scheduler.cc:510: TRACE_EVENT_INSTANT0("cc", "SkipBeginImplFrameToReduceLatency", Should this be Scheduler::SkipBeginImplFrameToReduceLatency ? https://codereview.chromium.org/1133673004/diff/140001/cc/scheduler/scheduler... cc/scheduler/scheduler.cc:528: nit: Extra newline? https://codereview.chromium.org/1133673004/diff/140001/cc/scheduler/scheduler... cc/scheduler/scheduler.cc:565: state_machine_.CurrentBeginImplFrameDeadlineMode(); nit: Random new line removal? https://codereview.chromium.org/1133673004/diff/140001/cc/scheduler/scheduler... cc/scheduler/scheduler.cc:833: begin_impl_frame_args_.frame_time < begin_impl_frame_args_.deadline; When is this ever not true? Is it because begin_impl_frame_args_ has been adjusted? https://codereview.chromium.org/1133673004/diff/140001/cc/scheduler/scheduler... cc/scheduler/scheduler.cc:842: return frame_time_is_before_deadline; I can't remember, are we allow to use fall through in Chrome? https://codereview.chromium.org/1133673004/diff/140001/cc/scheduler/scheduler... cc/scheduler/scheduler.cc:862: nit: Unrelated removal? https://codereview.chromium.org/1133673004/diff/140001/cc/scheduler/scheduler.h File cc/scheduler/scheduler.h (right): https://codereview.chromium.org/1133673004/diff/140001/cc/scheduler/scheduler... cc/scheduler/scheduler.h:101: void SetAuthoritativeVSyncInterval(const base::TimeDelta& interval); Moving this SetAuthoritativeVSyncInterval seems unrelated to the rest of this change? Should it be split out into a separate CL? https://codereview.chromium.org/1133673004/diff/140001/cc/scheduler/scheduler... File cc/scheduler/scheduler_unittest.cc (right): https://codereview.chromium.org/1133673004/diff/140001/cc/scheduler/scheduler... cc/scheduler/scheduler_unittest.cc:155: // test latency recovery logic unless a test explicitly wants to. These tests should be deterministic, so I'm not sure why we would be getting transitions where we don't expect? Can you explain what is going on here? Also, wouldn't the most common case for tests to always have very quick operations and be operating in low latency mode? Could we instead add a way to check that the tests never go into the high latency mode unless explicitly requested? https://codereview.chromium.org/1133673004/diff/140001/cc/scheduler/scheduler... cc/scheduler/scheduler_unittest.cc:211: class SchedulerClientWithCustomEstimates : public FakeSchedulerClient { Is there any reason we don't just make this part of FakeSchedulerClient? https://codereview.chromium.org/1133673004/diff/140001/cc/scheduler/scheduler... cc/scheduler/scheduler_unittest.cc:501: new SchedulerClientWithCustomEstimates( It might be worth adding a comment which says which value is for which. Really wish that C++ had keyword arguments like Python. https://codereview.chromium.org/1133673004/diff/140001/cc/scheduler/scheduler... cc/scheduler/scheduler_unittest.cc:1436: if (should_draw_and_swap_in_high_latency) { Instead of using if statements here, does it make more sense to split this into multiple functions? Being explicit in tests makes them easier to understand and work with. https://codereview.chromium.org/1133673004/diff/140001/cc/scheduler/scheduler... cc/scheduler/scheduler_unittest.cc:1440: // skip the BeginImplFrame. I feel like there isn't enough checks in this section of the if statement. https://codereview.chromium.org/1133673004/diff/140001/cc/scheduler/scheduler... cc/scheduler/scheduler_unittest.cc:1452: task_runner().RunPendingTasks(); // Run posted deadline. Please run tasks until a condition is met rather than just randomly running any pending tasks. https://codereview.chromium.org/1133673004/diff/140001/cc/scheduler/scheduler... cc/scheduler/scheduler_unittest.cc:1487: EXPECT_SCOPED(SwapIsHighLatency(1, 1, 1, true, true, false)); It's really hard to understand how the values (1,1,1,true,true,false) end up testing SkipSwapIfHighLatencyAndCanDrawBeforeDeadline_SwapAckThenDeadline. After re-reading the code the 5th time, I'm still unsure your test is actually correct. This makes me feel like the splitting up of SwapIsHighLatency into multiple steps you explicitly call here is a good idea. The big difference between MainFrameInHighLatencyMode code above (that I assume you based this section on) and this section is that; the checks / code in MainFrameInHighLatencyMode are not dependent on the passed in arguments. This means you are not continually going "okay which side of this if statement are we taking" and "does this side of the if statement match what I'm actually trying to test". https://codereview.chromium.org/1133673004/diff/140001/cc/scheduler/scheduler... cc/scheduler/scheduler_unittest.cc:1515: auto slow_duration = base::TimeDelta::FromMilliseconds(10); 10ms it a slow estimate? https://codereview.chromium.org/1133673004/diff/140001/cc/scheduler/scheduler... cc/scheduler/scheduler_unittest.cc:1570: // Lower estimates so that the scheduler will attempt latency recovery. How would this happen in the "real" world? https://codereview.chromium.org/1133673004/diff/140001/cc/scheduler/scheduler... cc/scheduler/scheduler_unittest.cc:1575: // Now that both threads are in a high latency mode, make sure we I think you should have checks here which verify your precondition mentioned in the comment, IE EXPECT_TRUE(scheduler_->MainThreadIsInHighLatencyMode()); https://codereview.chromium.org/1133673004/diff/140001/cc/scheduler/scheduler... cc/scheduler/scheduler_unittest.cc:1584: task_runner().RunPendingTasks(); // Run posted deadline. RunPendingTasks is the devil.... What are you actually running tasks to achieve? https://codereview.chromium.org/1133673004/diff/140001/cc/scheduler/scheduler... cc/scheduler/scheduler_unittest.cc:1595: task_runner().RunPendingTasks(); // Run posted deadline. .... https://codereview.chromium.org/1133673004/diff/140001/cc/test/begin_frame_ar... File cc/test/begin_frame_args_test.cc (right): https://codereview.chromium.org/1133673004/diff/140001/cc/test/begin_frame_ar... cc/test/begin_frame_args_test.cc:24: BeginFrameArgs::DefaultEstimatedParentDrawTime(), nit: I wonder if this should be in a separate CL? https://codereview.chromium.org/1133673004/diff/140001/cc/test/begin_frame_ar... cc/test/begin_frame_args_test.cc:65: BeginFrameArgs::DefaultEstimatedParentDrawTime(), nit: Same as above.... https://codereview.chromium.org/1133673004/diff/140001/cc/trees/proxy_timing_... File cc/trees/proxy_timing_history.cc (right): https://codereview.chromium.org/1133673004/diff/140001/cc/trees/proxy_timing_... cc/trees/proxy_timing_history.cc:12: const int kDrawDurationEstimatePaddingInMicroseconds = 1000; Why this change? This is also a magic number, why do we pad the DrawDurationEstimate with 1ms? This also feels like the wrong place to be doing any padding?
https://codereview.chromium.org/1133673004/diff/140001/cc/scheduler/scheduler.cc File cc/scheduler/scheduler.cc (right): https://codereview.chromium.org/1133673004/diff/140001/cc/scheduler/scheduler... cc/scheduler/scheduler.cc:833: begin_impl_frame_args_.frame_time < begin_impl_frame_args_.deadline; On 2015/05/27 04:40:14, mithro wrote: > When is this ever not true? Is it because begin_impl_frame_args_ has been > adjusted? Yes, if the estimated draw time is 20ms, for example. I will add a comment. https://codereview.chromium.org/1133673004/diff/140001/cc/scheduler/scheduler... cc/scheduler/scheduler.cc:842: return frame_time_is_before_deadline; On 2015/05/27 04:40:14, mithro wrote: > I can't remember, are we allow to use fall through in Chrome? Looks like it is allowed. https://codereview.chromium.org/1133673004/diff/140001/cc/scheduler/scheduler.h File cc/scheduler/scheduler.h (right): https://codereview.chromium.org/1133673004/diff/140001/cc/scheduler/scheduler... cc/scheduler/scheduler.h:101: void SetAuthoritativeVSyncInterval(const base::TimeDelta& interval); On 2015/05/27 04:40:14, mithro wrote: > Moving this SetAuthoritativeVSyncInterval seems unrelated to the rest of this > change? > > Should it be split out into a separate CL? I'll move it to a separate CL. https://codereview.chromium.org/1133673004/diff/140001/cc/test/begin_frame_ar... File cc/test/begin_frame_args_test.cc (right): https://codereview.chromium.org/1133673004/diff/140001/cc/test/begin_frame_ar... cc/test/begin_frame_args_test.cc:24: BeginFrameArgs::DefaultEstimatedParentDrawTime(), On 2015/05/27 04:40:15, mithro wrote: > nit: I wonder if this should be in a separate CL? Will split it out. https://codereview.chromium.org/1133673004/diff/140001/cc/trees/proxy_timing_... File cc/trees/proxy_timing_history.cc (right): https://codereview.chromium.org/1133673004/diff/140001/cc/trees/proxy_timing_... cc/trees/proxy_timing_history.cc:12: const int kDrawDurationEstimatePaddingInMicroseconds = 1000; On 2015/05/27 04:40:15, mithro wrote: > Why this change? > > This is also a magic number, why do we pad the DrawDurationEstimate with 1ms? > > This also feels like the wrong place to be doing any padding? I'll move the padding to the scheduler so it's more obvious. I added the padding to make races with the DisplayScheduler's deadline less likely. We don't account for IPC propogation/descheduling delays anywhere else at the moment. I can move this to a separate patch. Before this patch, if we lost the race then we'd just go into a high latency mode. If we lose the race with this patch though, we might try to recovery the latency and lose the race over and over again. I should probably add logic to make latency recovery less likely in the future if it has failed in the past...
Addressed all comments, ptal. https://codereview.chromium.org/1133673004/diff/120001/cc/scheduler/scheduler.cc File cc/scheduler/scheduler.cc (right): https://codereview.chromium.org/1133673004/diff/120001/cc/scheduler/scheduler... cc/scheduler/scheduler.cc:509: } else if (ShouldRecoverImplLatency()) { On 2015/05/22 21:15:04, sunnyps wrote: > Does it make sense to prioritize skipping impl frame first? I.e. if both impl > thread and main thread are in high latency and we know we cannot draw in time we > should skip the impl frame but not skip the main frame. I prioritize skipping the main frame first for two reasons: 1) ShouldRecoverMainLatency will be true when there is a touch handler and we care about synchronization with the main thread. In that case, skipping an Impl frame will not help with synchornization. 2) Skipping the impl frame then a main frame back-to-back will arguably be more jarring than the other way around since there will be two janks (the swap after the skipped impl frame has an impl-thread discontinuity and the swap after the skipped main frame has an main-thread discontinuity) instead of one (the main and impl discontinuities show up in the same swap). https://codereview.chromium.org/1133673004/diff/120001/cc/scheduler/scheduler... File cc/scheduler/scheduler_state_machine.h (right): https://codereview.chromium.org/1133673004/diff/120001/cc/scheduler/scheduler... cc/scheduler/scheduler_state_machine.h:148: bool ImplThreadIsLikelyInHighLatencyMode() const { On 2015/05/22 21:15:05, sunnyps wrote: > Rename this to IsSwapThrottled or similar. We are only likely in high latency > mode if this is true at the start of the frame. Done. https://codereview.chromium.org/1133673004/diff/120001/cc/scheduler/scheduler... File cc/scheduler/scheduler_unittest.cc (right): https://codereview.chromium.org/1133673004/diff/120001/cc/scheduler/scheduler... cc/scheduler/scheduler_unittest.cc:1410: SetUpScheduler(make_scoped_ptr(client).Pass(), true); On 2015/05/22 21:15:05, sunnyps wrote: > No need to call Pass() here. NA in latest patch set. https://codereview.chromium.org/1133673004/diff/120001/cc/scheduler/scheduler... cc/scheduler/scheduler_unittest.cc:1414: client_->SetAutomaticSwapAck(false); On 2015/05/22 21:15:05, sunnyps wrote: > Use either client_ or client consistently. I would prefer client_ and maybe the > client can be created inline in SetUpScheduler so that we have no pointers other > than client_ to it but that's optional. NA in latest patch set. https://codereview.chromium.org/1133673004/diff/120001/cc/scheduler/scheduler... cc/scheduler/scheduler_unittest.cc:1415: On 2015/05/22 21:15:05, sunnyps wrote: > Please make these changes to the MainFrameInHighLatencyMode test too. Also NA now. Looked into using client_ in all scheduler tests, but most tests need access to the methods exposed by the descendant class. https://codereview.chromium.org/1133673004/diff/120001/cc/scheduler/scheduler... cc/scheduler/scheduler_unittest.cc:1426: task_runner().RunPendingTasks(); // Run posted deadline. On 2015/05/22 21:15:05, sunnyps wrote: > nit: RunTasksWhile(deadline) Done. https://codereview.chromium.org/1133673004/diff/120001/cc/scheduler/scheduler... cc/scheduler/scheduler_unittest.cc:1435: EXPECT_FALSE(scheduler_->MainThreadIsInHighLatencyMode()); On 2015/05/22 21:15:06, sunnyps wrote: > Use SendNextBeginFrame in both cases. Use different expectations > (WillBeginImplFrame and deadline) in each case. Done. https://codereview.chromium.org/1133673004/diff/120001/cc/scheduler/scheduler... cc/scheduler/scheduler_unittest.cc:1462: EXPECT_EQ(should_draw_and_swap_in_high_latency, On 2015/05/22 21:15:05, sunnyps wrote: > Is this meant to be an implication or equality? If it's implies, change the > expect condition. Meant to test equality. Will update comment. https://codereview.chromium.org/1133673004/diff/120001/cc/scheduler/scheduler... cc/scheduler/scheduler_unittest.cc:1464: if (should_draw_and_swap_in_high_latency) On 2015/05/22 21:15:05, sunnyps wrote: > nit: Consider combining this and the above expect using and if - else statement. Done. https://codereview.chromium.org/1133673004/diff/120001/cc/scheduler/scheduler... cc/scheduler/scheduler_unittest.cc:1498: // Set up client so that estimates indicate that the commit cannot finish On 2015/05/22 21:15:05, sunnyps wrote: > the draw cannot finish before the deadline. Done. https://codereview.chromium.org/1133673004/diff/120001/cc/scheduler/scheduler... cc/scheduler/scheduler_unittest.cc:1504: // Set up client so that estimates indicate that the activate cannot finish On 2015/05/22 21:15:05, sunnyps wrote: > commit and activation cannot finish before the deadline. Done. https://codereview.chromium.org/1133673004/diff/120001/cc/scheduler/scheduler... cc/scheduler/scheduler_unittest.cc:1515: auto slow_duration = base::TimeDelta::FromMilliseconds(10); On 2015/05/22 21:15:05, sunnyps wrote: > nit: base::TimeDelta instead of auto. Done. https://codereview.chromium.org/1133673004/diff/120001/cc/scheduler/scheduler... cc/scheduler/scheduler_unittest.cc:1534: task_runner().RunPendingTasks(); // Run posted deadline. On 2015/05/22 21:15:06, sunnyps wrote: > nit: RunTasksWhile(deadline) instead of RunPendingTasks Done. https://codereview.chromium.org/1133673004/diff/120001/cc/scheduler/scheduler... cc/scheduler/scheduler_unittest.cc:1565: // Note: BeginMainFrame and BeginImplFrame are skipped here because On 2015/05/22 21:15:05, sunnyps wrote: > You mean Draw and not BeginImplFrame right? Fixed. https://codereview.chromium.org/1133673004/diff/120001/cc/scheduler/scheduler... cc/scheduler/scheduler_unittest.cc:1571: client->begin_main_frame_to_commit_duration_ = fast_duration; On 2015/05/22 21:15:05, sunnyps wrote: > nit: Use setters instead of accessing member vars directly. Or remove the > trailing underscores from the names. Done. https://codereview.chromium.org/1133673004/diff/120001/cc/scheduler/scheduler... cc/scheduler/scheduler_unittest.cc:1581: EXPECT_TRUE(scheduler_->NeedsCommit()); // Previous commit still outstanding. On 2015/05/22 21:15:05, sunnyps wrote: > NeedsCommit doesn't mean that previous commit is outstanding - it's also true > when we haven't sent a BeginMainFrame yet. Updated the comment. https://codereview.chromium.org/1133673004/diff/140001/cc/scheduler/scheduler.cc File cc/scheduler/scheduler.cc (right): https://codereview.chromium.org/1133673004/diff/140001/cc/scheduler/scheduler... cc/scheduler/scheduler.cc:862: On 2015/05/27 04:40:14, mithro wrote: > nit: Unrelated removal? I added a trace event on line 506 above and removed this one.
The code generally looks good but that I'm still finding it hard to figure out the SchedulerTest::ImplFrameInHighLatency tests. https://codereview.chromium.org/1133673004/diff/180001/cc/scheduler/scheduler.cc File cc/scheduler/scheduler.cc (right): https://codereview.chromium.org/1133673004/diff/180001/cc/scheduler/scheduler... cc/scheduler/scheduler.cc:476: BeginImplFrame(); As discussed, you were going to change this code back to making BeginImplFrame take the args? https://codereview.chromium.org/1133673004/diff/180001/cc/scheduler/scheduler... cc/scheduler/scheduler.cc:771: begin_impl_frame_tracker_.DangerousMethodCurrentOrLast(); This seems wrong? Why does this need the *last* BeginFrameArgs here? https://codereview.chromium.org/1133673004/diff/180001/cc/scheduler/scheduler... File cc/scheduler/scheduler_unittest.cc (right): https://codereview.chromium.org/1133673004/diff/180001/cc/scheduler/scheduler... cc/scheduler/scheduler_unittest.cc:1438: if (should_draw_and_swap_in_high_latency) { I don't quite understand what is going on here in this if statement chain.
Addressed Tim's comments, ptal! https://codereview.chromium.org/1133673004/diff/180001/cc/scheduler/scheduler.cc File cc/scheduler/scheduler.cc (right): https://codereview.chromium.org/1133673004/diff/180001/cc/scheduler/scheduler... cc/scheduler/scheduler.cc:476: BeginImplFrame(); On 2015/07/06 11:58:42, mithro wrote: > As discussed, you were going to change this code back to making BeginImplFrame > take the args? Done. https://codereview.chromium.org/1133673004/diff/180001/cc/scheduler/scheduler... cc/scheduler/scheduler.cc:771: begin_impl_frame_tracker_.DangerousMethodCurrentOrLast(); On 2015/07/06 11:58:42, mithro wrote: > This seems wrong? Why does this need the *last* BeginFrameArgs here? Yeah, I should have just called Current(). Latest patch doesn't start the frame if we decide to skip it, so this is no longer an issue. https://codereview.chromium.org/1133673004/diff/180001/cc/scheduler/scheduler... File cc/scheduler/scheduler_unittest.cc (right): https://codereview.chromium.org/1133673004/diff/180001/cc/scheduler/scheduler... cc/scheduler/scheduler_unittest.cc:1438: if (should_draw_and_swap_in_high_latency) { On 2015/07/06 11:58:43, mithro wrote: > I don't quite understand what is going on here in this if statement chain. I renamed should_draw_and_swap_in_high_latency to expect_begin_impl_frame_in_high_latency, which will hopefully reduce the confusion. (The old variable name was a carryover from the initial patch set.) Or do you think the test should be split up with fewer options in each test?
Hi Brian, Code looks good but the tests could use a little bit of improvement. Tim 'mithro' Ansell https://codereview.chromium.org/1133673004/diff/180001/cc/scheduler/scheduler... File cc/scheduler/scheduler_unittest.cc (right): https://codereview.chromium.org/1133673004/diff/180001/cc/scheduler/scheduler... cc/scheduler/scheduler_unittest.cc:1438: if (should_draw_and_swap_in_high_latency) { On 2015/07/07 02:04:49, brianderson wrote: > On 2015/07/06 11:58:43, mithro wrote: > > I don't quite understand what is going on here in this if statement chain. > > I renamed should_draw_and_swap_in_high_latency to > expect_begin_impl_frame_in_high_latency, which will hopefully reduce the > confusion. (The old variable name was a carryover from the initial patch set.) > > Or do you think the test should be split up with fewer options in each test? > I think you should split the giant SchedulerTest::ImplFrameInHighLatency function into a bunch of smaller functions which you call from the test code. Generally the more "if statements" in a test, the more likely the test is hard to understand and easy to miss what is going on. https://codereview.chromium.org/1133673004/diff/200001/cc/scheduler/scheduler... File cc/scheduler/scheduler_unittest.cc (right): https://codereview.chromium.org/1133673004/diff/200001/cc/scheduler/scheduler... cc/scheduler/scheduler_unittest.cc:1441: EXPECT_ACTION("ScheduledActionAnimate", client_, 1, 2); Why does "has_impl_side_updates" imply a ScheduledActionAnimate? https://codereview.chromium.org/1133673004/diff/200001/cc/scheduler/scheduler... cc/scheduler/scheduler_unittest.cc:1455: scheduler_->DidSwapBuffersComplete(); Why does this path require all the extra Notify calls but the other ones don't? https://codereview.chromium.org/1133673004/diff/200001/cc/scheduler/scheduler... cc/scheduler/scheduler_unittest.cc:1476: scheduler_->DidSwapBuffersComplete(); The expect_begin_impl_frame_in_high_latency seems to call DidSwapBuffersComplete() one more time than when it is false. I believe this is correct, but it is hard to understand why when looking at this test in isolation. https://codereview.chromium.org/1133673004/diff/200001/cc/scheduler/scheduler... cc/scheduler/scheduler_unittest.cc:1543: TEST_F(SchedulerTest, NotSkipSwapIfHighLatencyAndCanDrawTooLong) { nit: CanDrawTooLong? DontSkipSwapIfHighLatencyAndDrawEstimateTooLong?
I'll figure out how to break up the different test configurations. It is pretty hard to follow. https://codereview.chromium.org/1133673004/diff/200001/cc/scheduler/scheduler... File cc/scheduler/scheduler_unittest.cc (right): https://codereview.chromium.org/1133673004/diff/200001/cc/scheduler/scheduler... cc/scheduler/scheduler_unittest.cc:1441: EXPECT_ACTION("ScheduledActionAnimate", client_, 1, 2); On 2015/07/07 07:35:38, mithro wrote: > Why does "has_impl_side_updates" imply a ScheduledActionAnimate? Opened up a a followup bug for this: https://code.google.com/p/chromium/issues/detail?id=507754 https://codereview.chromium.org/1133673004/diff/200001/cc/scheduler/scheduler... cc/scheduler/scheduler_unittest.cc:1455: scheduler_->DidSwapBuffersComplete(); On 2015/07/07 07:35:38, mithro wrote: > Why does this path require all the extra Notify calls but the other ones don't? Because the DidSwapBuffersComplete will trigger a ScheduledActionSendBeginMainFrame in this case, but not in others. https://codereview.chromium.org/1133673004/diff/200001/cc/scheduler/scheduler... cc/scheduler/scheduler_unittest.cc:1476: scheduler_->DidSwapBuffersComplete(); On 2015/07/07 07:35:38, mithro wrote: > The expect_begin_impl_frame_in_high_latency seems to call > DidSwapBuffersComplete() one more time than when it is false. I believe this is > correct, but it is hard to understand why when looking at this test in > isolation. I will add a comment. The ScheduledActionSendBeginMainFrame ultimately results in an extra draw, which needs another DidSwapBuffersComplete to put us in the same low-latency mode that other configurations of the test will be at this point. https://codereview.chromium.org/1133673004/diff/200001/cc/scheduler/scheduler... cc/scheduler/scheduler_unittest.cc:1543: TEST_F(SchedulerTest, NotSkipSwapIfHighLatencyAndCanDrawTooLong) { On 2015/07/07 07:35:38, mithro wrote: > nit: CanDrawTooLong? DontSkipSwapIfHighLatencyAndDrawEstimateTooLong? Yeah, the test names are pretty bad. DontSkipSwapIfHighLatencyAndDrawEstimateTooLong sounds good. Maybe DontSkipImplFrameIfHighLatencyAndDrawEstimateTooLong
Improved the test readability and coverage based on Tim's comments. Ptal.
Hi Brian, Thanks for fixing up those tests, they are much easier to understand now and I'm pretty confident that they are correct and doing what you intended now. Just a couple of minor nits, otherwise LGTM. Tim 'mithro' Ansell https://codereview.chromium.org/1133673004/diff/220001/cc/scheduler/scheduler... File cc/scheduler/scheduler_unittest.cc (right): https://codereview.chromium.org/1133673004/diff/220001/cc/scheduler/scheduler... cc/scheduler/scheduler_unittest.cc:1300: void SchedulerTest::MainFrameInHighLatencyMode( nit: I think this could be named better. My thought is that this method checks what happens to a BeginMainFrame *after* making commit missing an impl frame deadline. Hence something like Check (XXXXX) After (doing YYYY) - IE CheckBeginMainFrameAfterMakingCommitMissImplDeadline(bool expect_send_begin_main_frame); Then a person doesn't even have to understand what is meant by "InHighLatencyMode". https://codereview.chromium.org/1133673004/diff/220001/cc/scheduler/scheduler... cc/scheduler/scheduler_unittest.cc:1327: TEST_F(SchedulerTest, SkipMainFrameIfHighLatencyAndCanDrawBeforeDeadline) { nit: s/IfHighLatency/IfLateCommit/ or maybe IfMissCommit? https://codereview.chromium.org/1133673004/diff/220001/cc/scheduler/scheduler... cc/scheduler/scheduler_unittest.cc:1353: fake_compositor_timing_history_->SetAllEstimatesTo(base::TimeDelta()); nit: It seems like it could be dangerous to be using zero TimeDelta estimates here? https://codereview.chromium.org/1133673004/diff/220001/cc/scheduler/scheduler... cc/scheduler/scheduler_unittest.cc:1355: base::TimeDelta::FromMilliseconds(16)); nit: Should we use a larger value here? https://codereview.chromium.org/1133673004/diff/220001/cc/scheduler/scheduler... cc/scheduler/scheduler_unittest.cc:1390: void SchedulerTest::ImplFrameInHighLatencySkipImplFrame( nit: Name - see comments above too. https://codereview.chromium.org/1133673004/diff/220001/cc/scheduler/scheduler... cc/scheduler/scheduler_unittest.cc:1444: // Also verify the scheduler logic that determines if we are in a high Where is the verification that this comment is referring too?
Addressed latest comments from Tim. Will let https://codereview.chromium.org/1192663005/ marinate a bit before landing this.
The CQ bit was checked by mithro@mithis.com to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from mithro@mithis.com Link to the patchset: https://codereview.chromium.org/1133673004/#ps240001 (title: "Better names and durations")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1133673004/240001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2015/07/08 03:35:38, commit-bot: I haz the power wrote: > Dry run: This issue passed the CQ dry run. BTW I just sent this for a **dry run**. Brian, do you want to actually land it now?
On 2015/07/08 03:53:25, mithro wrote: > On 2015/07/08 03:35:38, commit-bot: I haz the power wrote: > > Dry run: This issue passed the CQ dry run. > > BTW I just sent this for a **dry run**. Brian, do you want to actually land it > now? I'll take a look at this tomorrow morning - didn't get the chance to look at this today :/
On 2015/07/08 04:25:13, sunnyps wrote: > On 2015/07/08 03:53:25, mithro wrote: > > On 2015/07/08 03:35:38, commit-bot: I haz the power wrote: > > > Dry run: This issue passed the CQ dry run. > > > > BTW I just sent this for a **dry run**. Brian, do you want to actually land it > > now? > > I'll take a look at this tomorrow morning - didn't get the chance to look at > this today :/ Np. I'll hold off on landing for now.
Left a "few" comments - most are nits, the most significant one is about the skipping main thread condition. Looks good overall - the tests are great! https://codereview.chromium.org/1133673004/diff/240001/cc/scheduler/scheduler.cc File cc/scheduler/scheduler.cc (right): https://codereview.chromium.org/1133673004/diff/240001/cc/scheduler/scheduler... cc/scheduler/scheduler.cc:758: if (state_machine_.impl_latency_takes_priority()) Might be better to check if deadline mode != LATE. https://codereview.chromium.org/1133673004/diff/240001/cc/scheduler/scheduler... cc/scheduler/scheduler.cc:784: BEGIN_IMPL_FRAME_DEADLINE_MODE_BLOCKED_ON_READY_TO_DRAW: Would be great if we had comments explaining each case. https://codereview.chromium.org/1133673004/diff/240001/cc/scheduler/scheduler... File cc/scheduler/scheduler_state_machine.cc (right): https://codereview.chromium.org/1133673004/diff/240001/cc/scheduler/scheduler... cc/scheduler/scheduler_state_machine.cc:881: skip_next_begin_main_frame_to_reduce_latency_ = false; Why was this moved here? https://codereview.chromium.org/1133673004/diff/240001/cc/scheduler/scheduler... File cc/scheduler/scheduler_state_machine.h (right): https://codereview.chromium.org/1133673004/diff/240001/cc/scheduler/scheduler... cc/scheduler/scheduler_state_machine.h:148: bool SwapThrottled() const { return pending_swaps_ >= max_pending_swaps_; } nit: This does not follow the naming convention for inline methods. Make this non-inline. https://codereview.chromium.org/1133673004/diff/240001/cc/scheduler/scheduler... File cc/scheduler/scheduler_unittest.cc (right): https://codereview.chromium.org/1133673004/diff/240001/cc/scheduler/scheduler... cc/scheduler/scheduler_unittest.cc:1307: task_runner().RunPendingTasks(); // Run posted deadline. nit: RunTasksWhile(DeadlinePending(true)) https://codereview.chromium.org/1133673004/diff/240001/cc/scheduler/scheduler... cc/scheduler/scheduler_unittest.cc:1311: scheduler_->NotifyReadyToActivate(); Can you add the necessary EXPECT_ACTION calls here - just so that we don't miss anything by accisdent? https://codereview.chromium.org/1133673004/diff/240001/cc/scheduler/scheduler... cc/scheduler/scheduler_unittest.cc:1340: MainFrameNotSkippedAfterLateCommitInPreferImplLatencyMode) { This test should be generalized - we want to check that we don't skip a BeginMainFrame if we're not waiting for it to draw. There are conditions other than impl latency takes priority that affect whether we wait to draw or not e.g. active_tree_needs_first_draw. See ShouldTriggerBeginImplFrameDeadlineImmediately. https://codereview.chromium.org/1133673004/diff/240001/cc/scheduler/scheduler... cc/scheduler/scheduler_unittest.cc:1416: // deadline. nit: Move setting the estimates to the test - so that it looks like the other tests. https://codereview.chromium.org/1133673004/diff/240001/cc/scheduler/scheduler... cc/scheduler/scheduler_unittest.cc:1479: scheduler_->SetNeedsCommit(); Shouldn't need to call SetNeedsCommit or SetNeedsRedraw again. Also SetNeedsRedraw is redundant - activation will set the needs_redraw flag. https://codereview.chromium.org/1133673004/diff/240001/cc/scheduler/scheduler... cc/scheduler/scheduler_unittest.cc:1620: fake_compositor_timing_history_->SetDrawDurationEstimate(slow_duration); This doesn't match the comment above. https://codereview.chromium.org/1133673004/diff/240001/cc/scheduler/scheduler... cc/scheduler/scheduler_unittest.cc:1679: EXPECT_FALSE(client_->HasAction("ScheduledActionDrawAndSwapIfPossible")); EXPECT_TRUE that both impl thread and main thread are in high latency mode. https://codereview.chromium.org/1133673004/diff/240001/cc/scheduler/scheduler... cc/scheduler/scheduler_unittest.cc:1693: SendNextBeginFrame(); Same as above (before the deadline runs). https://codereview.chromium.org/1133673004/diff/240001/cc/scheduler/scheduler... cc/scheduler/scheduler_unittest.cc:1695: EXPECT_FALSE(scheduler_->MainThreadIsInHighLatencyMode()); nit: Move this EXPECT below so that it reads sequentially. https://codereview.chromium.org/1133673004/diff/240001/cc/scheduler/scheduler... cc/scheduler/scheduler_unittest.cc:1700: EXPECT_FALSE(client_->HasAction("ScheduledActionSendBeginMainFrame")); nit: Don't need the last EXPECT_FALSE.
https://codereview.chromium.org/1133673004/diff/240001/cc/scheduler/scheduler.cc File cc/scheduler/scheduler.cc (right): https://codereview.chromium.org/1133673004/diff/240001/cc/scheduler/scheduler... cc/scheduler/scheduler.cc:758: if (state_machine_.impl_latency_takes_priority()) On 2015/07/08 22:37:12, sunnyps wrote: > Might be better to check if deadline mode != LATE. I should clarify what I mean here: we should check if we're going to wait for commit before drawing - if we are then it makes sense to skip begin main frame but if we're not then we shouldn't skip begin main frame. Browser impl-side painting makes this more complicated - we might be waiting for READY_TO_DRAW - so do we skip begin main frame in that case? or not?
Addressed most of Sunny's comments. I think I need to add tests for the corner cases of Scheduler::ShouldRecoverImplLatency... https://codereview.chromium.org/1133673004/diff/240001/cc/scheduler/scheduler.cc File cc/scheduler/scheduler.cc (right): https://codereview.chromium.org/1133673004/diff/240001/cc/scheduler/scheduler... cc/scheduler/scheduler.cc:758: if (state_machine_.impl_latency_takes_priority()) On 2015/07/08 22:40:07, sunnyps wrote: > On 2015/07/08 22:37:12, sunnyps wrote: > > Might be better to check if deadline mode != LATE. > > I should clarify what I mean here: we should check if we're going to wait for > commit before drawing - if we are then it makes sense to skip begin main frame > but if we're not then we shouldn't skip begin main frame. We might not wait for the commit because active_tree_needs_first_draw_ is true. However, in that case, we still want to be able to recover the main thread latency. We can make this more general if needed, but I think impl_latency_takes_priority is the only case where we never want to wait for the main thread. > > Browser impl-side painting makes this more complicated - we might be waiting for > READY_TO_DRAW - so do we skip begin main frame in that case? or not? The Browser runs in a forced low-latency mode (where we never send the next BeginMainFrame before the deadline/draw) so this line should never hit. (Note: this is for the main thread latency recovery.) https://codereview.chromium.org/1133673004/diff/240001/cc/scheduler/scheduler... cc/scheduler/scheduler.cc:784: BEGIN_IMPL_FRAME_DEADLINE_MODE_BLOCKED_ON_READY_TO_DRAW: On 2015/07/08 22:37:12, sunnyps wrote: > Would be great if we had comments explaining each case. In writing the comments, I realized I can't rely directly on the deadline mode since being swap throttled (and whether we are currently between a BeginImplFrame and deadline) affects what deadline mode we select. I'll fix up this code and add comments to that. https://codereview.chromium.org/1133673004/diff/240001/cc/scheduler/scheduler... File cc/scheduler/scheduler_state_machine.cc (right): https://codereview.chromium.org/1133673004/diff/240001/cc/scheduler/scheduler... cc/scheduler/scheduler_state_machine.cc:881: skip_next_begin_main_frame_to_reduce_latency_ = false; On 2015/07/08 22:37:12, sunnyps wrote: > Why was this moved here? Resetting it here allowed me to get rid of the skip_begin_main_frame_to_reduce_latency_ variable. https://codereview.chromium.org/1133673004/diff/240001/cc/scheduler/scheduler... File cc/scheduler/scheduler_state_machine.h (right): https://codereview.chromium.org/1133673004/diff/240001/cc/scheduler/scheduler... cc/scheduler/scheduler_state_machine.h:148: bool SwapThrottled() const { return pending_swaps_ >= max_pending_swaps_; } On 2015/07/08 22:37:13, sunnyps wrote: > nit: This does not follow the naming convention for inline methods. Make this > non-inline. Done. https://codereview.chromium.org/1133673004/diff/240001/cc/scheduler/scheduler... File cc/scheduler/scheduler_unittest.cc (right): https://codereview.chromium.org/1133673004/diff/240001/cc/scheduler/scheduler... cc/scheduler/scheduler_unittest.cc:1307: task_runner().RunPendingTasks(); // Run posted deadline. On 2015/07/08 22:37:13, sunnyps wrote: > nit: RunTasksWhile(DeadlinePending(true)) Done. https://codereview.chromium.org/1133673004/diff/240001/cc/scheduler/scheduler... cc/scheduler/scheduler_unittest.cc:1311: scheduler_->NotifyReadyToActivate(); On 2015/07/08 22:37:13, sunnyps wrote: > Can you add the necessary EXPECT_ACTION calls here - just so that we don't miss > anything by accisdent? Done. https://codereview.chromium.org/1133673004/diff/240001/cc/scheduler/scheduler... cc/scheduler/scheduler_unittest.cc:1340: MainFrameNotSkippedAfterLateCommitInPreferImplLatencyMode) { On 2015/07/08 22:37:13, sunnyps wrote: > This test should be generalized - we want to check that we don't skip a > BeginMainFrame if we're not waiting for it to draw. There are conditions other > than impl latency takes priority that affect whether we wait to draw or not e.g. > active_tree_needs_first_draw. See > ShouldTriggerBeginImplFrameDeadlineImmediately. Not sure if this needs to be generalized. See my comment on Scheduler::ShouldRecoverMainLatency. https://codereview.chromium.org/1133673004/diff/240001/cc/scheduler/scheduler... cc/scheduler/scheduler_unittest.cc:1416: // deadline. On 2015/07/08 22:37:13, sunnyps wrote: > nit: Move setting the estimates to the test - so that it looks like the other > tests. Done. https://codereview.chromium.org/1133673004/diff/240001/cc/scheduler/scheduler... cc/scheduler/scheduler_unittest.cc:1479: scheduler_->SetNeedsCommit(); On 2015/07/08 22:37:13, sunnyps wrote: > Shouldn't need to call SetNeedsCommit or SetNeedsRedraw again. Also > SetNeedsRedraw is redundant - activation will set the needs_redraw flag. Ah, these calls are redundant. Will remove them. I'd like to keep the SetNeedsRedraw() above though in case the scheduler tries to swap outside of the BeginImplFrame before we get a new active tree. This isn't a problem with the current approach, but was an issue I ran into with my original approach and could be something to protect against if we decouple draws from the deadline. https://codereview.chromium.org/1133673004/diff/240001/cc/scheduler/scheduler... cc/scheduler/scheduler_unittest.cc:1620: fake_compositor_timing_history_->SetDrawDurationEstimate(slow_duration); On 2015/07/08 22:37:13, sunnyps wrote: > This doesn't match the comment above. Done. https://codereview.chromium.org/1133673004/diff/240001/cc/scheduler/scheduler... cc/scheduler/scheduler_unittest.cc:1679: EXPECT_FALSE(client_->HasAction("ScheduledActionDrawAndSwapIfPossible")); On 2015/07/08 22:37:13, sunnyps wrote: > EXPECT_TRUE that both impl thread and main thread are in high latency mode. Done. https://codereview.chromium.org/1133673004/diff/240001/cc/scheduler/scheduler... cc/scheduler/scheduler_unittest.cc:1693: SendNextBeginFrame(); On 2015/07/08 22:37:13, sunnyps wrote: > Same as above (before the deadline runs). Thanks for the suggestion. Found out I was calling DidSwapBuffersComplete on line 1672 too early. Had to make BeginImplFrameWithDeadline virtual for testing to make this work. https://codereview.chromium.org/1133673004/diff/240001/cc/scheduler/scheduler... cc/scheduler/scheduler_unittest.cc:1695: EXPECT_FALSE(scheduler_->MainThreadIsInHighLatencyMode()); On 2015/07/08 22:37:13, sunnyps wrote: > nit: Move this EXPECT below so that it reads sequentially. Done. https://codereview.chromium.org/1133673004/diff/240001/cc/scheduler/scheduler... cc/scheduler/scheduler_unittest.cc:1700: EXPECT_FALSE(client_->HasAction("ScheduledActionSendBeginMainFrame")); On 2015/07/08 22:37:13, sunnyps wrote: > nit: Don't need the last EXPECT_FALSE. Done.
The CQ bit was checked by brianderson@chromium.org to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from mithro@mithis.com Link to the patchset: https://codereview.chromium.org/1133673004/#ps280001 (title: "So many tests")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1133673004/280001
Added tests for each of the conditions in Scheduler::ShouldRecoverImplLatency. Ptal.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/1133673004/diff/280001/cc/scheduler/scheduler.cc File cc/scheduler/scheduler.cc (right): https://codereview.chromium.org/1133673004/diff/280001/cc/scheduler/scheduler... cc/scheduler/scheduler.cc:793: if (state_machine_.active_tree_needs_first_draw()) Hmm. I think there are a lot of issues with this case. I'm going to remove it and leave hairy cases like this for future improvements. As a heuristic, I think it's too aggressive and will skip frames we don't want to be skipped - for example when the main thread is 30fps and impl thread is 60fps this decision could oscillate too much.
https://codereview.chromium.org/1133673004/diff/280001/cc/scheduler/scheduler.cc File cc/scheduler/scheduler.cc (right): https://codereview.chromium.org/1133673004/diff/280001/cc/scheduler/scheduler... cc/scheduler/scheduler.cc:793: if (state_machine_.active_tree_needs_first_draw()) On 2015/07/09 05:44:06, brianderson wrote: > Hmm. I think there are a lot of issues with this case. I'm going to remove it > and leave hairy cases like this for future improvements. As a heuristic, I think > it's too aggressive and will skip frames we don't want to be skipped - for > example when the main thread is 30fps and impl thread is 60fps this decision > could oscillate too much. Removed.
Should be good to go once these comments are addressed. https://codereview.chromium.org/1133673004/diff/240001/cc/scheduler/scheduler.cc File cc/scheduler/scheduler.cc (right): https://codereview.chromium.org/1133673004/diff/240001/cc/scheduler/scheduler... cc/scheduler/scheduler.cc:784: BEGIN_IMPL_FRAME_DEADLINE_MODE_BLOCKED_ON_READY_TO_DRAW: On 2015/07/09 01:28:11, brianderson wrote: > On 2015/07/08 22:37:12, sunnyps wrote: > > Would be great if we had comments explaining each case. > > In writing the comments, I realized I can't rely directly on the deadline mode > since being swap throttled (and whether we are currently between a > BeginImplFrame and deadline) affects what deadline mode we select. > > I'll fix up this code and add comments to that. > Oh yeah! Nice catch. https://codereview.chromium.org/1133673004/diff/300001/cc/scheduler/scheduler.cc File cc/scheduler/scheduler.cc (right): https://codereview.chromium.org/1133673004/diff/300001/cc/scheduler/scheduler... cc/scheduler/scheduler.cc:778: bool frame_time_is_before_deadline = args.frame_time < args.deadline; nit: Rename this to can_draw_before_deadline. https://codereview.chromium.org/1133673004/diff/300001/cc/scheduler/scheduler.h File cc/scheduler/scheduler.h (right): https://codereview.chromium.org/1133673004/diff/300001/cc/scheduler/scheduler... cc/scheduler/scheduler.h:208: virtual void BeginImplFrameWithDeadline(const BeginFrameArgs& args); Left a comment below about how this isn't necessary. Please remove the virtual and make these private if you address that. https://codereview.chromium.org/1133673004/diff/300001/cc/scheduler/scheduler... cc/scheduler/scheduler.h:231: base::WeakPtrFactory<Scheduler> weak_factory_; weak_factory_ should always be private. https://codereview.chromium.org/1133673004/diff/300001/cc/scheduler/scheduler... File cc/scheduler/scheduler_state_machine.cc (right): https://codereview.chromium.org/1133673004/diff/300001/cc/scheduler/scheduler... cc/scheduler/scheduler_state_machine.cc:1004: needs_commit_ || commit_state_ != COMMIT_STATE_IDLE || has_pending_tree_; has_pending_tree_ implies commit_state_ != COMMIT_STATE_IDLE https://codereview.chromium.org/1133673004/diff/300001/cc/scheduler/scheduler... File cc/scheduler/scheduler_unittest.cc (right): https://codereview.chromium.org/1133673004/diff/300001/cc/scheduler/scheduler... cc/scheduler/scheduler_unittest.cc:394: void ImplFrameSkippedAfterLateSwapAck_MainThreadMakesDeadline( I think the "MainThreadMakesDeadline" suffix is incorrect. https://codereview.chromium.org/1133673004/diff/300001/cc/scheduler/scheduler... cc/scheduler/scheduler_unittest.cc:396: void ImplFrameSkippedAfterLateSwapAck_MainThreadMissesDeadline(); This isn't implemented anywhere. https://codereview.chromium.org/1133673004/diff/300001/cc/scheduler/scheduler... cc/scheduler/scheduler_unittest.cc:1585: task_runner().RunTasksWhile(client_->ImplFrameDeadlinePending(true)); In this mode the deadline will be an immediate one - maybe we want to add an EXPECT for that by running task_runner().RunTasksUntil(Now()) and then checking the deadline was consumed? https://codereview.chromium.org/1133673004/diff/300001/cc/test/scheduler_test... File cc/test/scheduler_test_common.cc (right): https://codereview.chromium.org/1133673004/diff/300001/cc/test/scheduler_test... cc/test/scheduler_test_common.cc:190: was_swap_throttled_at_last_begin_frame_ = state_machine_.SwapThrottled(); I think a better way to do this is to hook up FakeSchedulerClient::WillBeginImplFrame (or SchedulerTest::SendNextBeginFrame) to just record this in the test.
Ptal. https://codereview.chromium.org/1133673004/diff/300001/cc/scheduler/scheduler.cc File cc/scheduler/scheduler.cc (right): https://codereview.chromium.org/1133673004/diff/300001/cc/scheduler/scheduler... cc/scheduler/scheduler.cc:778: bool frame_time_is_before_deadline = args.frame_time < args.deadline; On 2015/07/09 23:44:30, sunnyps wrote: > nit: Rename this to can_draw_before_deadline. Done. https://codereview.chromium.org/1133673004/diff/300001/cc/scheduler/scheduler.h File cc/scheduler/scheduler.h (right): https://codereview.chromium.org/1133673004/diff/300001/cc/scheduler/scheduler... cc/scheduler/scheduler.h:208: virtual void BeginImplFrameWithDeadline(const BeginFrameArgs& args); On 2015/07/09 23:44:30, sunnyps wrote: > Left a comment below about how this isn't necessary. Please remove the virtual > and make these private if you address that. Done. https://codereview.chromium.org/1133673004/diff/300001/cc/scheduler/scheduler... cc/scheduler/scheduler.h:231: base::WeakPtrFactory<Scheduler> weak_factory_; On 2015/07/09 23:44:30, sunnyps wrote: > weak_factory_ should always be private. Done. https://codereview.chromium.org/1133673004/diff/300001/cc/scheduler/scheduler... File cc/scheduler/scheduler_state_machine.cc (right): https://codereview.chromium.org/1133673004/diff/300001/cc/scheduler/scheduler... cc/scheduler/scheduler_state_machine.cc:1004: needs_commit_ || commit_state_ != COMMIT_STATE_IDLE || has_pending_tree_; On 2015/07/09 23:44:30, sunnyps wrote: > has_pending_tree_ implies commit_state_ != COMMIT_STATE_IDLE Usually, but not if main_frame_before_activation_enabled is true. https://codereview.chromium.org/1133673004/diff/300001/cc/scheduler/scheduler... File cc/scheduler/scheduler_unittest.cc (right): https://codereview.chromium.org/1133673004/diff/300001/cc/scheduler/scheduler... cc/scheduler/scheduler_unittest.cc:394: void ImplFrameSkippedAfterLateSwapAck_MainThreadMakesDeadline( On 2015/07/09 23:44:30, sunnyps wrote: > I think the "MainThreadMakesDeadline" suffix is incorrect. Done. https://codereview.chromium.org/1133673004/diff/300001/cc/scheduler/scheduler... cc/scheduler/scheduler_unittest.cc:396: void ImplFrameSkippedAfterLateSwapAck_MainThreadMissesDeadline(); On 2015/07/09 23:44:30, sunnyps wrote: > This isn't implemented anywhere. Removed. https://codereview.chromium.org/1133673004/diff/300001/cc/scheduler/scheduler... cc/scheduler/scheduler_unittest.cc:1585: task_runner().RunTasksWhile(client_->ImplFrameDeadlinePending(true)); On 2015/07/09 23:44:30, sunnyps wrote: > In this mode the deadline will be an immediate one - maybe we want to add an > EXPECT for that by running task_runner().RunTasksUntil(Now()) and then checking > the deadline was consumed? Actually, there is no deadline to run at all since there is no deadline pending! See dcheck on line 1578. I got rid of the task runner call. https://codereview.chromium.org/1133673004/diff/300001/cc/test/scheduler_test... File cc/test/scheduler_test_common.cc (right): https://codereview.chromium.org/1133673004/diff/300001/cc/test/scheduler_test... cc/test/scheduler_test_common.cc:190: was_swap_throttled_at_last_begin_frame_ = state_machine_.SwapThrottled(); On 2015/07/09 23:44:30, sunnyps wrote: > I think a better way to do this is to hook up > FakeSchedulerClient::WillBeginImplFrame (or SchedulerTest::SendNextBeginFrame) > to just record this in the test. I wanted to use FakeSchedulerClient::WillBeginImplFrame initially, but when we skip the ImplFrame, we never get the WillBeginImplFrame. This approach would have worked with the original patch set that skipped the swap rather than the BeginImplFrame, but it's a lot cleaner everywhere else to skip the BeginImplFrame altogether. Using SchedulerTest::SendNextBeginFrame will work for all cases except RetroFrames - but we don't need this method with RetroFrames for all existing tests - and RetroFrames will probably go away soon anyway. In this case, I can just check IsSwapThrottled directly in the tests before calling SendNextBeginFrame, so it's more direct.
LGTM except for one minor issue. https://codereview.chromium.org/1133673004/diff/300001/cc/scheduler/scheduler... File cc/scheduler/scheduler_unittest.cc (right): https://codereview.chromium.org/1133673004/diff/300001/cc/scheduler/scheduler... cc/scheduler/scheduler_unittest.cc:1585: task_runner().RunTasksWhile(client_->ImplFrameDeadlinePending(true)); On 2015/07/10 00:34:08, brianderson wrote: > On 2015/07/09 23:44:30, sunnyps wrote: > > In this mode the deadline will be an immediate one - maybe we want to add an > > EXPECT for that by running task_runner().RunTasksUntil(Now()) and then > checking > > the deadline was consumed? > > Actually, there is no deadline to run at all since there is no deadline pending! > See dcheck on line 1578. I got rid of the task runner call. Oops, I meant the next deadline below for the frame which is not skipped.
On 2015/07/10 00:40:15, sunnyps wrote: > LGTM except for one minor issue. > > https://codereview.chromium.org/1133673004/diff/300001/cc/scheduler/scheduler... > File cc/scheduler/scheduler_unittest.cc (right): > > https://codereview.chromium.org/1133673004/diff/300001/cc/scheduler/scheduler... > cc/scheduler/scheduler_unittest.cc:1585: > task_runner().RunTasksWhile(client_->ImplFrameDeadlinePending(true)); > On 2015/07/10 00:34:08, brianderson wrote: > > On 2015/07/09 23:44:30, sunnyps wrote: > > > In this mode the deadline will be an immediate one - maybe we want to add an > > > EXPECT for that by running task_runner().RunTasksUntil(Now()) and then > > checking > > > the deadline was consumed? > > > > Actually, there is no deadline to run at all since there is no deadline > pending! > > See dcheck on line 1578. I got rid of the task runner call. > > Oops, I meant the next deadline below for the frame which is not skipped. Ah, ok. Updated! Thanks for the detailed reviews Sunny and Tim. Sending this off to the cq.
The CQ bit was checked by brianderson@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mithro@mithis.com, sunnyps@chromium.org Link to the patchset: https://codereview.chromium.org/1133673004/#ps360001 (title: "task_runner().RunUntilTime(now_src_->NowTicks());3.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1133673004/360001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) (exceeded global retry quota)
The CQ bit was checked by brianderson@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sunnyps@chromium.org, mithro@mithis.com Link to the patchset: https://codereview.chromium.org/1133673004/#ps380001 (title: "rebaswe")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1133673004/380001
Message was sent while issue was closed.
Committed patchset #20 (id:380001)
Message was sent while issue was closed.
Patchset 20 (id:??) landed as https://crrev.com/6d50e7a7df5b33048b9bcd641171b31642b58850 Cr-Commit-Position: refs/heads/master@{#338205} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
