Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1320633002:
Optimize for TouchStart responsiveness (Closed)
|
Created:
5 years, 3 months ago by alex clarke (OOO till 29th) Modified:
5 years, 3 months ago CC:
cc-bugs_chromium.org, chromium-reviews, scheduler-bugs_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionOptimize for TouchStart responsiveness by modeling what the user is doing.
We have come to realize that TouchStart responsiveness is really critical
for a good user experience.
This patch introduces a (initially very simple) system for modeling
what the user is doing and predicting when they are likely to initiate a
new gesture (taps are ignored). We order task priority accordingly.
In addition we extend the estimation of task cost to loading tasks
which are often a significant source of jank.
Load tasks can be very long (easily up to 1000ms). There is
no good way to cooperatively schedule such a task but the
least worst user experience seems to be if we get them out
of the way as fast as possible. Ideally we'd prioritize them
but the order of IPC and LoadingTasks currently needs to
be more or less in lockstep. Instead we deprioritize compositor
tasks inside a compositor gesture, which has the effect of
prioritizing other work.
Expected changes on Perf bots:
1. first_gesture_scroll_update_latency in smoothness.scrolling_tough_ad_cases
should go down. This should be highly user visible.
2. It's possible some compositor metrics such as queueing_durations
may regress. I don't expect this to be user visible.
BUG=510398
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
Committed: https://crrev.com/464b1a5144ddb5135dad845aceb5b1f074221387
Cr-Commit-Position: refs/heads/master@{#347937}
Patch Set 1 #Patch Set 2 : Rebase + remove some unwanted changes #
Total comments: 42
Patch Set 3 : Introduced a UserModel class #
Total comments: 17
Patch Set 4 : Fix moFix most of Sami's concerns #Patch Set 5 : Fix dodgy merge #Patch Set 6 : Responding to feedback #Patch Set 7 : Rebase plus fix destructor bug #Patch Set 8 : Fix small bug in UpdateForInputEventOnCompositorThread #
Total comments: 10
Patch Set 9 : Rebased + changes for Sami #
Total comments: 2
Patch Set 10 : No longer prioritizing loading tasks by default. #Patch Set 11 : Fix a test #Patch Set 12 : Fix unit tests and prioritize default tasks when loading tasks are priotized. #
Total comments: 4
Patch Set 13 : NONE prioritization is gone #
Total comments: 2
Patch Set 14 : Fix bug with default TQ not being checked when comparing policies #Patch Set 15 : Deprioritize compositor tasks during the first part of UseCase::COMPOSITOR_GESTURE #Patch Set 16 : Reset cost estimation on navigation comit #Patch Set 17 : Remove code to surpress blocking of expensive tasks in the 1st sec, the reset should suffice. #Patch Set 18 : Handle taps differently from scrolls in the UserModel #Patch Set 19 : TEST #Patch Set 20 : TEST2 #Patch Set 21 : TEST£ #Patch Set 22 : Try set OnPageLoadStarted in RenderFrameImpl::OnNavigate #Messages
Total messages: 88 (36 generated)
The CQ bit was checked by alexclarke@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1320633002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1320633002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: 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...)
The CQ bit was checked by alexclarke@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1320633002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1320633002/20001
alexclarke@chromium.org changed reviewers: + skyostil@chromium.org
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_...)
The new use case based approach looks awesome. https://codereview.chromium.org/1320633002/diff/20001/cc/trees/thread_proxy.cc File cc/trees/thread_proxy.cc (right): https://codereview.chromium.org/1320633002/diff/20001/cc/trees/thread_proxy.c... cc/trees/thread_proxy.cc:1183: // Put the scheduler in impl latency prioritization mode if there's no new nit: the comment isn't entirely accurate (we might still have new content but we're just not prioritizing it). Could you split this change into its own patch since I think it'd be useful by itself and might trigger interesting things on the perf bots? Mainly I'm wondering what this will do to the scroll update metrics and how much we should care about that. One thing we could try would be to ship both bits (BeginMainFrame is critical, a scroll handler is being targeted) separately and then prioritizing scrolling if it looks like it can do the job fast enough. WDYT? https://codereview.chromium.org/1320633002/diff/20001/components/scheduler/re... File components/scheduler/renderer/renderer_scheduler_impl.cc (left): https://codereview.chromium.org/1320633002/diff/20001/components/scheduler/re... components/scheduler/renderer/renderer_scheduler_impl.cc:543: // TODO(skyostil): Consider removing in_idle_period_ and Wanna keep this todo since the code is still here? https://codereview.chromium.org/1320633002/diff/20001/components/scheduler/re... File components/scheduler/renderer/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/1320633002/diff/20001/components/scheduler/re... components/scheduler/renderer/renderer_scheduler_impl.cc:21: const double kTimerTaskEstimationPercentile = 90; Any reason for tweaking these in this way? https://codereview.chromium.org/1320633002/diff/20001/components/scheduler/re... components/scheduler/renderer/renderer_scheduler_impl.cc:71: base::TimeDelta::FromMilliseconds(16)); BeginFrameArgs::DefaultInterval()? https://codereview.chromium.org/1320633002/diff/20001/components/scheduler/re... components/scheduler/renderer/renderer_scheduler_impl.cc:308: CASE_TYPE(Undefined); Instead of duplicating this from RenderWidget, could we move the list there to a common place? https://codereview.chromium.org/1320633002/diff/20001/components/scheduler/re... components/scheduler/renderer/renderer_scheduler_impl.cc:445: case UseCase::NOT_SCROLLING: Could we not be expecting a scroll in this state? https://codereview.chromium.org/1320633002/diff/20001/components/scheduler/re... components/scheduler/renderer/renderer_scheduler_impl.cc:522: if (AnyThread().last_touchstart_time_.is_null() || Touchstart doesn't get sent if there are no touch handlers, so I think we need to track the scroll gesture instead. https://codereview.chromium.org/1320633002/diff/20001/components/scheduler/re... components/scheduler/renderer/renderer_scheduler_impl.cc:531: return false; Should we always return true for main thread scrolling? https://codereview.chromium.org/1320633002/diff/20001/components/scheduler/re... components/scheduler/renderer/renderer_scheduler_impl.cc:587: new_policy.timer_queue_priority_ = TaskQueue::DISABLED_PRIORITY; block_expensive_tasks = true? https://codereview.chromium.org/1320633002/diff/20001/components/scheduler/re... components/scheduler/renderer/renderer_scheduler_impl.cc:599: new_policy.timer_queue_priority_ = TaskQueue::HIGH_PRIORITY; Why are timers now high priority? They used to be best effort. https://codereview.chromium.org/1320633002/diff/20001/components/scheduler/re... components/scheduler/renderer/renderer_scheduler_impl.cc:627: use_case == MainThreadOnly().current_use_case_ && Do we need to check if the use case changed? Could we just always save the computed use case and only look at the policy after that? https://codereview.chromium.org/1320633002/diff/20001/components/scheduler/re... components/scheduler/renderer/renderer_scheduler_impl.cc:666: // No., so arrange for compositor tasks to be run at the highest priority. typo: . https://codereview.chromium.org/1320633002/diff/20001/components/scheduler/re... components/scheduler/renderer/renderer_scheduler_impl.cc:773: state->SetString("current_use_case", Do we want to trace the policy too? One way would be to add a AsValue() method to Policy and call that here. https://codereview.chromium.org/1320633002/diff/20001/components/scheduler/re... File components/scheduler/renderer/renderer_scheduler_impl.h (right): https://codereview.chromium.org/1320633002/diff/20001/components/scheduler/re... components/scheduler/renderer/renderer_scheduler_impl.h:70: NOT_SCROLLING, I feel like scrolling as a term is too narrow since there are also things like pinch-to-zoom that fall under this. How about COMPOSITOR_INTERACTION vs. MAIN_THREAD_INTERACTION or something? Also, the base case could just be IDLE? https://codereview.chromium.org/1320633002/diff/20001/components/scheduler/re... components/scheduler/renderer/renderer_scheduler_impl.h:72: MAINTHREAD_SCROLLING, nit: MAIN_THREAD_SCROLLING https://codereview.chromium.org/1320633002/diff/20001/components/scheduler/re... components/scheduler/renderer/renderer_scheduler_impl.h:83: TaskQueue::QueuePriority compositor_queue_priority_; Public struct members shouldn't have trailing underscores (yep, we've got other places where we're doing this wrong atm.) https://codereview.chromium.org/1320633002/diff/20001/components/scheduler/re... components/scheduler/renderer/renderer_scheduler_impl.h:193: base::TimeDelta* new_policy_duration) const; Should this now be the policy duration or the use case duration? https://codereview.chromium.org/1320633002/diff/20001/components/scheduler/re... components/scheduler/renderer/renderer_scheduler_impl.h:197: bool InputSignalsSuggestScrolling(base::TimeTicks now) const; Suggest main thread scrolling? https://codereview.chromium.org/1320633002/diff/20001/components/scheduler/re... components/scheduler/renderer/renderer_scheduler_impl.h:209: bool ScrollExpectedSoon(UseCase use_case, Could you split all the logic about predicting the user into a new UserModel class? I suspect it's going to become a lot more complicated and it's fairly independent anyway.
PTAL https://codereview.chromium.org/1320633002/diff/20001/cc/trees/thread_proxy.cc File cc/trees/thread_proxy.cc (right): https://codereview.chromium.org/1320633002/diff/20001/cc/trees/thread_proxy.c... cc/trees/thread_proxy.cc:1183: // Put the scheduler in impl latency prioritization mode if there's no new On 2015/08/26 13:38:01, Sami wrote: > nit: the comment isn't entirely accurate (we might still have new content but > we're just not prioritizing it). > > Could you split this change into its own patch since I think it'd be useful by > itself and might trigger interesting things on the perf bots? Mainly I'm > wondering what this will do to the scroll update metrics and how much we should > care about that. > > One thing we could try would be to ship both bits (BeginMainFrame is critical, a > scroll handler is being targeted) separately and then prioritizing scrolling if > it looks like it can do the job fast enough. WDYT? As discussed offline I'm going to do this in a separate patch. https://codereview.chromium.org/1320633002/diff/20001/components/scheduler/re... File components/scheduler/renderer/renderer_scheduler_impl.cc (left): https://codereview.chromium.org/1320633002/diff/20001/components/scheduler/re... components/scheduler/renderer/renderer_scheduler_impl.cc:543: // TODO(skyostil): Consider removing in_idle_period_ and On 2015/08/26 13:38:01, Sami wrote: > Wanna keep this todo since the code is still here? Done. https://codereview.chromium.org/1320633002/diff/20001/components/scheduler/re... File components/scheduler/renderer/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/1320633002/diff/20001/components/scheduler/re... components/scheduler/renderer/renderer_scheduler_impl.cc:21: const double kTimerTaskEstimationPercentile = 90; On 2015/08/26 13:38:01, Sami wrote: > Any reason for tweaking these in this way? Yes, I was hoping for a more pessimistic result that reacted a little sooner. I think there's plenty of scope for tweaking these. https://codereview.chromium.org/1320633002/diff/20001/components/scheduler/re... components/scheduler/renderer/renderer_scheduler_impl.cc:71: base::TimeDelta::FromMilliseconds(16)); On 2015/08/26 13:38:01, Sami wrote: > BeginFrameArgs::DefaultInterval()? Done. https://codereview.chromium.org/1320633002/diff/20001/components/scheduler/re... components/scheduler/renderer/renderer_scheduler_impl.cc:308: CASE_TYPE(Undefined); On 2015/08/26 13:38:01, Sami wrote: > Instead of duplicating this from RenderWidget, could we move the list there to a > common place? Acknowledged. https://codereview.chromium.org/1320633002/diff/20001/components/scheduler/re... components/scheduler/renderer/renderer_scheduler_impl.cc:445: case UseCase::NOT_SCROLLING: On 2015/08/26 13:38:01, Sami wrote: > Could we not be expecting a scroll in this state? Yeah we could. Maybe we should yeild in that situation. Likewise this applies to MAINTHREAD_SCROLLING too. https://codereview.chromium.org/1320633002/diff/20001/components/scheduler/re... components/scheduler/renderer/renderer_scheduler_impl.cc:522: if (AnyThread().last_touchstart_time_.is_null() || On 2015/08/26 13:38:01, Sami wrote: > Touchstart doesn't get sent if there are no touch handlers, so I think we need > to track the scroll gesture instead. Maybe this function is badly named then (see comment below). I'm under the impression that without Touch Start being sent, the main thread is largely irrelevant to the scroll. If so I think I should rename this to TouchstartExpectedSoon, and it should return false if there is no touch handler. That suggests we need to wire up that signal we talked about from blink to say there is a touch handler. WDYT? https://codereview.chromium.org/1320633002/diff/20001/components/scheduler/re... components/scheduler/renderer/renderer_scheduler_impl.cc:531: return false; On 2015/08/26 13:38:01, Sami wrote: > Should we always return true for main thread scrolling? Good question. I think that depends on what we want to do with this signal. My current thinking is, this is trying to predict when a TouchStart is likely and as such we should treat all scrolls the same. WDYT? https://codereview.chromium.org/1320633002/diff/20001/components/scheduler/re... components/scheduler/renderer/renderer_scheduler_impl.cc:587: new_policy.timer_queue_priority_ = TaskQueue::DISABLED_PRIORITY; On 2015/08/26 13:38:01, Sami wrote: > block_expensive_tasks = true? We could do that, but it would be a nop. Do you still want this? https://codereview.chromium.org/1320633002/diff/20001/components/scheduler/re... components/scheduler/renderer/renderer_scheduler_impl.cc:599: new_policy.timer_queue_priority_ = TaskQueue::HIGH_PRIORITY; On 2015/08/26 13:38:01, Sami wrote: > Why are timers now high priority? They used to be best effort. They don't need to be :) https://codereview.chromium.org/1320633002/diff/20001/components/scheduler/re... components/scheduler/renderer/renderer_scheduler_impl.cc:627: use_case == MainThreadOnly().current_use_case_ && On 2015/08/26 13:38:01, Sami wrote: > Do we need to check if the use case changed? Could we just always save the > computed use case and only look at the policy after that? Yeah I think we could do that. https://codereview.chromium.org/1320633002/diff/20001/components/scheduler/re... components/scheduler/renderer/renderer_scheduler_impl.cc:666: // No., so arrange for compositor tasks to be run at the highest priority. On 2015/08/26 13:38:01, Sami wrote: > typo: . Done. https://codereview.chromium.org/1320633002/diff/20001/components/scheduler/re... components/scheduler/renderer/renderer_scheduler_impl.cc:773: state->SetString("current_use_case", On 2015/08/26 13:38:01, Sami wrote: > Do we want to trace the policy too? One way would be to add a AsValue() method > to Policy and call that here. We could do that, mind you it's already logged in the TQM trace. WDYT? https://codereview.chromium.org/1320633002/diff/20001/components/scheduler/re... File components/scheduler/renderer/renderer_scheduler_impl.h (right): https://codereview.chromium.org/1320633002/diff/20001/components/scheduler/re... components/scheduler/renderer/renderer_scheduler_impl.h:70: NOT_SCROLLING, On 2015/08/26 13:38:02, Sami wrote: > I feel like scrolling as a term is too narrow since there are also things like > pinch-to-zoom that fall under this. How about COMPOSITOR_INTERACTION vs. > MAIN_THREAD_INTERACTION or something? Also, the base case could just be IDLE? As discussed offline I went with COMPOSITOR_GESTURE & MAIN_THREAD_GESTURE https://codereview.chromium.org/1320633002/diff/20001/components/scheduler/re... components/scheduler/renderer/renderer_scheduler_impl.h:72: MAINTHREAD_SCROLLING, On 2015/08/26 13:38:02, Sami wrote: > nit: MAIN_THREAD_SCROLLING Acknowledged. https://codereview.chromium.org/1320633002/diff/20001/components/scheduler/re... components/scheduler/renderer/renderer_scheduler_impl.h:83: TaskQueue::QueuePriority compositor_queue_priority_; On 2015/08/26 13:38:02, Sami wrote: > Public struct members shouldn't have trailing underscores (yep, we've got other > places where we're doing this wrong atm.) OK lets fix em. Or at least the ones in this class. https://codereview.chromium.org/1320633002/diff/20001/components/scheduler/re... components/scheduler/renderer/renderer_scheduler_impl.h:193: base::TimeDelta* new_policy_duration) const; On 2015/08/26 13:38:02, Sami wrote: > Should this now be the policy duration or the use case duration? How about expected_use_case_duration? https://codereview.chromium.org/1320633002/diff/20001/components/scheduler/re... components/scheduler/renderer/renderer_scheduler_impl.h:197: bool InputSignalsSuggestScrolling(base::TimeTicks now) const; This is used to avoid posting lots of policy updates from the compositor thread. As discussed offline I think InputSignalsSuggestGestureInProgress is a better name. https://codereview.chromium.org/1320633002/diff/20001/components/scheduler/re... components/scheduler/renderer/renderer_scheduler_impl.h:209: bool ScrollExpectedSoon(UseCase use_case, On 2015/08/26 13:38:02, Sami wrote: > Could you split all the logic about predicting the user into a new UserModel > class? I suspect it's going to become a lot more complicated and it's fairly > independent anyway. Done.
The CQ bit was checked by alexclarke@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1320633002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1320633002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
https://codereview.chromium.org/1320633002/diff/20001/components/scheduler/re... File components/scheduler/renderer/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/1320633002/diff/20001/components/scheduler/re... components/scheduler/renderer/renderer_scheduler_impl.cc:587: new_policy.timer_queue_priority_ = TaskQueue::DISABLED_PRIORITY; On 2015/08/27 12:02:51, alexclarke1 wrote: > On 2015/08/26 13:38:01, Sami wrote: > > block_expensive_tasks = true? > > We could do that, but it would be a nop. Do you still want this? Yeah, I was thinking it would be better to have it here for consistency and safety if we adjust the logic later. https://codereview.chromium.org/1320633002/diff/20001/components/scheduler/re... components/scheduler/renderer/renderer_scheduler_impl.cc:773: state->SetString("current_use_case", On 2015/08/27 12:02:51, alexclarke1 wrote: > On 2015/08/26 13:38:01, Sami wrote: > > Do we want to trace the policy too? One way would be to add a AsValue() method > > to Policy and call that here. > > We could do that, mind you it's already logged in the TQM trace. WDYT? Right, let's add it once it actually adds new information. https://codereview.chromium.org/1320633002/diff/40001/components/scheduler/re... File components/scheduler/renderer/renderer_scheduler.cc (right): https://codereview.chromium.org/1320633002/diff/40001/components/scheduler/re... components/scheduler/renderer/renderer_scheduler.cc:45: return "mainthread_gesture"; nit: main_thread_gesture https://codereview.chromium.org/1320633002/diff/40001/components/scheduler/re... File components/scheduler/renderer/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/1320633002/diff/40001/components/scheduler/re... components/scheduler/renderer/renderer_scheduler_impl.cc:546: // Tracing is done before the early out check, because it's quite possible we Should we only do this if the use case or the policy changes? Otherwise I think this can become a little too spammy. https://codereview.chromium.org/1320633002/diff/40001/components/scheduler/re... File components/scheduler/renderer/renderer_scheduler_impl.h (right): https://codereview.chromium.org/1320633002/diff/40001/components/scheduler/re... components/scheduler/renderer/renderer_scheduler_impl.h:165: // Tries to guess if a Touchstart is expected soon. Currently this is nit: the second sentence is guaranteed to get out of date :) Maybe leave it out from here? https://codereview.chromium.org/1320633002/diff/40001/components/scheduler/re... components/scheduler/renderer/renderer_scheduler_impl.h:171: // Helper for computing the policy. |expected_usecase_duration| will be filled s/policy/use case/ https://codereview.chromium.org/1320633002/diff/40001/components/scheduler/re... components/scheduler/renderer/renderer_scheduler_impl.h:175: UseCase ComputeCurrentUseCase( Idle thought for the future: maybe the use case detector could be split into a separate class too. https://codereview.chromium.org/1320633002/diff/40001/components/scheduler/re... File components/scheduler/renderer/user_model.h (right): https://codereview.chromium.org/1320633002/diff/40001/components/scheduler/re... components/scheduler/renderer/user_model.h:27: void DidHandleInputEventOnCompositorThread( This is called on the impl thread but it doesn't look like there's any locking here? Is there a way to isolate the user model from threading somehow? https://codereview.chromium.org/1320633002/diff/40001/components/scheduler/re... components/scheduler/renderer/user_model.h:42: bool TouchStartExpectedSoon(RendererScheduler::UseCase use_case, nit: Maybe IsTouchStartExpectedSoon() to highlight that this is a query and not a statement of fact? https://codereview.chromium.org/1320633002/diff/40001/components/scheduler/re... components/scheduler/renderer/user_model.h:54: // We consider further input invents to be likely if the user has interacted typo: events https://codereview.chromium.org/1320633002/diff/40001/components/scheduler/re... components/scheduler/renderer/user_model.h:60: int pending_main_thread_input_event_count_; It feels like this shouldn't be part of the user model since it's really an artifact of how input events are delivered. Do you think we could pull it out?
Also re: the title, this isn't really a refactor since it's changing the scheduling policy in a pretty big way. Could you either split the actual policy change into a separate patch or rewrite the description to be more about the way the policy is changing?
PTAL. As discussed off line I changed UserModel to have no references to threading. Also DidStartProcessingInputEvent is always called with a matching DidFinishProcessingInputEvent. https://codereview.chromium.org/1320633002/diff/20001/components/scheduler/re... File components/scheduler/renderer/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/1320633002/diff/20001/components/scheduler/re... components/scheduler/renderer/renderer_scheduler_impl.cc:587: new_policy.timer_queue_priority_ = TaskQueue::DISABLED_PRIORITY; On 2015/08/27 15:15:58, Sami wrote: > On 2015/08/27 12:02:51, alexclarke1 wrote: > > On 2015/08/26 13:38:01, Sami wrote: > > > block_expensive_tasks = true? > > > > We could do that, but it would be a nop. Do you still want this? > > Yeah, I was thinking it would be better to have it here for consistency and > safety if we adjust the logic later. Done. https://codereview.chromium.org/1320633002/diff/20001/components/scheduler/re... components/scheduler/renderer/renderer_scheduler_impl.cc:773: state->SetString("current_use_case", On 2015/08/27 15:15:58, Sami wrote: > On 2015/08/27 12:02:51, alexclarke1 wrote: > > On 2015/08/26 13:38:01, Sami wrote: > > > Do we want to trace the policy too? One way would be to add a AsValue() > method > > > to Policy and call that here. > > > > We could do that, mind you it's already logged in the TQM trace. WDYT? > > Right, let's add it once it actually adds new information. Acknowledged. https://codereview.chromium.org/1320633002/diff/40001/components/scheduler/re... File components/scheduler/renderer/renderer_scheduler.cc (right): https://codereview.chromium.org/1320633002/diff/40001/components/scheduler/re... components/scheduler/renderer/renderer_scheduler.cc:45: return "mainthread_gesture"; On 2015/08/27 15:15:59, Sami wrote: > nit: main_thread_gesture Done. https://codereview.chromium.org/1320633002/diff/40001/components/scheduler/re... File components/scheduler/renderer/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/1320633002/diff/40001/components/scheduler/re... components/scheduler/renderer/renderer_scheduler_impl.cc:546: // Tracing is done before the early out check, because it's quite possible we On 2015/08/27 15:15:59, Sami wrote: > Should we only do this if the use case or the policy changes? Otherwise I think > this can become a little too spammy. I actually think it was way too hard to figure out what state the scheduler was in before if you started recording after the policy was set. This does spam more, but it's still several orders of magnitude less than the task queue manager logging. WDYT? https://codereview.chromium.org/1320633002/diff/40001/components/scheduler/re... File components/scheduler/renderer/renderer_scheduler_impl.h (right): https://codereview.chromium.org/1320633002/diff/40001/components/scheduler/re... components/scheduler/renderer/renderer_scheduler_impl.h:165: // Tries to guess if a Touchstart is expected soon. Currently this is On 2015/08/27 15:15:59, Sami wrote: > nit: the second sentence is guaranteed to get out of date :) Maybe leave it out > from here? Done. https://codereview.chromium.org/1320633002/diff/40001/components/scheduler/re... components/scheduler/renderer/renderer_scheduler_impl.h:171: // Helper for computing the policy. |expected_usecase_duration| will be filled On 2015/08/27 15:15:59, Sami wrote: > s/policy/use case/ Done. https://codereview.chromium.org/1320633002/diff/40001/components/scheduler/re... components/scheduler/renderer/renderer_scheduler_impl.h:175: UseCase ComputeCurrentUseCase( On 2015/08/27 15:15:59, Sami wrote: > Idle thought for the future: maybe the use case detector could be split into a > separate class too. Acknowledged. https://codereview.chromium.org/1320633002/diff/40001/components/scheduler/re... File components/scheduler/renderer/user_model.h (right): https://codereview.chromium.org/1320633002/diff/40001/components/scheduler/re... components/scheduler/renderer/user_model.h:27: void DidHandleInputEventOnCompositorThread( On 2015/08/27 15:15:59, Sami wrote: > This is called on the impl thread but it doesn't look like there's any locking > here? Is there a way to isolate the user model from threading somehow? This class isn't by itself thread safe. However it's protected by the AnyThread() lock inside of RendererSchedulerImpl. I think that's fine, but perhaps it's confusing to mention threads here? WDYT https://codereview.chromium.org/1320633002/diff/40001/components/scheduler/re... components/scheduler/renderer/user_model.h:42: bool TouchStartExpectedSoon(RendererScheduler::UseCase use_case, On 2015/08/27 15:15:59, Sami wrote: > nit: Maybe IsTouchStartExpectedSoon() to highlight that this is a query and not > a statement of fact? Done. https://codereview.chromium.org/1320633002/diff/40001/components/scheduler/re... components/scheduler/renderer/user_model.h:54: // We consider further input invents to be likely if the user has interacted On 2015/08/27 15:15:59, Sami wrote: > typo: events Done.
The CQ bit was checked by alexclarke@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1320633002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1320633002/120001
The CQ bit was checked by alexclarke@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1320633002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1320633002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
Looks great overall. Just a few suggestions for the UserModel class. https://codereview.chromium.org/1320633002/diff/140001/components/scheduler/r... File components/scheduler/renderer/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/1320633002/diff/140001/components/scheduler/r... components/scheduler/renderer/renderer_scheduler_impl.cc:384: // The touchstart and main-thread scrolling states indicate a strong nit: s/scrolling/gesture/ https://codereview.chromium.org/1320633002/diff/140001/components/scheduler/r... components/scheduler/renderer/renderer_scheduler_impl.cc:474: // TouchStartExpectedSoon if there is at least one. By the way, this is sort of already happening as a happy accident of sorts: TouchStart is only sent if the page has a registered touch handler. However because we only know that after the first TouchStart we see, the prediction for the first gesture could be wrong. An explicit signal would therefore be better. https://codereview.chromium.org/1320633002/diff/140001/components/scheduler/r... components/scheduler/renderer/renderer_scheduler_impl.cc:475: base::TimeDelta scroll_expected_flag_valid_for_duration; nit: touchstart_expected_flag_valid_for_duration? https://codereview.chromium.org/1320633002/diff/140001/components/scheduler/r... File components/scheduler/renderer/user_model.h (right): https://codereview.chromium.org/1320633002/diff/140001/components/scheduler/r... components/scheduler/renderer/user_model.h:32: base::TimeDelta TimeLeftInInputEscalatedPolicy(base::TimeTicks now) const; It feels like the user model shouldn't know anything about priorities or policies. How about calling this TimeLeftInUserGesture, or ExpectedUserGestureDuration? https://codereview.chromium.org/1320633002/diff/140001/components/scheduler/r... components/scheduler/renderer/user_model.h:37: bool IsTouchStartExpectedSoon( Similarly the reason why TouchStart is special is a little outside this class's jurisdiction. Maybe this should just be IsUserGestureExpectedSoon() and in addition to touchstart we track GestureScrollBegin and GesturePinchBegin (i.e., all the continuous gestures). |last_touchstart_time_| could then be |last_gesture_start_time_|. WDYT?
https://codereview.chromium.org/1320633002/diff/140001/components/scheduler/r... File components/scheduler/renderer/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/1320633002/diff/140001/components/scheduler/r... components/scheduler/renderer/renderer_scheduler_impl.cc:384: // The touchstart and main-thread scrolling states indicate a strong On 2015/09/04 10:58:21, Sami wrote: > nit: s/scrolling/gesture/ Done. https://codereview.chromium.org/1320633002/diff/140001/components/scheduler/r... components/scheduler/renderer/renderer_scheduler_impl.cc:474: // TouchStartExpectedSoon if there is at least one. On 2015/09/04 10:58:21, Sami wrote: > By the way, this is sort of already happening as a happy accident of sorts: > TouchStart is only sent if the page has a registered touch handler. However > because we only know that after the first TouchStart we see, the prediction for > the first gesture could be wrong. An explicit signal would therefore be better. Interesting point. I've updated the comment. https://codereview.chromium.org/1320633002/diff/140001/components/scheduler/r... components/scheduler/renderer/renderer_scheduler_impl.cc:475: base::TimeDelta scroll_expected_flag_valid_for_duration; On 2015/09/04 10:58:21, Sami wrote: > nit: touchstart_expected_flag_valid_for_duration? Done. https://codereview.chromium.org/1320633002/diff/140001/components/scheduler/r... File components/scheduler/renderer/user_model.h (right): https://codereview.chromium.org/1320633002/diff/140001/components/scheduler/r... components/scheduler/renderer/user_model.h:32: base::TimeDelta TimeLeftInInputEscalatedPolicy(base::TimeTicks now) const; On 2015/09/04 10:58:21, Sami wrote: > It feels like the user model shouldn't know anything about priorities or > policies. How about calling this TimeLeftInUserGesture, or > ExpectedUserGestureDuration? Done. https://codereview.chromium.org/1320633002/diff/140001/components/scheduler/r... components/scheduler/renderer/user_model.h:37: bool IsTouchStartExpectedSoon( On 2015/09/04 10:58:21, Sami wrote: > Similarly the reason why TouchStart is special is a little outside this class's > jurisdiction. Maybe this should just be IsUserGestureExpectedSoon() and in > addition to touchstart we track GestureScrollBegin and GesturePinchBegin (i.e., > all the continuous gestures). |last_touchstart_time_| could then be > |last_gesture_start_time_|. WDYT? Done.
Thanks, lgtm! https://codereview.chromium.org/1320633002/diff/160001/components/scheduler/r... File components/scheduler/renderer/renderer_scheduler_impl.h (right): https://codereview.chromium.org/1320633002/diff/160001/components/scheduler/r... components/scheduler/renderer/renderer_scheduler_impl.h:171: base::TimeDelta TimeLeftInUserGesture(base::TimeTicks now) const; This method and the one below are no longer in this class, right?
The CQ bit was checked by alexclarke@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from skyostil@chromium.org Link to the patchset: https://codereview.chromium.org/1320633002/#ps180001 (title: "No longer prioritizing loading tasks by default.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1320633002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1320633002/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by alexclarke@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from skyostil@chromium.org Link to the patchset: https://codereview.chromium.org/1320633002/#ps200001 (title: "Fix a test")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1320633002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1320633002/200001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)
The CQ bit was checked by alexclarke@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1320633002/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1320633002/220001
PTAL https://codereview.chromium.org/1320633002/diff/160001/components/scheduler/r... File components/scheduler/renderer/renderer_scheduler_impl.h (right): https://codereview.chromium.org/1320633002/diff/160001/components/scheduler/r... components/scheduler/renderer/renderer_scheduler_impl.h:171: base::TimeDelta TimeLeftInUserGesture(base::TimeTicks now) const; On 2015/09/07 11:37:23, Sami wrote: > This method and the one below are no longer in this class, right? Done.
Let's split the NONE prioritization tweak to a different patch like we talked about. https://codereview.chromium.org/1320633002/diff/220001/components/scheduler/r... File components/scheduler/renderer/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/1320633002/diff/220001/components/scheduler/r... components/scheduler/renderer/renderer_scheduler_impl.cc:553: if (touchstart_expected_soon) nit: please add braces for both branches https://codereview.chromium.org/1320633002/diff/220001/components/scheduler/r... components/scheduler/renderer/renderer_scheduler_impl.cc:574: if (touchstart_expected_soon) ditto.
Thanks, lgtm. https://codereview.chromium.org/1320633002/diff/220001/components/scheduler/r... File components/scheduler/renderer/renderer_scheduler_impl_unittest.cc (right): https://codereview.chromium.org/1320633002/diff/220001/components/scheduler/r... components/scheduler/renderer/renderer_scheduler_impl_unittest.cc:1009: testing::ElementsAre(std::string("D1"), std::string("D2"), This should change back now that we're no longer prioritizing default tasks, right? https://codereview.chromium.org/1320633002/diff/240001/components/scheduler/r... File components/scheduler/renderer/renderer_scheduler_impl_unittest.cc (right): https://codereview.chromium.org/1320633002/diff/240001/components/scheduler/r... components/scheduler/renderer/renderer_scheduler_impl_unittest.cc:874: // Loading tasks are actually still priprized by default. This is no longer true, right?
The CQ bit was checked by alexclarke@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from skyostil@chromium.org Link to the patchset: https://codereview.chromium.org/1320633002/#ps250001 (title: "Fix bug with default TQ not being checked when comparing policies")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1320633002/250001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1320633002/250001
https://codereview.chromium.org/1320633002/diff/220001/components/scheduler/r... File components/scheduler/renderer/renderer_scheduler_impl_unittest.cc (right): https://codereview.chromium.org/1320633002/diff/220001/components/scheduler/r... components/scheduler/renderer/renderer_scheduler_impl_unittest.cc:1009: testing::ElementsAre(std::string("D1"), std::string("D2"), On 2015/09/07 16:57:57, Sami wrote: > This should change back now that we're no longer prioritizing default tasks, > right? It did. I think you're looking at the wrong patch set. https://codereview.chromium.org/1320633002/diff/240001/components/scheduler/r... File components/scheduler/renderer/renderer_scheduler_impl_unittest.cc (right): https://codereview.chromium.org/1320633002/diff/240001/components/scheduler/r... components/scheduler/renderer/renderer_scheduler_impl_unittest.cc:874: // Loading tasks are actually still priprized by default. On 2015/09/07 16:57:57, Sami wrote: > This is no longer true, right? Yup. And this found a bug, I needed to check the state of the default TQ when checking if the state has changed.
The CQ bit was unchecked by alexclarke@chromium.org
The CQ bit was checked by alexclarke@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from skyostil@chromium.org Link to the patchset: https://codereview.chromium.org/1320633002/#ps270001 (title: "Deprioritize compositor tasks during the first part of UseCase::COMPOSITOR_GESTURE")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1320633002/270001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1320633002/270001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by alexclarke@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1320633002/290001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1320633002/290001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Sami please take one (hopefully) final look! This now passes the CQ.
Sami please take one (hopefully) final look! This now passes the CQ.
alexclarke@chromium.org changed reviewers: + jochen@chromium.org
jochen@chromium.org: Please review changes in content/renderer/render_frame_impl.cc
lgtm
Thanks for sorting out the failures, lgtm to land. I'm a tiny bit worried about what compositor best-effortness will do to main thread CSS animations during scroll, but we can always tweak that later.
The CQ bit was checked by alexclarke@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org Link to the patchset: https://codereview.chromium.org/1320633002/#ps310001 (title: "Remove code to surpress blocking of expensive tasks in the 1st sec, the reset should suffice.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1320633002/310001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1320633002/310001
The diff base for renderer_scheduler_impl.cc is slightly weird due to a non-trivial rebase. This wil be fixed soon once the UseCase refactor is submitted.
On 2015/09/08 16:31:42, alexclarke1 wrote: > The diff base for renderer_scheduler_impl.cc is slightly weird due to a > non-trivial rebase. This wil be fixed soon once the UseCase refactor is > submitted. Oops please ignore comment. Typed into wrong window ;)
On 2015/09/08 16:31:42, alexclarke1 wrote: > The diff base for renderer_scheduler_impl.cc is slightly weird due to a > non-trivial rebase. This wil be fixed soon once the UseCase refactor is > submitted. Oops please ignore comment. Typed into wrong window ;)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_blink_rel on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/7...)
The CQ bit was checked by alexclarke@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1320633002/330001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1320633002/330001
The CQ bit was checked by alexclarke@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1320633002/410001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1320633002/410001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
The CQ bit was checked by alexclarke@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1320633002/410001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1320633002/410001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by alexclarke@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org, skyostil@chromium.org Link to the patchset: https://codereview.chromium.org/1320633002/#ps410001 (title: "Try set OnPageLoadStarted in RenderFrameImpl::OnNavigate")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1320633002/410001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1320633002/410001
Message was sent while issue was closed.
Committed patchset #22 (id:410001)
Message was sent while issue was closed.
Patchset 22 (id:??) landed as https://crrev.com/464b1a5144ddb5135dad845aceb5b1f074221387 Cr-Commit-Position: refs/heads/master@{#347937}
Message was sent while issue was closed.
A revert of this CL (patchset #22 id:410001) has been created in https://codereview.chromium.org/1337993002/ by jam@chromium.org. The reason for reverting is: adds flaky test, causes other test to be flaky BUG=530250. |
