Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2258133002:
[scheduler] Implement time-based cpu throttling. (Closed)
Description[scheduler] Implement time-based cpu throttling.
This patch adds TimeBudgetPool API to renderer scheduler, which allows to group task queues into pools and set shared limits on how long this pool of queues is allowed to run (as per cent of wall time). Queues violating this limit will be throttled.
This patch also renames throttling_helper.cc to task_queue_throttler.cc.
BUG=639852
Committed: https://crrev.com/906d4a1e806b27681bad0ec63f64c0c9fc239a77
Cr-Commit-Position: refs/heads/master@{#420842}
Patch Set 1 #Patch Set 2 : Tests #Patch Set 3 : Removed "Locked" from MaybeSchedulePumpThrottledTasksLocked #Patch Set 4 : Changed similarity #
Total comments: 35
Patch Set 5 : Addressed comments #Patch Set 6 : Use double instead of base::TimeTicks #
Total comments: 74
Patch Set 7 : Addressed comments #Patch Set 8 : Formatted #
Total comments: 10
Patch Set 9 : Addressed comments #Patch Set 10 : Renamed Enable/Disable to Enable/DisableThrottling. #
Total comments: 56
Patch Set 11 : Rebased & addressed comments #
Total comments: 4
Patch Set 12 : More fixes #
Total comments: 6
Patch Set 13 : Addressed comments #
Total comments: 4
Patch Set 14 : One more fix #
Total comments: 33
Patch Set 15 : Fixed compilation #Patch Set 16 : git cl try #Patch Set 17 : Fix trybot failures (hopefully) #Patch Set 18 : Rebased #Messages
Total messages: 73 (32 generated)
Description was changed from ========== Prototype: cpu-based background task throttling BUG= ========== to ========== [scheduler] Implement time-based cpu throttling. Add TimeBudgetPool API to TaskQueueThrottler. BUG=639852 ==========
altimin@chromium.org changed reviewers: + alexclarke@chromium.org, skyostil@chromium.org
https://codereview.chromium.org/2258133002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.cc (right): https://codereview.chromium.org/2258133002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.cc:543: if (wakeup) { I don't think we should allow |wakeup| to be null. BTW main_thread_only().delayed_incoming_queue.begin() is O(1). https://codereview.chromium.org/2258133002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/child/scheduler_helper.h (right): https://codereview.chromium.org/2258133002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/child/scheduler_helper.h:67: inline void CheckOnValidThread() const { Out of curiosity why was this needed? Did it actually make any difference in practice?
Overall looks great! My main suggestion is to improve the debuggability a bit. I think we should also get started on the intent-to-implement & ship for this. https://codereview.chromium.org/2258133002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.h (right): https://codereview.chromium.org/2258133002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.h:40: base::TimeTicks startTime, Unfortunately we can't use TimeTicks in Blink yet. We might want to propose allowing them, but for now let's stick to doubles. (We could consider splitting this into two interfaces since we don't really care about the task queue at this level either.) https://codereview.chromium.org/2258133002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc (left): https://codereview.chromium.org/2258133002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:159: time_domain_->ClearExpiredWakeups(); Do we need to remove this? https://codereview.chromium.org/2258133002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:162: // Maybe schedule a call to ThrottlingHelper::PumpThrottledTasks if there is Is this comment still valid? https://codereview.chromium.org/2258133002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc (right): https://codereview.chromium.org/2258133002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:22: const base::TimeDelta kMaxTimeBudget = base::TimeDelta::FromSeconds(1); No static global non-pod constructors please. https://codereview.chromium.org/2258133002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:42: task_queue_throttler_->time_budget_pool_for_queue_[queue] = this; Do we need to disable the queue if the budget has run out? https://codereview.chromium.org/2258133002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:47: task_queue_throttler_->time_budget_pool_for_queue_.erase(queue); Do we need to re-enable the queue? https://codereview.chromium.org/2258133002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:236: continue; Should we add a trace event here so we can see when things get throttled? For bonus points please also add a call from RendererSchedulerImpl::AsValue() to here so we can see the remaining budgets of each queue. Basically I want make it as easy as possible to debug any mysterious failures that come our way once this lands. https://codereview.chromium.org/2258133002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:242: base::TimeTicks wakeup; nit: wake_up https://codereview.chromium.org/2258133002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:323: if (time_budget_pool) { Can this ever be null? https://codereview.chromium.org/2258133002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:330: base::TimeTicks now = tick_clock_->NowTicks(); Could we just use end_time for this? If not, move this inside the 'if'? https://codereview.chromium.org/2258133002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h (right): https://codereview.chromium.org/2258133002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h:35: // Of course the TaskQueueThrottler isn't the only sub-system that wants to Could you re-wrap the text here so it doesn't look awkward? https://codereview.chromium.org/2258133002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h:56: void SetTimeBudget(double cpu_percentange); typo: percentage Please also mention what this does to the budget. Also, what's the default budget? It might be nice if we could change the budget without resetting it, e.g., by reducing the budget based on the time a tab has been in the background. https://codereview.chromium.org/2258133002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h:81: }; DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/2258133002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h:126: TimeBudgetPool* CreateTimeBudgetPool(); Who owns this? (unique_ptr if its the caller) https://codereview.chromium.org/2258133002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/web_view_scheduler_impl.h (right): https://codereview.chromium.org/2258133002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/web_view_scheduler_impl.h:62: friend class WebFrameSchedulerImpl; Why's this needed? https://codereview.chromium.org/2258133002/diff/60001/third_party/WebKit/publ... File third_party/WebKit/public/platform/scheduler/base/task_queue.h (right): https://codereview.chromium.org/2258133002/diff/60001/third_party/WebKit/publ... third_party/WebKit/public/platform/scheduler/base/task_queue.h:154: virtual bool NextScheduledWakeUp(base::TimeTicks* wakeup) = 0; nit: GetNext... to match the naming around here. nit: wake_up
PTAL. (WebPerfAgentInspector thing is still TBD). https://codereview.chromium.org/2258133002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.cc (right): https://codereview.chromium.org/2258133002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.cc:543: if (wakeup) { On 2016/09/07 15:13:07, alex clarke wrote: > I don't think we should allow |wakeup| to be null. BTW > main_thread_only().delayed_incoming_queue.begin() is O(1). Done. https://codereview.chromium.org/2258133002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/child/scheduler_helper.h (right): https://codereview.chromium.org/2258133002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/child/scheduler_helper.h:67: inline void CheckOnValidThread() const { On 2016/09/07 15:13:07, alex clarke wrote: > Out of curiosity why was this needed? Did it actually make any difference in > practice? I was thinking about calling CheckOnValidThread in TaskQueueThrottler, but I've abandoned the idea (resulted in more complex and less readable code). inline was added to avoid function call with empty contents in release mode (most of compilers were doing this anyway, but I wanted to be sure). https://codereview.chromium.org/2258133002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc (left): https://codereview.chromium.org/2258133002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:159: time_domain_->ClearExpiredWakeups(); On 2016/09/07 15:20:43, Sami wrote: > Do we need to remove this? Yes, now we don't rely on TimeDomain::NextScheduledRunTime — we get more fine-grained results from each queue. https://codereview.chromium.org/2258133002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:162: // Maybe schedule a call to ThrottlingHelper::PumpThrottledTasks if there is On 2016/09/07 15:20:43, Sami wrote: > Is this comment still valid? Done. https://codereview.chromium.org/2258133002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc (right): https://codereview.chromium.org/2258133002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:22: const base::TimeDelta kMaxTimeBudget = base::TimeDelta::FromSeconds(1); On 2016/09/07 15:20:43, Sami wrote: > No static global non-pod constructors please. Done. https://codereview.chromium.org/2258133002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:42: task_queue_throttler_->time_budget_pool_for_queue_[queue] = this; On 2016/09/07 15:20:43, Sami wrote: > Do we need to disable the queue if the budget has run out? Done. https://codereview.chromium.org/2258133002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:47: task_queue_throttler_->time_budget_pool_for_queue_.erase(queue); On 2016/09/07 15:20:43, Sami wrote: > Do we need to re-enable the queue? Done. https://codereview.chromium.org/2258133002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:236: continue; On 2016/09/07 15:20:43, Sami wrote: > Should we add a trace event here so we can see when things get throttled? > > For bonus points please also add a call from RendererSchedulerImpl::AsValue() to > here so we can see the remaining budgets of each queue. Basically I want make it > as easy as possible to debug any mysterious failures that come our way once this > lands. Done. https://codereview.chromium.org/2258133002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:242: base::TimeTicks wakeup; On 2016/09/07 15:20:43, Sami wrote: > nit: wake_up Done. https://codereview.chromium.org/2258133002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:323: if (time_budget_pool) { On 2016/09/07 15:20:43, Sami wrote: > Can this ever be null? Done. https://codereview.chromium.org/2258133002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:330: base::TimeTicks now = tick_clock_->NowTicks(); On 2016/09/07 15:20:43, Sami wrote: > Could we just use end_time for this? If not, move this inside the 'if'? Done. https://codereview.chromium.org/2258133002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h (right): https://codereview.chromium.org/2258133002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h:35: // Of course the TaskQueueThrottler isn't the only sub-system that wants to On 2016/09/07 15:20:44, Sami wrote: > Could you re-wrap the text here so it doesn't look awkward? Done. https://codereview.chromium.org/2258133002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h:56: void SetTimeBudget(double cpu_percentange); On 2016/09/07 15:20:44, Sami wrote: > typo: percentage > > Please also mention what this does to the budget. Also, what's the default > budget? > > It might be nice if we could change the budget without resetting it, e.g., by > reducing the budget based on the time a tab has been in the background. Done. https://codereview.chromium.org/2258133002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h:81: }; On 2016/09/07 15:20:43, Sami wrote: > DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/2258133002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h:126: TimeBudgetPool* CreateTimeBudgetPool(); On 2016/09/07 15:20:44, Sami wrote: > Who owns this? (unique_ptr if its the caller) Done. https://codereview.chromium.org/2258133002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/web_view_scheduler_impl.h (right): https://codereview.chromium.org/2258133002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/web_view_scheduler_impl.h:62: friend class WebFrameSchedulerImpl; On 2016/09/07 15:20:44, Sami wrote: > Why's this needed? Thanks for spotting, left from prototyping phase. We'll need it when we actually start throttling. https://codereview.chromium.org/2258133002/diff/60001/third_party/WebKit/publ... File third_party/WebKit/public/platform/scheduler/base/task_queue.h (right): https://codereview.chromium.org/2258133002/diff/60001/third_party/WebKit/publ... third_party/WebKit/public/platform/scheduler/base/task_queue.h:154: virtual bool NextScheduledWakeUp(base::TimeTicks* wakeup) = 0; On 2016/09/07 15:20:44, Sami wrote: > nit: GetNext... to match the naming around here. > > nit: wake_up Done.
Overall I like what you're trying to do but actually reviewing this code is proving to be very hard. The problem is I can't see how TimeBudgetPool is going to be used in situ. I know we normally ask people to cut the size of patches down but I'd like you to add the code that uses it. It's quote possible half my comments on the TimeBudgetPool are ill informed as a result of this lack of context. https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.h (right): https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.h:153: base::Optional<base::TimeTicks> GetNextTaskRunTime() override; I'm fine with adding GetNextScheduledWakeUp but I'm not keen on GetNextTaskRunTime being part of the TaskQueue API for these reasons: 1. It's too similar to GetNextScheduledWakeUp. How would you know which to call? 2. You can simulate it with HasPendingImmediateWork() plus GetNextScheduledWakeUp() I'd suggest moving the logic into a helper inside TaskQueueThrottler. https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/base/task_time_tracker.h (right): https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/task_time_tracker.h:15: class TaskTimeTracker { Is this used? https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:1482: base::TimeTicks end_time_ticks = MonotonicTimeInSecondsToTimeTicks(end_time); :( I'm not keen on these conversions. I may get rid of them if you don't get there first ;) Please add a TODO(alexclarke): Remove conversions https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.h (right): https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.h:156: void CreateTraceEventObjectSnapshot() const; Please add a comment explaining what this does. https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc (right): https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:61: queue->SetQueueEnabled(false); Is this safe? See TaskQueueThrottler::SetQueueEnable. The RendererScheduler may override this. https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:80: void TaskQueueThrottler::TimeBudgetPool::Enable(base::TimeTicks now) { Lazy now? https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:89: queue->SetQueueEnabled(false); Ditto. https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:96: void TaskQueueThrottler::TimeBudgetPool::Disable(base::TimeTicks now) { Lazy now? https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:96: void TaskQueueThrottler::TimeBudgetPool::Disable(base::TimeTicks now) { I can't see how this is going to be used but I notice TimeBudgetPool::Enable disables the queue but this doesn't. Do we run the risk of the queue not getting re-enabled if TimeBudgetPool::Enable and TimeBudgetPool::Disable are called in quick succession? https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:370: // GetNextScheduledWakeUp() moves tasks from incoming queue to delayed 1. I'm not sure that's right, InsertFence should not change the result of any operation I'm aware of to get the next wake up time. 2. The formatting of this comment looks funny, please fix. https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:452: if (throttled_queues_.find(task_queue) == throttled_queues_.end()) This is going to get called a lot, currently TaskQueueMap is using TaskQueueMap = std::map<TaskQueue*, Metadata>; I wonder if we should change that to: using TaskQueueMap = std::unordered_map<TaskQueue*, Metadata>; https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:491: TaskQueueThrottler::GetTimeBudgetPoolForQueue(TaskQueue* queue) { If you move TimeBudgetPool* into the Metadata struct, I think this method can be removed. https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h (right): https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h:48: // This class is main-thread only. The TaskQueueThrottler (nee-ThrottlingHelper) is main thread only so consider moving this comment to the top level. If we're worried some of the calls may come from the wrong thread, consider adding a debug-only checker. https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h:49: class TimeBudgetPool { Please add an overview comment explaining what the job of TimeBudgetPool is. https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h:49: class TimeBudgetPool { Do we even want this API to be public? https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h:58: void AddQueue(base::TimeTicks now, TaskQueue* queue); Please document these. Side effects such |queue| getting disabled are a bit surprising to me given the name. Note without seeing the code that uses this I can't tell if this is a problem with the name or the functionality yet. https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h:66: void Enable(base::TimeTicks now); Please document Enable & Disable. https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h:73: void Close(); This only seems to be called by test code, is that expected? Makes it hard to understand the lifetime of this object. https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h:149: TimeBudgetPool* CreateTimeBudgetPool(const char* name); This only seems to be called by tests, is that expected? https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h:205: time_budget_pools_; This construct gives me pause for thought. I can't tell what the lifecycle of TimeBudgetPool is outside of tests yet, but this makes me wonder if is should actually be ref counted? https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h:206: std::unordered_map<TaskQueue*, TimeBudgetPool*> time_budget_pool_for_queue_; Can we instead move TimeBudgetPool* into struct Metadata? https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/renderer/web_frame_scheduler_impl.cc (right): https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/web_frame_scheduler_impl.cc:80: nit: remove stray white space here and below. https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/renderer/web_view_scheduler_impl.h (right): https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/web_view_scheduler_impl.h:62: friend class WebFrameSchedulerImpl; Is this still needed? https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/pub... File third_party/WebKit/public/platform/scheduler/base/task_time_observer.h (right): https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/pub... third_party/WebKit/public/platform/scheduler/base/task_time_observer.h:16: class BLINK_PLATFORM_EXPORT TaskTimeObserver { We're trying to add some top level comments to scheduler header files. Could you briefly explain what this class is for. https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/pub... third_party/WebKit/public/platform/scheduler/base/task_time_observer.h:21: virtual void ReportTaskTime(TaskQueue* task_queue, Please add a comment explaining what the members are. I know it's obvious to us, but it may not be to those new to this code.
Thanks for adding the tracing support! https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.h (right): https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.h:39: scheduler::TaskQueue*, Could you make this const? https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/base/task_time_tracker.h (right): https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/task_time_tracker.h:5: #ifndef THIRD_PARTY_WEBKIT_SOURCE_PLATFORM_SCHEDULER_BASE_TASK_TRACKER_H_ TASK_TIME_TRACKER https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/task_time_tracker.h:25: DISSALLOW_COPY_AND_ASSIGN(TaskTimeTracker); -S :) (It doesn't look like this file is being used since the build didn't fail?) https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc (right): https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:158: // Yes, that's memory-infra-approved way of tracing pointers. Deal with it. I think we can do without this comment :) https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:304: // GetNextTaskRunTime requires a lock and OnTimeDomainHasDelayedWork nit: a lock on the queue? https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:371: // queue, nit: please reformat https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:517: base::Optional<base::TimeTicks> next_run_time = budget->NextAllowedRunTime(); I think it's a little weird for this to ask information from the budget -- which is the one calling into this function. Should this method live on the budget class instead? https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h (right): https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h:58: void AddQueue(base::TimeTicks now, TaskQueue* queue); We should probably document the side-effects here (queue gets disabled etc.) https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h:61: void RecordTaskRunTime(base::TimeDelta task_run_time); Does all of this functionality need to be public or could we move the parts that only the throttler needs to be private? Alternatively should we make this class private and just provide the few entrypoints we need in the TaskQueueThrottler public API? https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h:66: void Enable(base::TimeTicks now); What do enable/disable mean for a budget pool? Maybe there's a more descriptive operation name we could use? https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h:100: std::unordered_set<TaskQueue*> assosiated_task_queues_; typo: associated. Also I think we need to make these refpointers to make sure the queues don't get deleted from under us?
PTAL. Intended usage of this API can be found in crrev.com/2345483002 https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.h (right): https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.h:39: scheduler::TaskQueue*, On 2016/09/12 17:49:09, Sami wrote: > Could you make this const? Not easily — we may need to disable this queue when queue is throttled as a result of ReportTaskTime call. Or are you okay with const_cast in RendererSchedulerImpl::ReportTaskTime? https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.h (right): https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.h:153: base::Optional<base::TimeTicks> GetNextTaskRunTime() override; On 2016/09/12 17:45:26, alex clarke wrote: > I'm fine with adding GetNextScheduledWakeUp but I'm not keen on > GetNextTaskRunTime being part of the TaskQueue API for these reasons: > > 1. It's too similar to GetNextScheduledWakeUp. How would you know which to > call? > 2. You can simulate it with HasPendingImmediateWork() plus > GetNextScheduledWakeUp() > > I'd suggest moving the logic into a helper inside TaskQueueThrottler. I added a comment about this in TaskQueue itself, copying it here: Difference is that GetNextTaskRunTime behaves "atomically" i.e. it avoids problem when task becomes ready immediately after HasPendingImmediateWork. https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/base/task_time_tracker.h (right): https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/task_time_tracker.h:5: #ifndef THIRD_PARTY_WEBKIT_SOURCE_PLATFORM_SCHEDULER_BASE_TASK_TRACKER_H_ On 2016/09/12 17:49:09, Sami wrote: > TASK_TIME_TRACKER Deleted. https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/task_time_tracker.h:15: class TaskTimeTracker { On 2016/09/12 17:45:26, alex clarke wrote: > Is this used? Deleted. https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/task_time_tracker.h:25: DISSALLOW_COPY_AND_ASSIGN(TaskTimeTracker); On 2016/09/12 17:49:09, Sami wrote: > -S :) > > (It doesn't look like this file is being used since the build didn't fail?) Deleted. https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:1482: base::TimeTicks end_time_ticks = MonotonicTimeInSecondsToTimeTicks(end_time); On 2016/09/12 17:45:26, alex clarke wrote: > :( > > I'm not keen on these conversions. I may get rid of them if you don't get there > first ;) > > Please add a TODO(alexclarke): Remove conversions The problem is that ReportTaskTime interface was exposed to outside Blink world, and splitting it in two is costly and adds a lot of overhead. We should change doubles to base::TimeTicks throughout Blink, but I don't think that it's a good idea to do it in this patch. https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.h (right): https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.h:156: void CreateTraceEventObjectSnapshot() const; On 2016/09/12 17:45:26, alex clarke wrote: > Please add a comment explaining what this does. Done. https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc (right): https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:61: queue->SetQueueEnabled(false); On 2016/09/12 17:45:26, alex clarke wrote: > Is this safe? See TaskQueueThrottler::SetQueueEnable. > > The RendererScheduler may override this. Yes, it's safe to disable a throttled queue. For example, it's exactly what TaskQueueThrottler does for achieving timer alignment. https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:80: void TaskQueueThrottler::TimeBudgetPool::Enable(base::TimeTicks now) { On 2016/09/12 17:45:26, alex clarke wrote: > Lazy now? Done. https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:89: queue->SetQueueEnabled(false); On 2016/09/12 17:45:26, alex clarke wrote: > Ditto. Same as above. https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:96: void TaskQueueThrottler::TimeBudgetPool::Disable(base::TimeTicks now) { On 2016/09/12 17:45:26, alex clarke wrote: > I can't see how this is going to be used but I notice TimeBudgetPool::Enable > disables the queue but this doesn't. Do we run the risk of the queue not > getting re-enabled if TimeBudgetPool::Enable and TimeBudgetPool::Disable are > called in quick succession? We can't enable queue here because of timer alignment here. MaybeSchedulePumpQueue will do it when needed. https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:96: void TaskQueueThrottler::TimeBudgetPool::Disable(base::TimeTicks now) { On 2016/09/12 17:45:26, alex clarke wrote: > Lazy now? Done. https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:158: // Yes, that's memory-infra-approved way of tracing pointers. Deal with it. On 2016/09/12 17:49:10, Sami wrote: > I think we can do without this comment :) Done. https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:304: // GetNextTaskRunTime requires a lock and OnTimeDomainHasDelayedWork On 2016/09/12 17:49:10, Sami wrote: > nit: a lock on the queue? Done. https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:370: // GetNextScheduledWakeUp() moves tasks from incoming queue to delayed On 2016/09/12 17:45:26, alex clarke wrote: > 1. I'm not sure that's right, InsertFence should not change the result of any > operation I'm aware of to get the next wake up time. > 2. The formatting of this comment looks funny, please fix. My comment was badly phrased. We just want to move ready tasks to delayed_work_queue before installing a fence. https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:371: // queue, On 2016/09/12 17:49:10, Sami wrote: > nit: please reformat Done. https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:452: if (throttled_queues_.find(task_queue) == throttled_queues_.end()) On 2016/09/12 17:45:26, alex clarke wrote: > This is going to get called a lot, currently TaskQueueMap is > > using TaskQueueMap = std::map<TaskQueue*, Metadata>; > > I wonder if we should change that to: > > using TaskQueueMap = std::unordered_map<TaskQueue*, Metadata>; > Done. https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:491: TaskQueueThrottler::GetTimeBudgetPoolForQueue(TaskQueue* queue) { On 2016/09/12 17:45:26, alex clarke wrote: > If you move TimeBudgetPool* into the Metadata struct, I think this method can be > removed. It will break the assumption that we have only throttled queues in throttled_queues_ (unthrottled queue can have a time budget pool associated with it, but can't have a Metadata entry). https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:517: base::Optional<base::TimeTicks> next_run_time = budget->NextAllowedRunTime(); On 2016/09/12 17:49:10, Sami wrote: > I think it's a little weird for this to ask information from the budget -- which > is the one calling into this function. Should this method live on the budget > class instead? Done. https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h (right): https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h:48: // This class is main-thread only. On 2016/09/12 17:45:27, alex clarke wrote: > The TaskQueueThrottler (nee-ThrottlingHelper) is main thread only so consider > moving this comment to the top level. > > If we're worried some of the calls may come from the wrong thread, consider > adding a debug-only checker. Done. https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h:49: class TimeBudgetPool { On 2016/09/12 17:45:27, alex clarke wrote: > Do we even want this API to be public? It's public inside scheduler and I think we want to keep it this way — it was our explicit goal to provide a basis for a number of interventions. https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h:49: class TimeBudgetPool { On 2016/09/12 17:45:27, alex clarke wrote: > Please add an overview comment explaining what the job of TimeBudgetPool is. Done. https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h:58: void AddQueue(base::TimeTicks now, TaskQueue* queue); On 2016/09/12 17:45:27, alex clarke wrote: > Please document these. Side effects such |queue| getting disabled are a bit > surprising to me given the name. > > Note without seeing the code that uses this I can't tell if this is a problem > with the name or the functionality yet. Done. https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h:58: void AddQueue(base::TimeTicks now, TaskQueue* queue); On 2016/09/12 17:45:27, alex clarke wrote: > Please document these. Side effects such |queue| getting disabled are a bit > surprising to me given the name. > > Note without seeing the code that uses this I can't tell if this is a problem > with the name or the functionality yet. Done. https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h:61: void RecordTaskRunTime(base::TimeDelta task_run_time); On 2016/09/12 17:49:10, Sami wrote: > Does all of this functionality need to be public or could we move the parts that > only the throttler needs to be private? > > Alternatively should we make this class private and just provide the few > entrypoints we need in the TaskQueueThrottler public API? Done. https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h:66: void Enable(base::TimeTicks now); On 2016/09/12 17:49:10, Sami wrote: > What do enable/disable mean for a budget pool? Maybe there's a more descriptive > operation name we could use? It means "activate this pool and start throttling queues associated with it". Main use case is when renderer is backgrounded/foregrounded we want to enable/disable time budget pool corresponding to throttling background tabs. https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h:66: void Enable(base::TimeTicks now); On 2016/09/12 17:45:27, alex clarke wrote: > Please document Enable & Disable. Done. https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h:73: void Close(); On 2016/09/12 17:45:27, alex clarke wrote: > This only seems to be called by test code, is that expected? Makes it hard to > understand the lifetime of this object. For the moment — yes, it's expected. Lifetime is simple — it's deleted when Close() is called or when TaskQueueThrottler (and RendererScheduler) is deleted. https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h:100: std::unordered_set<TaskQueue*> assosiated_task_queues_; On 2016/09/12 17:49:10, Sami wrote: > typo: associated. > > Also I think we need to make these refpointers to make sure the queues don't get > deleted from under us? Done. https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h:149: TimeBudgetPool* CreateTimeBudgetPool(const char* name); On 2016/09/12 17:45:26, alex clarke wrote: > This only seems to be called by tests, is that expected? For the moment yes, but it will change in the next patch. https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h:205: time_budget_pools_; On 2016/09/12 17:45:27, alex clarke wrote: > This construct gives me pause for thought. I can't tell what the lifecycle of > TimeBudgetPool is outside of tests yet, but this makes me wonder if is should > actually be ref counted? My understanding was that TaskQueueThrottler (and RendererScheduler) outlive pretty much everything in renderer. If this is correct, this approach should be fine. https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h:206: std::unordered_map<TaskQueue*, TimeBudgetPool*> time_budget_pool_for_queue_; On 2016/09/12 17:45:27, alex clarke wrote: > Can we instead move TimeBudgetPool* into struct Metadata? It will break the assumption that queue is throttled when it is in throttled_queues_. I'm not sure if we want to do it. https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/renderer/web_frame_scheduler_impl.cc (right): https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/web_frame_scheduler_impl.cc:80: On 2016/09/12 17:45:27, alex clarke wrote: > nit: remove stray white space here and below. Done. https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/renderer/web_view_scheduler_impl.h (right): https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/web_view_scheduler_impl.h:62: friend class WebFrameSchedulerImpl; On 2016/09/12 17:45:27, alex clarke wrote: > Is this still needed? Done. https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/pub... File third_party/WebKit/public/platform/scheduler/base/task_time_observer.h (right): https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/pub... third_party/WebKit/public/platform/scheduler/base/task_time_observer.h:16: class BLINK_PLATFORM_EXPORT TaskTimeObserver { On 2016/09/12 17:45:27, alex clarke wrote: > We're trying to add some top level comments to scheduler header files. Could > you briefly explain what this class is for. Done. https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/pub... third_party/WebKit/public/platform/scheduler/base/task_time_observer.h:21: virtual void ReportTaskTime(TaskQueue* task_queue, On 2016/09/12 17:45:27, alex clarke wrote: > Please add a comment explaining what the members are. I know it's obvious to > us, but it may not be to those new to this code. Done.
PTAL. Intended usage of this API can be found in crrev.com/2345483002 https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.h (right): https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.h:39: scheduler::TaskQueue*, On 2016/09/12 17:49:09, Sami wrote: > Could you make this const? Not easily — we may need to disable this queue when queue is throttled as a result of ReportTaskTime call. Or are you okay with const_cast in RendererSchedulerImpl::ReportTaskTime? https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.h (right): https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.h:153: base::Optional<base::TimeTicks> GetNextTaskRunTime() override; On 2016/09/12 17:45:26, alex clarke wrote: > I'm fine with adding GetNextScheduledWakeUp but I'm not keen on > GetNextTaskRunTime being part of the TaskQueue API for these reasons: > > 1. It's too similar to GetNextScheduledWakeUp. How would you know which to > call? > 2. You can simulate it with HasPendingImmediateWork() plus > GetNextScheduledWakeUp() > > I'd suggest moving the logic into a helper inside TaskQueueThrottler. I added a comment about this in TaskQueue itself, copying it here: Difference is that GetNextTaskRunTime behaves "atomically" i.e. it avoids problem when task becomes ready immediately after HasPendingImmediateWork. https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/base/task_time_tracker.h (right): https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/task_time_tracker.h:5: #ifndef THIRD_PARTY_WEBKIT_SOURCE_PLATFORM_SCHEDULER_BASE_TASK_TRACKER_H_ On 2016/09/12 17:49:09, Sami wrote: > TASK_TIME_TRACKER Deleted. https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/task_time_tracker.h:15: class TaskTimeTracker { On 2016/09/12 17:45:26, alex clarke wrote: > Is this used? Deleted. https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/task_time_tracker.h:25: DISSALLOW_COPY_AND_ASSIGN(TaskTimeTracker); On 2016/09/12 17:49:09, Sami wrote: > -S :) > > (It doesn't look like this file is being used since the build didn't fail?) Deleted. https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:1482: base::TimeTicks end_time_ticks = MonotonicTimeInSecondsToTimeTicks(end_time); On 2016/09/12 17:45:26, alex clarke wrote: > :( > > I'm not keen on these conversions. I may get rid of them if you don't get there > first ;) > > Please add a TODO(alexclarke): Remove conversions The problem is that ReportTaskTime interface was exposed to outside Blink world, and splitting it in two is costly and adds a lot of overhead. We should change doubles to base::TimeTicks throughout Blink, but I don't think that it's a good idea to do it in this patch. https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.h (right): https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.h:156: void CreateTraceEventObjectSnapshot() const; On 2016/09/12 17:45:26, alex clarke wrote: > Please add a comment explaining what this does. Done. https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc (right): https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:61: queue->SetQueueEnabled(false); On 2016/09/12 17:45:26, alex clarke wrote: > Is this safe? See TaskQueueThrottler::SetQueueEnable. > > The RendererScheduler may override this. Yes, it's safe to disable a throttled queue. For example, it's exactly what TaskQueueThrottler does for achieving timer alignment. https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:80: void TaskQueueThrottler::TimeBudgetPool::Enable(base::TimeTicks now) { On 2016/09/12 17:45:26, alex clarke wrote: > Lazy now? Done. https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:89: queue->SetQueueEnabled(false); On 2016/09/12 17:45:26, alex clarke wrote: > Ditto. Same as above. https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:96: void TaskQueueThrottler::TimeBudgetPool::Disable(base::TimeTicks now) { On 2016/09/12 17:45:26, alex clarke wrote: > I can't see how this is going to be used but I notice TimeBudgetPool::Enable > disables the queue but this doesn't. Do we run the risk of the queue not > getting re-enabled if TimeBudgetPool::Enable and TimeBudgetPool::Disable are > called in quick succession? We can't enable queue here because of timer alignment here. MaybeSchedulePumpQueue will do it when needed. https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:96: void TaskQueueThrottler::TimeBudgetPool::Disable(base::TimeTicks now) { On 2016/09/12 17:45:26, alex clarke wrote: > Lazy now? Done. https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:158: // Yes, that's memory-infra-approved way of tracing pointers. Deal with it. On 2016/09/12 17:49:10, Sami wrote: > I think we can do without this comment :) Done. https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:304: // GetNextTaskRunTime requires a lock and OnTimeDomainHasDelayedWork On 2016/09/12 17:49:10, Sami wrote: > nit: a lock on the queue? Done. https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:370: // GetNextScheduledWakeUp() moves tasks from incoming queue to delayed On 2016/09/12 17:45:26, alex clarke wrote: > 1. I'm not sure that's right, InsertFence should not change the result of any > operation I'm aware of to get the next wake up time. > 2. The formatting of this comment looks funny, please fix. My comment was badly phrased. We just want to move ready tasks to delayed_work_queue before installing a fence. https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:371: // queue, On 2016/09/12 17:49:10, Sami wrote: > nit: please reformat Done. https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:452: if (throttled_queues_.find(task_queue) == throttled_queues_.end()) On 2016/09/12 17:45:26, alex clarke wrote: > This is going to get called a lot, currently TaskQueueMap is > > using TaskQueueMap = std::map<TaskQueue*, Metadata>; > > I wonder if we should change that to: > > using TaskQueueMap = std::unordered_map<TaskQueue*, Metadata>; > Done. https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:491: TaskQueueThrottler::GetTimeBudgetPoolForQueue(TaskQueue* queue) { On 2016/09/12 17:45:26, alex clarke wrote: > If you move TimeBudgetPool* into the Metadata struct, I think this method can be > removed. It will break the assumption that we have only throttled queues in throttled_queues_ (unthrottled queue can have a time budget pool associated with it, but can't have a Metadata entry). https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:517: base::Optional<base::TimeTicks> next_run_time = budget->NextAllowedRunTime(); On 2016/09/12 17:49:10, Sami wrote: > I think it's a little weird for this to ask information from the budget -- which > is the one calling into this function. Should this method live on the budget > class instead? Done. https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h (right): https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h:48: // This class is main-thread only. On 2016/09/12 17:45:27, alex clarke wrote: > The TaskQueueThrottler (nee-ThrottlingHelper) is main thread only so consider > moving this comment to the top level. > > If we're worried some of the calls may come from the wrong thread, consider > adding a debug-only checker. Done. https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h:49: class TimeBudgetPool { On 2016/09/12 17:45:27, alex clarke wrote: > Do we even want this API to be public? It's public inside scheduler and I think we want to keep it this way — it was our explicit goal to provide a basis for a number of interventions. https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h:49: class TimeBudgetPool { On 2016/09/12 17:45:27, alex clarke wrote: > Please add an overview comment explaining what the job of TimeBudgetPool is. Done. https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h:58: void AddQueue(base::TimeTicks now, TaskQueue* queue); On 2016/09/12 17:45:27, alex clarke wrote: > Please document these. Side effects such |queue| getting disabled are a bit > surprising to me given the name. > > Note without seeing the code that uses this I can't tell if this is a problem > with the name or the functionality yet. Done. https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h:58: void AddQueue(base::TimeTicks now, TaskQueue* queue); On 2016/09/12 17:45:27, alex clarke wrote: > Please document these. Side effects such |queue| getting disabled are a bit > surprising to me given the name. > > Note without seeing the code that uses this I can't tell if this is a problem > with the name or the functionality yet. Done. https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h:61: void RecordTaskRunTime(base::TimeDelta task_run_time); On 2016/09/12 17:49:10, Sami wrote: > Does all of this functionality need to be public or could we move the parts that > only the throttler needs to be private? > > Alternatively should we make this class private and just provide the few > entrypoints we need in the TaskQueueThrottler public API? Done. https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h:66: void Enable(base::TimeTicks now); On 2016/09/12 17:49:10, Sami wrote: > What do enable/disable mean for a budget pool? Maybe there's a more descriptive > operation name we could use? It means "activate this pool and start throttling queues associated with it". Main use case is when renderer is backgrounded/foregrounded we want to enable/disable time budget pool corresponding to throttling background tabs. https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h:66: void Enable(base::TimeTicks now); On 2016/09/12 17:45:27, alex clarke wrote: > Please document Enable & Disable. Done. https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h:73: void Close(); On 2016/09/12 17:45:27, alex clarke wrote: > This only seems to be called by test code, is that expected? Makes it hard to > understand the lifetime of this object. For the moment — yes, it's expected. Lifetime is simple — it's deleted when Close() is called or when TaskQueueThrottler (and RendererScheduler) is deleted. https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h:100: std::unordered_set<TaskQueue*> assosiated_task_queues_; On 2016/09/12 17:49:10, Sami wrote: > typo: associated. > > Also I think we need to make these refpointers to make sure the queues don't get > deleted from under us? Done. https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h:149: TimeBudgetPool* CreateTimeBudgetPool(const char* name); On 2016/09/12 17:45:26, alex clarke wrote: > This only seems to be called by tests, is that expected? For the moment yes, but it will change in the next patch. https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h:205: time_budget_pools_; On 2016/09/12 17:45:27, alex clarke wrote: > This construct gives me pause for thought. I can't tell what the lifecycle of > TimeBudgetPool is outside of tests yet, but this makes me wonder if is should > actually be ref counted? My understanding was that TaskQueueThrottler (and RendererScheduler) outlive pretty much everything in renderer. If this is correct, this approach should be fine. https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h:206: std::unordered_map<TaskQueue*, TimeBudgetPool*> time_budget_pool_for_queue_; On 2016/09/12 17:45:27, alex clarke wrote: > Can we instead move TimeBudgetPool* into struct Metadata? It will break the assumption that queue is throttled when it is in throttled_queues_. I'm not sure if we want to do it. https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/renderer/web_frame_scheduler_impl.cc (right): https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/web_frame_scheduler_impl.cc:80: On 2016/09/12 17:45:27, alex clarke wrote: > nit: remove stray white space here and below. Done. https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/renderer/web_view_scheduler_impl.h (right): https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/web_view_scheduler_impl.h:62: friend class WebFrameSchedulerImpl; On 2016/09/12 17:45:27, alex clarke wrote: > Is this still needed? Done. https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/pub... File third_party/WebKit/public/platform/scheduler/base/task_time_observer.h (right): https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/pub... third_party/WebKit/public/platform/scheduler/base/task_time_observer.h:16: class BLINK_PLATFORM_EXPORT TaskTimeObserver { On 2016/09/12 17:45:27, alex clarke wrote: > We're trying to add some top level comments to scheduler header files. Could > you briefly explain what this class is for. Done. https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/pub... third_party/WebKit/public/platform/scheduler/base/task_time_observer.h:21: virtual void ReportTaskTime(TaskQueue* task_queue, On 2016/09/12 17:45:27, alex clarke wrote: > Please add a comment explaining what the members are. I know it's obvious to > us, but it may not be to those new to this code. Done.
https://codereview.chromium.org/2258133002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.cc (right): https://codereview.chromium.org/2258133002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.cc:543: MoveReadyDelayedTasksToDelayedWorkQueue(&lazy_now); Per offline discussion lets drop the call to MoveReadyDelayedTasksToDelayedWorkQueue. https://codereview.chromium.org/2258133002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.cc:551: base::Optional<base::TimeTicks> TaskQueueImpl::GetNextTaskRunTime() { Per offline discussion lets try and remove this from the API. https://codereview.chromium.org/2258133002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc (left): https://codereview.chromium.org/2258133002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:83: if (task_queue->HasPendingImmediateWork()) { Do we need to change this? https://codereview.chromium.org/2258133002/diff/140001/third_party/WebKit/pub... File third_party/WebKit/public/platform/scheduler/base/task_queue.h (right): https://codereview.chromium.org/2258133002/diff/140001/third_party/WebKit/pub... third_party/WebKit/public/platform/scheduler/base/task_queue.h:170: virtual base::Optional<base::TimeTicks> GetNextTaskRunTime() = 0; No please don't put this here. It's confusing from an interface point of view.
https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.h (right): https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.h:39: scheduler::TaskQueue*, On 2016/09/14 11:23:14, altimin wrote: > On 2016/09/12 17:49:09, Sami wrote: > > Could you make this const? > > Not easily — we may need to disable this queue when queue is throttled as a > result of ReportTaskTime call. > > Or are you okay with const_cast in RendererSchedulerImpl::ReportTaskTime? I see. Let's keep this non-const then. https://codereview.chromium.org/2258133002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h (right): https://codereview.chromium.org/2258133002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h:77: void Enable(LazyNow* now); nit: How about EnableDisableThrottling()? I find it a bit counter intuitive that calling budget_pool->Disable() actually allows in the pool to run.
PTAL https://codereview.chromium.org/2258133002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.cc (right): https://codereview.chromium.org/2258133002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.cc:543: MoveReadyDelayedTasksToDelayedWorkQueue(&lazy_now); On 2016/09/14 12:36:34, alex clarke wrote: > Per offline discussion lets drop the call to > MoveReadyDelayedTasksToDelayedWorkQueue. Done. https://codereview.chromium.org/2258133002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.cc:551: base::Optional<base::TimeTicks> TaskQueueImpl::GetNextTaskRunTime() { On 2016/09/14 12:36:34, alex clarke wrote: > Per offline discussion lets try and remove this from the API. Done. https://codereview.chromium.org/2258133002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc (left): https://codereview.chromium.org/2258133002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:83: if (task_queue->HasPendingImmediateWork()) { On 2016/09/14 12:36:34, alex clarke wrote: > Do we need to change this? Done. https://codereview.chromium.org/2258133002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h (right): https://codereview.chromium.org/2258133002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h:77: void Enable(LazyNow* now); On 2016/09/14 13:50:33, Sami wrote: > nit: How about EnableDisableThrottling()? I find it a bit counter intuitive that > calling budget_pool->Disable() actually allows in the pool to run. Done. https://codereview.chromium.org/2258133002/diff/140001/third_party/WebKit/pub... File third_party/WebKit/public/platform/scheduler/base/task_queue.h (right): https://codereview.chromium.org/2258133002/diff/140001/third_party/WebKit/pub... third_party/WebKit/public/platform/scheduler/base/task_queue.h:170: virtual base::Optional<base::TimeTicks> GetNextTaskRunTime() = 0; On 2016/09/14 12:36:34, alex clarke wrote: > No please don't put this here. It's confusing from an interface point of view. Done.
https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h (right): https://codereview.chromium.org/2258133002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h:206: std::unordered_map<TaskQueue*, TimeBudgetPool*> time_budget_pool_for_queue_; On 2016/09/14 11:23:17, altimin wrote: > On 2016/09/12 17:45:27, alex clarke wrote: > > Can we instead move TimeBudgetPool* into struct Metadata? > > It will break the assumption that queue is throttled when it is in > throttled_queues_. I'm not sure if we want to do it. I don't see why we'd want to have multiple maps (the map itself has overhead). Instead I think we can simply add a is_throttled bool to Metadata. https://codereview.chromium.org/2258133002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.cc (right): https://codereview.chromium.org/2258133002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.cc:517: LazyNow lazy_now = main_thread_only().time_domain->CreateLazyNow(); nit: revert this and change on line 529 https://codereview.chromium.org/2258133002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.cc:720: // the reformat please. https://codereview.chromium.org/2258133002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc (right): https://codereview.chromium.org/2258133002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:229: nit: remove blank line here and after the FOR_EACH_OBSERVER https://codereview.chromium.org/2258133002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/base/time_domain.h (right): https://codereview.chromium.org/2258133002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/time_domain.h:45: virtual void OnTimeDomainHasImmediateWork(TaskQueue*) = 0; We're using chromium style here so please add a variable name and also document what it contains. https://codereview.chromium.org/2258133002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/time_domain.h:49: virtual void OnTimeDomainHasDelayedWork(TaskQueue*) = 0; Ditto. https://codereview.chromium.org/2258133002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/child/scheduler_helper.h (right): https://codereview.chromium.org/2258133002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/child/scheduler_helper.h:67: inline void CheckOnValidThread() const { does this make any difference? https://codereview.chromium.org/2258133002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/2258133002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:1499: nit: remove blank line https://codereview.chromium.org/2258133002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc (right): https://codereview.chromium.org/2258133002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:63: if (!task_queue_throttler_->IsThrottled(queue)) Note: if |time_budget_pool_for_queue_| gets rolled into Metadata we can do one map lookup instead of two. https://codereview.chromium.org/2258133002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:125: bool TaskQueueThrottler::TimeBudgetPool::IsAllowedToRun(base::TimeTicks now) { I wonder if HasEnoughBudgetToRun is a better name, Sami WDYT? https://codereview.chromium.org/2258133002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:358: TRACE_EVENT0("renderer.scheduler", "TaskQueueThrottler::PumpThrottledTasks"); Why change tracing_category_? https://codereview.chromium.org/2258133002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:369: base::TimeTicks next_allowed_run_time = Please add a comment here saying: Don't pump queues whose budget pool doesn't allow them to run now. https://codereview.chromium.org/2258133002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:377: "renderer.scheduler", tracing_category_? https://codereview.chromium.org/2258133002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:384: } can we put continue; in there for clarity? https://codereview.chromium.org/2258133002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:389: if (next_allowed_run_time == lazy_now.Now()) { For readability I would be tempted instead to write; if (next_allowed_run_time > lazy_now.Now()) continue; task_queue->SetQueueEnabled(true); task_queue->InsertFence(); https://codereview.chromium.org/2258133002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:416: base::TimeTicks runtime) { unaligned_runtiume https://codereview.chromium.org/2258133002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:473: if (time_budget_pool) { For readability I think we should prefer: if (!time_budget_pool) return; https://codereview.chromium.org/2258133002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:528: if (next_run_time) { Can this ever actually be false? Should we DCHECK? https://codereview.chromium.org/2258133002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h (right): https://codereview.chromium.org/2258133002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h:28: // queues. TaskQueueThrottler s/TaskQueueThrottler/The TaskQueueThrottler: https://codereview.chromium.org/2258133002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h:31: // This is done by disabling throttled queues and running Maybe add a blank line before this. https://codereview.chromium.org/2258133002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h:52: // Consider putting "This limit..." onto the same line as "on execution time." It reads like it should be part of the same paragraph. https://codereview.chromium.org/2258133002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h:171: void OnTaskRunTimeReported(TaskQueue* task_queue, Please document what this does. https://codereview.chromium.org/2258133002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h:191: TimeBudgetPool* time_budget_pool; // NOT OWNED
PTAL https://codereview.chromium.org/2258133002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.cc (right): https://codereview.chromium.org/2258133002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.cc:517: LazyNow lazy_now = main_thread_only().time_domain->CreateLazyNow(); On 2016/09/15 12:19:34, alex clarke wrote: > nit: revert this and change on line 529 Done. https://codereview.chromium.org/2258133002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.cc:720: // the On 2016/09/15 12:19:34, alex clarke wrote: > reformat please. Done. https://codereview.chromium.org/2258133002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc (right): https://codereview.chromium.org/2258133002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:229: On 2016/09/15 12:19:35, alex clarke wrote: > nit: remove blank line here and after the FOR_EACH_OBSERVER Done. https://codereview.chromium.org/2258133002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/base/time_domain.h (right): https://codereview.chromium.org/2258133002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/time_domain.h:45: virtual void OnTimeDomainHasImmediateWork(TaskQueue*) = 0; On 2016/09/15 12:19:35, alex clarke wrote: > We're using chromium style here so please add a variable name and also document > what it contains. Done. https://codereview.chromium.org/2258133002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/time_domain.h:49: virtual void OnTimeDomainHasDelayedWork(TaskQueue*) = 0; On 2016/09/15 12:19:35, alex clarke wrote: > Ditto. Done. https://codereview.chromium.org/2258133002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/child/scheduler_helper.h (right): https://codereview.chromium.org/2258133002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/child/scheduler_helper.h:67: inline void CheckOnValidThread() const { On 2016/09/15 12:19:35, alex clarke wrote: > does this make any difference? It gives us guarantee that we will not call this function with DCHECKs disabled. https://codereview.chromium.org/2258133002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/2258133002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:1499: On 2016/09/15 12:19:35, alex clarke wrote: > nit: remove blank line Done. https://codereview.chromium.org/2258133002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc (right): https://codereview.chromium.org/2258133002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:63: if (!task_queue_throttler_->IsThrottled(queue)) On 2016/09/15 12:19:35, alex clarke wrote: > Note: if |time_budget_pool_for_queue_| gets rolled into Metadata we can do one > map lookup instead of two. It's not feasible to do this, because queue can be unthrottled and can have a time budget pool. https://codereview.chromium.org/2258133002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:125: bool TaskQueueThrottler::TimeBudgetPool::IsAllowedToRun(base::TimeTicks now) { On 2016/09/15 12:19:35, alex clarke wrote: > I wonder if HasEnoughBudgetToRun is a better name, Sami WDYT? Done. https://codereview.chromium.org/2258133002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:358: TRACE_EVENT0("renderer.scheduler", "TaskQueueThrottler::PumpThrottledTasks"); On 2016/09/15 12:19:35, alex clarke wrote: > Why change tracing_category_? Done. https://codereview.chromium.org/2258133002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:369: base::TimeTicks next_allowed_run_time = On 2016/09/15 12:19:35, alex clarke wrote: > Please add a comment here saying: Don't pump queues whose budget pool doesn't > allow them to run now. Done. https://codereview.chromium.org/2258133002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:377: "renderer.scheduler", On 2016/09/15 12:19:35, alex clarke wrote: > tracing_category_? Done. https://codereview.chromium.org/2258133002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:384: } On 2016/09/15 12:19:35, alex clarke wrote: > can we put continue; in there for clarity? Done. https://codereview.chromium.org/2258133002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:389: if (next_allowed_run_time == lazy_now.Now()) { On 2016/09/15 12:19:35, alex clarke wrote: > For readability I would be tempted instead to write; > > if (next_allowed_run_time > lazy_now.Now()) > continue; > > task_queue->SetQueueEnabled(true); > task_queue->InsertFence(); Done. https://codereview.chromium.org/2258133002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:416: base::TimeTicks runtime) { On 2016/09/15 12:19:35, alex clarke wrote: > unaligned_runtiume Done. https://codereview.chromium.org/2258133002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:473: if (time_budget_pool) { On 2016/09/15 12:19:35, alex clarke wrote: > For readability I think we should prefer: > > if (!time_budget_pool) > return; Done. https://codereview.chromium.org/2258133002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:528: if (next_run_time) { On 2016/09/15 12:19:35, alex clarke wrote: > Can this ever actually be false? Should we DCHECK? Yes. We're calling MaybeSchedulePumpQueue when we're adding/removing queues, and they can be empty, and |next_run_time| can be null. https://codereview.chromium.org/2258133002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h (right): https://codereview.chromium.org/2258133002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h:28: // queues. TaskQueueThrottler On 2016/09/15 12:19:35, alex clarke wrote: > s/TaskQueueThrottler/The TaskQueueThrottler: Done. https://codereview.chromium.org/2258133002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h:31: // This is done by disabling throttled queues and running On 2016/09/15 12:19:35, alex clarke wrote: > Maybe add a blank line before this. Done. https://codereview.chromium.org/2258133002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h:52: // On 2016/09/15 12:19:35, alex clarke wrote: > Consider putting "This limit..." onto the same line as "on execution time." > > It reads like it should be part of the same paragraph. Done. https://codereview.chromium.org/2258133002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h:171: void OnTaskRunTimeReported(TaskQueue* task_queue, On 2016/09/15 12:19:36, alex clarke wrote: > Please document what this does. Done. https://codereview.chromium.org/2258133002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h:191: TimeBudgetPool* time_budget_pool; On 2016/09/15 12:19:35, alex clarke wrote: > // NOT OWNED Deleted.
https://codereview.chromium.org/2258133002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc (right): https://codereview.chromium.org/2258133002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:63: if (!task_queue_throttler_->IsThrottled(queue)) On 2016/09/15 15:52:11, altimin wrote: > On 2016/09/15 12:19:35, alex clarke wrote: > > Note: if |time_budget_pool_for_queue_| gets rolled into Metadata we can do one > > map lookup instead of two. > > It's not feasible to do this, because queue can be unthrottled and can have a > time budget pool. That's not true. Just because |throttled_queues_| currently only contains throttled queues doesn't mean it has to stay that way. Obviously we'd need to change the name, perhaps |queue_details_| is a better name.
I think this is looking pretty neat now. One question about the set of pool operations. https://codereview.chromium.org/2258133002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.cc (right): https://codereview.chromium.org/2258133002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.cc:733: // We rely here on TimeDomain::Register/UnregisterAsUpdatableTaskQueue being nit: really we're relying on TimeDomain::MigrateQueue being thread safe. Could you add a comment to time_domain.h next to it saying that it is? https://codereview.chromium.org/2258133002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.h (right): https://codereview.chromium.org/2258133002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.h:178: TaskQueueThrottler* task_queue_throttler() { const? https://codereview.chromium.org/2258133002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h (right): https://codereview.chromium.org/2258133002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h:9: #include <map> Do we need this? https://codereview.chromium.org/2258133002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h:212: base::Optional<base::TimeTicks> next_possible_run_time = base::nullopt); nit: default arguments are generally not recommended. https://codereview.chromium.org/2258133002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc (right): https://codereview.chromium.org/2258133002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:85: void TaskQueueThrottler::TimeBudgetPool::EnableThrottling(LazyNow* lazy_now) { Maybe this makes more sense in future patches, but I'm wondering why we need an enable/disable for each pool instead of moving task queues between pools (or the null pool)? https://codereview.chromium.org/2258133002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h (right): https://codereview.chromium.org/2258133002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h:119: std::unordered_set<TaskQueue*> associated_task_queues_; I think this works too, but as a general comment the lifetime of task queues is a bit special because they are refcounted and shared across threads. We might want to refactor everything here to keep references because the API doesn't really prevent anyone from destroying a task queue without unregistering it here first.
PTAL Alex, thanks for the thorough review! https://codereview.chromium.org/2258133002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.cc (right): https://codereview.chromium.org/2258133002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.cc:733: // We rely here on TimeDomain::Register/UnregisterAsUpdatableTaskQueue being On 2016/09/15 16:30:29, Sami wrote: > nit: really we're relying on TimeDomain::MigrateQueue being thread safe. Could > you add a comment to time_domain.h next to it saying that it is? It's more tricky. TimeDomain::MigrateQueue is main-thread only, but it's really about other calls to Register/Unregister from task_queue_impl.cc which we're worried about. https://codereview.chromium.org/2258133002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.h (right): https://codereview.chromium.org/2258133002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.h:178: TaskQueueThrottler* task_queue_throttler() { On 2016/09/15 16:30:29, Sami wrote: > const? Done. https://codereview.chromium.org/2258133002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc (right): https://codereview.chromium.org/2258133002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:63: if (!task_queue_throttler_->IsThrottled(queue)) On 2016/09/15 16:18:40, alex clarke wrote: > On 2016/09/15 15:52:11, altimin wrote: > > On 2016/09/15 12:19:35, alex clarke wrote: > > > Note: if |time_budget_pool_for_queue_| gets rolled into Metadata we can do > one > > > map lookup instead of two. > > > > It's not feasible to do this, because queue can be unthrottled and can have a > > time budget pool. > > That's not true. Just because |throttled_queues_| currently only contains > throttled queues doesn't mean it has to stay that way. > > Obviously we'd need to change the name, perhaps |queue_details_| is a better > name. Done. https://codereview.chromium.org/2258133002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h (right): https://codereview.chromium.org/2258133002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h:9: #include <map> On 2016/09/15 16:30:30, Sami wrote: > Do we need this? Done. https://codereview.chromium.org/2258133002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h:212: base::Optional<base::TimeTicks> next_possible_run_time = base::nullopt); On 2016/09/15 16:30:30, Sami wrote: > nit: default arguments are generally not recommended. Done. https://codereview.chromium.org/2258133002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc (right): https://codereview.chromium.org/2258133002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:85: void TaskQueueThrottler::TimeBudgetPool::EnableThrottling(LazyNow* lazy_now) { On 2016/09/15 16:30:30, Sami wrote: > Maybe this makes more sense in future patches, but I'm wondering why we need an > enable/disable for each pool instead of moving task queues between pools (or the > null pool)? For example, when you want to start/stop throttling task queues when renderer goes into background/foreground it's more convenient to just call Enable/Disable than manually Add/Remove all queues. https://codereview.chromium.org/2258133002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h (right): https://codereview.chromium.org/2258133002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h:119: std::unordered_set<TaskQueue*> associated_task_queues_; On 2016/09/15 16:30:30, Sami wrote: > I think this works too, but as a general comment the lifetime of task queues is > a bit special because they are refcounted and shared across threads. We might > want to refactor everything here to keep references because the API doesn't > really prevent anyone from destroying a task queue without unregistering it here > first. I think it should be responsibility of the user who calls AddQueue to call RemoveQueue (same as Increase/DecreaseThrottleRefCount).
The CQ bit was checked by altimin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2258133002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc (right): https://codereview.chromium.org/2258133002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:63: if (!task_queue_throttler_->IsThrottled(queue)) On 2016/09/16 13:38:48, altimin wrote: > On 2016/09/15 16:18:40, alex clarke wrote: > > On 2016/09/15 15:52:11, altimin wrote: > > > On 2016/09/15 12:19:35, alex clarke wrote: > > > > Note: if |time_budget_pool_for_queue_| gets rolled into Metadata we can do > > one > > > > map lookup instead of two. > > > > > > It's not feasible to do this, because queue can be unthrottled and can have > a > > > time budget pool. > > > > That's not true. Just because |throttled_queues_| currently only contains > > throttled queues doesn't mean it has to stay that way. > > > > Obviously we'd need to change the name, perhaps |queue_details_| is a better > > name. > > Done. I'm not seeing this change, did you forget up load the patchset? https://codereview.chromium.org/2258133002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h (right): https://codereview.chromium.org/2258133002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h:26: // The job of the TaskQueueThrottler is to control tasks posted on throttled The job of the TaskQueueThrottler is to control when tasks posted on throttled queues get run. https://codereview.chromium.org/2258133002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h:65: void AddQueue(base::TimeTicks now, TaskQueue* queue); nit: blank line below this. https://codereview.chromium.org/2258133002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h:75: void EnableThrottling(LazyNow* now); nit: blank line below this.
PTAL https://codereview.chromium.org/2258133002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc (right): https://codereview.chromium.org/2258133002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:63: if (!task_queue_throttler_->IsThrottled(queue)) On 2016/09/16 14:36:18, alex clarke wrote: > On 2016/09/16 13:38:48, altimin wrote: > > On 2016/09/15 16:18:40, alex clarke wrote: > > > On 2016/09/15 15:52:11, altimin wrote: > > > > On 2016/09/15 12:19:35, alex clarke wrote: > > > > > Note: if |time_budget_pool_for_queue_| gets rolled into Metadata we can > do > > > one > > > > > map lookup instead of two. > > > > > > > > It's not feasible to do this, because queue can be unthrottled and can > have > > a > > > > time budget pool. > > > > > > That's not true. Just because |throttled_queues_| currently only contains > > > throttled queues doesn't mean it has to stay that way. > > > > > > Obviously we'd need to change the name, perhaps |queue_details_| is a better > > > name. > > > > Done. > > I'm not seeing this change, did you forget up load the patchset? Sorry, fixed now. https://codereview.chromium.org/2258133002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h (right): https://codereview.chromium.org/2258133002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h:26: // The job of the TaskQueueThrottler is to control tasks posted on throttled On 2016/09/16 14:36:18, alex clarke wrote: > The job of the TaskQueueThrottler is to control when tasks posted on throttled > queues get run. Done. https://codereview.chromium.org/2258133002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h:65: void AddQueue(base::TimeTicks now, TaskQueue* queue); On 2016/09/16 14:36:18, alex clarke wrote: > nit: blank line below this. Done. https://codereview.chromium.org/2258133002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h:75: void EnableThrottling(LazyNow* now); On 2016/09/16 14:36:18, alex clarke wrote: > nit: blank line below this. Done.
Feels like it's close now. https://codereview.chromium.org/2258133002/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc (right): https://codereview.chromium.org/2258133002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:63: if (metadata->throttling_ref_count == 0) Lets add an IsThrottled() helper function. https://codereview.chromium.org/2258133002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:520: if (!IsThrottled(task_queue)) Just want to make sure I understand this. Is it possible for a queue to be in a TimeBudgetPool but not be throttled? If so is this check the right thing to do?
PTAL https://codereview.chromium.org/2258133002/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc (right): https://codereview.chromium.org/2258133002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:63: if (metadata->throttling_ref_count == 0) On 2016/09/16 15:23:38, alex clarke wrote: > Lets add an IsThrottled() helper function. Done. https://codereview.chromium.org/2258133002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:520: if (!IsThrottled(task_queue)) On 2016/09/16 15:23:38, alex clarke wrote: > Just want to make sure I understand this. Is it possible for a queue to be in a > TimeBudgetPool but not be throttled? > > If so is this check the right thing to do? Yes, yes. Example use case: we have a common time budget pool for offscreen frames and we're throttling only ones which are offscreen. Onscreen frames are added to same budget pool but not throttled.
lgtm
altimin@chromium.org changed reviewers: + caseq@chromium.org, haraken@chromium.org
+caseq@ for core/inspector +haraken@ for platform/BUILD.gn PTAL
inspector/ lgtm + a bunch of drive-by comments. https://codereview.chromium.org/2258133002/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/base/test_task_time_observer.h (right): https://codereview.chromium.org/2258133002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/test_task_time_observer.h:18: double end_time) override{}; style: add space before {} https://codereview.chromium.org/2258133002/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/2258133002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:306: if (task_queue_throttler_.get()) nit: get() is redundant here. https://codereview.chromium.org/2258133002/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.h (right): https://codereview.chromium.org/2258133002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.h:20: #include "platform/scheduler/renderer/task_queue_throttler.h" Is this necessary? If so, please remove TaskQueueThrottler forward declaration below. https://codereview.chromium.org/2258133002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.h:157: void CreateTraceEventObjectSnapshot() const; Why is this public? https://codereview.chromium.org/2258133002/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc (right): https://codereview.chromium.org/2258133002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:166: (now - last_checkpoint_).InSecondsF()); perhaps just dump last_checkpoint_? https://codereview.chromium.org/2258133002/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h (right): https://codereview.chromium.org/2258133002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h:181: base::TimeTicks now) const; Why do we need a timestamp for something that is dumped to trace?
https://codereview.chromium.org/2258133002/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc (right): https://codereview.chromium.org/2258133002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:202: const char* tracing_category) Ouch -- just noticed this! Trace macros expect statically-allocated strings as categories and only do category lookup upon the first time the trace macro is hit. So the only reason this works as you expect is because you ever construct TaskQueueThrottler with one tracing_category. So please remove this parameter and corresponding member and just hard-code the category in trace macros. https://codereview.chromium.org/2258133002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:555: } else { style: we don't use else after return https://codereview.chromium.org/2258133002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:579: } else { ditto.
platform/BUILD.gn LGTM Add more explanation to the CL description so that others can follow what this CL is doing. Also consider splitting this kind of giant CL into pieces (move code from throtting_helper to task_queue_throtter + add the TimeBudgetPool API) from next time.
Nice, couple of small simplification suggestions. https://codereview.chromium.org/2258133002/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/base/test_task_time_observer.h (right): https://codereview.chromium.org/2258133002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/test_task_time_observer.h:18: double end_time) override{}; On 2016/09/22 00:52:34, caseq wrote: > style: add space before {} tip: remove the extra ';' so git cl format will do this for you automatically. https://codereview.chromium.org/2258133002/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc (right): https://codereview.chromium.org/2258133002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:34: } } // namespace https://codereview.chromium.org/2258133002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:63: if (metadata.IsThrottled() == 0) nit: !metadata.IsThrottled() https://codereview.chromium.org/2258133002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:95: for (TaskQueue* queue : associated_task_queues_) { Could we just call BlockQueues() here? https://codereview.chromium.org/2258133002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:147: if (is_enabled_) { nit: no {} https://codereview.chromium.org/2258133002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:202: const char* tracing_category) On 2016/09/22 01:25:26, caseq wrote: > Ouch -- just noticed this! Trace macros expect statically-allocated strings as > categories and only do category lookup upon the first time the trace macro is > hit. So the only reason this works as you expect is because you ever construct > TaskQueueThrottler with one tracing_category. So please remove this parameter > and corresponding member and just hard-code the category in trace macros. Good point. We might want to fix this separately since this pattern is not limited to this class (which already exists under a different name). In practice the categories are always the same and we pass pointers around to avoid duplicating the strings everywhere, but maybe we should just do that to make things more obvious. https://codereview.chromium.org/2258133002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:359: template <class T> Could you move these into the anonymous namespace at the top of this file? https://codereview.chromium.org/2258133002/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h (right): https://codereview.chromium.org/2258133002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h:112: void BlockQueues(base::TimeTicks now); nit: BlockThrottledQueues?
The CQ bit was checked by altimin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Sami, PTAL https://codereview.chromium.org/2258133002/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/base/test_task_time_observer.h (right): https://codereview.chromium.org/2258133002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/test_task_time_observer.h:18: double end_time) override{}; On 2016/09/22 00:52:34, caseq wrote: > style: add space before {} Done. https://codereview.chromium.org/2258133002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/test_task_time_observer.h:18: double end_time) override{}; On 2016/09/22 12:34:07, Sami wrote: > On 2016/09/22 00:52:34, caseq wrote: > > style: add space before {} > > tip: remove the extra ';' so git cl format will do this for you automatically. Done. https://codereview.chromium.org/2258133002/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/2258133002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:306: if (task_queue_throttler_.get()) On 2016/09/22 00:52:34, caseq wrote: > nit: get() is redundant here. Thanks! https://codereview.chromium.org/2258133002/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.h (right): https://codereview.chromium.org/2258133002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.h:20: #include "platform/scheduler/renderer/task_queue_throttler.h" On 2016/09/22 00:52:34, caseq wrote: > Is this necessary? If so, please remove TaskQueueThrottler forward declaration > below. Done. https://codereview.chromium.org/2258133002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.h:157: void CreateTraceEventObjectSnapshot() const; On 2016/09/22 00:52:34, caseq wrote: > Why is this public? It's public to scheduler/ because other components in scheduler can request a trace snapshot. https://codereview.chromium.org/2258133002/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc (right): https://codereview.chromium.org/2258133002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:34: } On 2016/09/22 12:34:07, Sami wrote: > } // namespace Done. https://codereview.chromium.org/2258133002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:63: if (metadata.IsThrottled() == 0) On 2016/09/22 12:34:07, Sami wrote: > nit: !metadata.IsThrottled() Ooops. Thanks! https://codereview.chromium.org/2258133002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:95: for (TaskQueue* queue : associated_task_queues_) { On 2016/09/22 12:34:07, Sami wrote: > Could we just call BlockQueues() here? Done. https://codereview.chromium.org/2258133002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:147: if (is_enabled_) { On 2016/09/22 12:34:07, Sami wrote: > nit: no {} Done. https://codereview.chromium.org/2258133002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:166: (now - last_checkpoint_).InSecondsF()); On 2016/09/22 00:52:34, caseq wrote: > perhaps just dump last_checkpoint_? I believe that delta is easier to read than random absolute time. https://codereview.chromium.org/2258133002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:202: const char* tracing_category) On 2016/09/22 12:34:07, Sami wrote: > On 2016/09/22 01:25:26, caseq wrote: > > Ouch -- just noticed this! Trace macros expect statically-allocated strings as > > categories and only do category lookup upon the first time the trace macro is > > hit. So the only reason this works as you expect is because you ever construct > > TaskQueueThrottler with one tracing_category. So please remove this parameter > > and corresponding member and just hard-code the category in trace macros. > > Good point. We might want to fix this separately since this pattern is not > limited to this class (which already exists under a different name). In practice > the categories are always the same and we pass pointers around to avoid > duplicating the strings everywhere, but maybe we should just do that to make > things more obvious. I'll add a TODO and address it in a next patch. https://codereview.chromium.org/2258133002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:359: template <class T> On 2016/09/22 12:34:08, Sami wrote: > Could you move these into the anonymous namespace at the top of this file? Done. https://codereview.chromium.org/2258133002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:555: } else { On 2016/09/22 01:25:26, caseq wrote: > style: we don't use else after return Done. https://codereview.chromium.org/2258133002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:579: } else { On 2016/09/22 01:25:26, caseq wrote: > ditto. Done. https://codereview.chromium.org/2258133002/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h (right): https://codereview.chromium.org/2258133002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h:112: void BlockQueues(base::TimeTicks now); On 2016/09/22 12:34:08, Sami wrote: > nit: BlockThrottledQueues? Done. https://codereview.chromium.org/2258133002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h:181: base::TimeTicks now) const; On 2016/09/22 00:52:34, caseq wrote: > Why do we need a timestamp for something that is dumped to trace? The most convenient way to write TimeTicks into trace is to convert them to delta from current moment. That's why we need |now| (passing |now| also prevents expensive calls to clock and ensures that there are now skews to due multiple calls to clock returning slightly different time).
lgtm, thank you. Please coordinate with Alex to make sure this collides in a controlled way with his DoWork patch.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by altimin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by altimin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by altimin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [scheduler] Implement time-based cpu throttling. Add TimeBudgetPool API to TaskQueueThrottler. BUG=639852 ========== to ========== [scheduler] Implement time-based cpu throttling. This patch adds TimeBudgetPool API to renderer scheduler, which allows to group task queues into pools and set shared limits on how long this pool of queues is allowed to run (as per cent of wall time). Queues violating this limit will be throttled. This patch also renames throttling_helper.cc to task_queue_throttler.cc. BUG=639852 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by altimin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, caseq@chromium.org, alexclarke@chromium.org, skyostil@chromium.org Link to the patchset: https://codereview.chromium.org/2258133002/#ps320001 (title: "Fix trybot failures (hopefully)")
The CQ bit was unchecked by altimin@chromium.org
The CQ bit was checked by altimin@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by altimin@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by altimin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, caseq@chromium.org, skyostil@chromium.org, alexclarke@chromium.org Link to the patchset: https://codereview.chromium.org/2258133002/#ps340001 (title: "Rebased")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by altimin@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [scheduler] Implement time-based cpu throttling. This patch adds TimeBudgetPool API to renderer scheduler, which allows to group task queues into pools and set shared limits on how long this pool of queues is allowed to run (as per cent of wall time). Queues violating this limit will be throttled. This patch also renames throttling_helper.cc to task_queue_throttler.cc. BUG=639852 ========== to ========== [scheduler] Implement time-based cpu throttling. This patch adds TimeBudgetPool API to renderer scheduler, which allows to group task queues into pools and set shared limits on how long this pool of queues is allowed to run (as per cent of wall time). Queues violating this limit will be throttled. This patch also renames throttling_helper.cc to task_queue_throttler.cc. BUG=639852 ==========
Message was sent while issue was closed.
Committed patchset #18 (id:340001)
Message was sent while issue was closed.
Description was changed from ========== [scheduler] Implement time-based cpu throttling. This patch adds TimeBudgetPool API to renderer scheduler, which allows to group task queues into pools and set shared limits on how long this pool of queues is allowed to run (as per cent of wall time). Queues violating this limit will be throttled. This patch also renames throttling_helper.cc to task_queue_throttler.cc. BUG=639852 ========== to ========== [scheduler] Implement time-based cpu throttling. This patch adds TimeBudgetPool API to renderer scheduler, which allows to group task queues into pools and set shared limits on how long this pool of queues is allowed to run (as per cent of wall time). Queues violating this limit will be throttled. This patch also renames throttling_helper.cc to task_queue_throttler.cc. BUG=639852 Committed: https://crrev.com/906d4a1e806b27681bad0ec63f64c0c9fc239a77 Cr-Commit-Position: refs/heads/master@{#420842} ==========
Message was sent while issue was closed.
Patchset 18 (id:??) landed as https://crrev.com/906d4a1e806b27681bad0ec63f64c0c9fc239a77 Cr-Commit-Position: refs/heads/master@{#420842}
Message was sent while issue was closed.
ojan@chromium.org changed reviewers: + ojan@chromium.org
Message was sent while issue was closed.
Drive-by after the fact comment: Please try to do these big changes in smaller patches. In addition to making it easier for reviewers, it makes it easier for people looking at these patches 3 years from now when they land on it during a blame. One simple way this patch could have been broken up is to do the class renames in a separate patch first and then the behavior changes in a followup patch.
Message was sent while issue was closed.
On 2016/10/05 17:37:34, ojan wrote: > Drive-by after the fact comment: Please try to do these big changes in smaller > patches. In addition to making it easier for reviewers, it makes it easier for > people looking at these patches 3 years from now when they land on it during a > blame. > > One simple way this patch could have been broken up is to do the class renames > in a separate patch first and then the behavior changes in a followup patch. +1, give that reviewer some small patches. Reviewers love small patches.
Message was sent while issue was closed.
On 2016/10/05 17:41:31, Sami wrote: > On 2016/10/05 17:37:34, ojan wrote: > > Drive-by after the fact comment: Please try to do these big changes in smaller > > patches. In addition to making it easier for reviewers, it makes it easier for > > people looking at these patches 3 years from now when they land on it during a > > blame. > > > > One simple way this patch could have been broken up is to do the class renames > > in a separate patch first and then the behavior changes in a followup patch. > > +1, give that reviewer some small patches. Reviewers love small patches. It was a small patch before the first round of comments! /s Seriously speaking, sorry, this got out of hand quickly. I'll try better next time.
Message was sent while issue was closed.
No worries. I just make a point of commenting everytime I see big patches in the hopes of maintaining a small patch culture. :) |
