Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2778123003:
[scheduler] Add WakeupBudgetPool. (Closed)
Description[scheduler] Add WakeupBudgetPool.
WakeupBudgetPool allows a short chain of fast continuation tasks to when
immediately even when throttled (old throttling mechanism introduced a
second delay between each pair).
Please note that full task queue throttler integration is still missing (task
queue throttler still inserts fences unconditionally) and will be added
in a later patch.
R=skyostil@chromium.org,alexclarke@chromium.org
BUG=699541
Review-Url: https://codereview.chromium.org/2778123003
Cr-Commit-Position: refs/heads/master@{#469736}
Committed: https://chromium.googlesource.com/chromium/src/+/db65a6534a5e4ee5bdc0e5d31bdec2d95bc65ca9
Patch Set 1 #Patch Set 2 : WIP #Patch Set 3 : First meaningful version #
Total comments: 51
Patch Set 4 : Rebased #Patch Set 5 : Second iteration. #
Total comments: 38
Patch Set 6 : addressed comments #
Total comments: 10
Patch Set 7 : Addressed comments from alexclarke@ #
Total comments: 12
Patch Set 8 : Rebased & addressed comments from skyostil@ and alexclarke@ #
Total comments: 8
Patch Set 9 : Addressed comments #
Total comments: 1
Messages
Total messages: 53 (30 generated)
I think my thought is he TimeDomain::Observer interface is wrong. The Throttler
is the only non test consumer and it's really not doing the Throttler any
favours.
Maybe the interface should be:
virtual void OnNextWakeUpChanged(
TaskQueue* task_queue,
base::TimeTicks next_wakeup) = 0;
Arguable that should be on TaskQueue not TimeDomain. Internally to the
throttler |next_wakeup| could be passed around.
PTAL. (I'll add some more tests tomorrow)
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 unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
https://codereview.chromium.org/2778123003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.h (right): https://codereview.chromium.org/2778123003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.h:265: base::Optional<EnqueueOrder> current_enqueue_order; This doesn't seem to be used? https://codereview.chromium.org/2778123003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.cc (right): https://codereview.chromium.org/2778123003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.cc:75: budget_pool_controller_->RemoveQueueFromBudgetPool(queue, this); Would it be confusing to call UnregisterQueue(queue) here? https://codereview.chromium.org/2778123003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.cc:324: // Subtract 1 microsecond to work with time alignment in task queue throttler. What happens if you don't do that? https://codereview.chromium.org/2778123003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.cc:330: if (now < last_wakeup_ + wakeup_duration_) Shouldn't that be <= ? https://codereview.chromium.org/2778123003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.cc:338: return now < last_wakeup_ + wakeup_duration_ && Ditto. https://codereview.chromium.org/2778123003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.cc:354: budget_pool_controller_->BlockQueue(GetBlockType(), now, queue); Maybe swap the order of these to avoid the negation? https://codereview.chromium.org/2778123003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.h (right): https://codereview.chromium.org/2778123003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.h:44: // Returns earliest time when the next pump can be scheduled to run new tasks. Returns the earliest... https://codereview.chromium.org/2778123003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.h:74: // is scheduled. Maybe comment this schedules a wake up if needed. https://codereview.chromium.org/2778123003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.h:220: class BLINK_PLATFORM_EXPORT WakeupBudgetPool : public BudgetPool { I think we should try to keep to 1 major class per .cc / .h file (I know we don't always do that, but files end up getting very long otherwise). https://codereview.chromium.org/2778123003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.h:225: ~WakeupBudgetPool(); override? https://codereview.chromium.org/2778123003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.h:227: void SetWakeupRate(base::TimeTicks now, double wakeups_per_second); Why do we need to pass |now| into this? I notice that there is a call to: GetMainThreadOnly().wake_up_budget_pool->SetWakeupRate(base::TimeTicks(), 1); I notice there already is OnWakeup(base::TimeTicks now) https://codereview.chromium.org/2778123003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.h:229: void SetWakeupDuration(base::TimeTicks now, base::TimeDelta duration); Same question. https://codereview.chromium.org/2778123003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/budget_pool_unittest.cc (right): https://codereview.chromium.org/2778123003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/budget_pool_unittest.cc:41: time_zero_ = clock_->NowTicks(); I think start_time_ would be a better name. https://codereview.chromium.org/2778123003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/budget_pool_unittest.cc:49: base::TimeTicks Milliseconds(int milliseconds) { I'd prefer if this and the function below had an AfterStart postfix. https://codereview.chromium.org/2778123003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc (right): https://codereview.chromium.org/2778123003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:149: if (iter->second.throttling_ref_count == 0) That looks really weird. Is this papering over a bug elsewhere? https://codereview.chromium.org/2778123003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:215: auto find_it = queue_details_.find(queue); I don't remember off hand if return value optimization kicks in here. Can this be const ref? (not needed if RVO happens). https://codereview.chromium.org/2778123003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:237: for (const auto& it : budget_pools_) uber nit: s/it/pair ;) https://codereview.chromium.org/2778123003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:251: // This task queue does not any tasks. Looks like you're missing a word in the comment. https://codereview.chromium.org/2778123003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:252: task_queue->InsertFence(TaskQueue::InsertFencePosition::NOW); Is it possible to get here? The queue has to be empty for NextTaskRunTime to return nullopt right? https://codereview.chromium.org/2778123003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:449: if (queue->HasFence()) I hope we don't get tempted to use fences for some other reason. Maybe add a DCHECK when adding a queue to check there is no pre-existing fence. https://codereview.chromium.org/2778123003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:477: base::TimeTicks now, I'm not 100% on this, but I wonder if things would be any simpler if we left the clamping to |now| up to the caller. https://codereview.chromium.org/2778123003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h (right): https://codereview.chromium.org/2778123003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h:35: enum class QueueBlockType { FULL, NEW_TASKS_ONLY }; These could do with some more documentation. I wonder if ALL_TASKS might make the call sites easier to understand. https://codereview.chromium.org/2778123003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h:35: enum class QueueBlockType { FULL, NEW_TASKS_ONLY }; Are you thinking of adding more blocking types? Currently there is a 1:1 mapping to fence type, do we need a new enum? (I'm not against adding this btw) https://codereview.chromium.org/2778123003/diff/40001/third_party/WebKit/publ... File third_party/WebKit/public/platform/scheduler/base/task_queue.h (right): https://codereview.chromium.org/2778123003/diff/40001/third_party/WebKit/publ... third_party/WebKit/public/platform/scheduler/base/task_queue.h:225: virtual bool BlockedByFence() const = 0; Please add: // Returns true if the queue has a fence which is blocking execution of tasks.
PTAL. Please note that due to the current mechanism we can't block immediate task (InsertFencePosition::NOW inserts fence after the task was scheduled). I plan to address it in a separate patch introducing InsertFencePosition::LAST_TASK). https://codereview.chromium.org/2778123003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.h (right): https://codereview.chromium.org/2778123003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.h:265: base::Optional<EnqueueOrder> current_enqueue_order; On 2017/04/21 09:14:18, alex clarke wrote: > This doesn't seem to be used? Thanks, removed. https://codereview.chromium.org/2778123003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.cc (right): https://codereview.chromium.org/2778123003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.cc:75: budget_pool_controller_->RemoveQueueFromBudgetPool(queue, this); On 2017/04/21 09:14:19, alex clarke wrote: > Would it be confusing to call UnregisterQueue(queue) here? I think yes. But given that I though about this too, let's introduce internal DissociateQueue method. https://codereview.chromium.org/2778123003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.cc:324: // Subtract 1 microsecond to work with time alignment in task queue throttler. On 2017/04/21 09:14:19, alex clarke wrote: > What happens if you don't do that? Alignment in PumpThrottledTasks happens. Improved comments to explain it. https://codereview.chromium.org/2778123003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.cc:330: if (now < last_wakeup_ + wakeup_duration_) On 2017/04/21 09:14:18, alex clarke wrote: > Shouldn't that be <= ? Added a comment. https://codereview.chromium.org/2778123003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.cc:338: return now < last_wakeup_ + wakeup_duration_ && On 2017/04/21 09:14:19, alex clarke wrote: > Ditto. Acknowledged. https://codereview.chromium.org/2778123003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.cc:354: budget_pool_controller_->BlockQueue(GetBlockType(), now, queue); On 2017/04/21 09:14:19, alex clarke wrote: > Maybe swap the order of these to avoid the negation? Done. https://codereview.chromium.org/2778123003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.h (right): https://codereview.chromium.org/2778123003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.h:44: // Returns earliest time when the next pump can be scheduled to run new tasks. On 2017/04/21 09:14:19, alex clarke wrote: > Returns the earliest... Done. https://codereview.chromium.org/2778123003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.h:74: // is scheduled. On 2017/04/21 09:14:19, alex clarke wrote: > Maybe comment this schedules a wake up if needed. Done. https://codereview.chromium.org/2778123003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.h:220: class BLINK_PLATFORM_EXPORT WakeupBudgetPool : public BudgetPool { On 2017/04/21 09:14:19, alex clarke wrote: > I think we should try to keep to 1 major class per .cc / .h file (I know we > don't always do that, but files end up getting very long otherwise). I was thinking that these classes are too similar and share a lot of code (in BudgetPool), so it makes sense to keep them here for now (but if we were to add one more budget pool here, I'd agree that we had to separate them). https://codereview.chromium.org/2778123003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.h:225: ~WakeupBudgetPool(); On 2017/04/21 09:14:19, alex clarke wrote: > override? Done. https://codereview.chromium.org/2778123003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.h:227: void SetWakeupRate(base::TimeTicks now, double wakeups_per_second); On 2017/04/21 09:14:19, alex clarke wrote: > Why do we need to pass |now| into this? I notice that there is a call to: > GetMainThreadOnly().wake_up_budget_pool->SetWakeupRate(base::TimeTicks(), 1); > > I notice there already is OnWakeup(base::TimeTicks now) Done. https://codereview.chromium.org/2778123003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.h:229: void SetWakeupDuration(base::TimeTicks now, base::TimeDelta duration); On 2017/04/21 09:14:19, alex clarke wrote: > Same question. Done. https://codereview.chromium.org/2778123003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/budget_pool_unittest.cc (right): https://codereview.chromium.org/2778123003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/budget_pool_unittest.cc:41: time_zero_ = clock_->NowTicks(); On 2017/04/21 09:14:19, alex clarke wrote: > I think start_time_ would be a better name. Done. https://codereview.chromium.org/2778123003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/budget_pool_unittest.cc:49: base::TimeTicks Milliseconds(int milliseconds) { On 2017/04/21 09:14:19, alex clarke wrote: > I'd prefer if this and the function below had an AfterStart postfix. Done. https://codereview.chromium.org/2778123003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc (right): https://codereview.chromium.org/2778123003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:149: if (iter->second.throttling_ref_count == 0) On 2017/04/21 09:14:19, alex clarke wrote: > That looks really weird. Is this papering over a bug elsewhere? There is a special test ensuring that extra DecreaseThrottleRefCount do nothing. I assumed that there are usages in the wild with this. I'm strongly in favour of RAII approach, where instead of Increase/Decrease ref count methods you can get a handle of sorts (instead of IncreaseThrottleRefCount) and reset/destroy it when needed (instead of DecreaseThrottleRefCount). I may do it a follow-up patch. https://codereview.chromium.org/2778123003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:215: auto find_it = queue_details_.find(queue); On 2017/04/21 09:14:19, alex clarke wrote: > I don't remember off hand if return value optimization kicks in here. Can this > be const ref? (not needed if RVO happens). auto find_it is a common pattern here. I don't want to refactor all usages in this patch (it's already too big). Maybe will address in a follow-up. https://codereview.chromium.org/2778123003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:237: for (const auto& it : budget_pools_) On 2017/04/21 09:14:19, alex clarke wrote: > uber nit: s/it/pair ;) Done. https://codereview.chromium.org/2778123003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:251: // This task queue does not any tasks. On 2017/04/21 09:14:19, alex clarke wrote: > Looks like you're missing a word in the comment. Done. https://codereview.chromium.org/2778123003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:252: task_queue->InsertFence(TaskQueue::InsertFencePosition::NOW); On 2017/04/21 09:14:19, alex clarke wrote: > Is it possible to get here? The queue has to be empty for NextTaskRunTime to > return nullopt right? Done. https://codereview.chromium.org/2778123003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:449: if (queue->HasFence()) On 2017/04/21 09:14:19, alex clarke wrote: > I hope we don't get tempted to use fences for some other reason. Maybe add a > DCHECK when adding a queue to check there is no pre-existing fence. Actually, we don't need this condition here. https://codereview.chromium.org/2778123003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:477: base::TimeTicks now, On 2017/04/21 09:14:19, alex clarke wrote: > I'm not 100% on this, but I wonder if things would be any simpler if we left the > clamping to |now| up to the caller. Agreed. https://codereview.chromium.org/2778123003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h (right): https://codereview.chromium.org/2778123003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h:35: enum class QueueBlockType { FULL, NEW_TASKS_ONLY }; On 2017/04/21 09:14:19, alex clarke wrote: > These could do with some more documentation. I wonder if ALL_TASKS might make > the call sites easier to understand. ALL_TASKS is a good idea, done. I hoped that it would be self-explanatory. https://codereview.chromium.org/2778123003/diff/40001/third_party/WebKit/publ... File third_party/WebKit/public/platform/scheduler/base/task_queue.h (right): https://codereview.chromium.org/2778123003/diff/40001/third_party/WebKit/publ... third_party/WebKit/public/platform/scheduler/base/task_queue.h:225: virtual bool BlockedByFence() const = 0; On 2017/04/21 09:14:19, alex clarke wrote: > Please add: // Returns true if the queue has a fence which is blocking > execution of tasks. Done.
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 unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The wake up logic seems good to me. Sorry for all the dumb questions below :) https://codereview.chromium.org/2778123003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.cc (right): https://codereview.chromium.org/2778123003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.cc:311: base::TimeDelta::FromMicroseconds(1); Would it be less arbitrary to subtract base::TimeDelta::FromInternalValue(1)? https://codereview.chromium.org/2778123003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.cc:316: if (!last_wakeup_) If we haven't had a wake up, should we allow everything to run? (same question below) https://codereview.chromium.org/2778123003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.h (right): https://codereview.chromium.org/2778123003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.h:49: // Returns true at a task can be run immediately at the given time. s/at/if/, also "immediately at the given time" seems contradictory, maybe drop the "immediately"? https://codereview.chromium.org/2778123003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.h:77: // become enabled immediately, but schedules a wakeup if needed. nit: "a wake up is scheduled if needed" (also let's be consistent about the spelling of "wake up" since we just fixed all instances? :) https://codereview.chromium.org/2778123003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc (right): https://codereview.chromium.org/2778123003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:220: budget_pool->OnTaskQueueHasWork(queue, now, next_wake_up); Could we also call this OnQueueNextWakeUpChanged because the semantic seems to be the same as this callback and I can't think of a reason why the name should be different? https://codereview.chromium.org/2778123003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:225: // TODO(altimin): Remove after moving to budget pools completely. Could you expand this comment? I'm not sure what it means. https://codereview.chromium.org/2778123003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:341: // to allow new tasks to run immediately. This is a little confusing: the next task doesn't want to run until |next_desired_run_time|, so why are we allowing new tasks to run immediately? Are we actually just allowing the next wake up to happen when removing the fence? https://codereview.chromium.org/2778123003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:347: time_domain_->SetNextTask(next_desired_run_time.value()); SetNextTaskRunTime? (also, why doesn't this just schedule a normal wake up in the time domain?) https://codereview.chromium.org/2778123003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:358: base::Optional<base::TimeTicks> next_wake_up = What's the difference between next_desired_run_time and next_wake_up? https://codereview.chromium.org/2778123003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:377: DCHECK(block_type); nit: DCHECK before actually using |block_type| https://codereview.chromium.org/2778123003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:398: // GetQueueBlockType() == QueueBlockType::kNewTasksOnly DCHECK? https://codereview.chromium.org/2778123003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:399: has_new_tasks_only_block = true; break? https://codereview.chromium.org/2778123003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler_unittest.cc (right): https://codereview.chromium.org/2778123003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler_unittest.cc:1105: base::Bind(&RunChainedTask, 10, timer_queue_, clock_.get(), Should we make this 10 larger to check that we only run the allowed set of tasks? https://codereview.chromium.org/2778123003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler_unittest.cc:1139: // TODO(altimin): Add fence mechanism to block immediate tasks. Just curious, how do interleaved immediate tasks work in the current design? They just run immediately?
https://codereview.chromium.org/2778123003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/2778123003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:1901: task_queue_throttler()->CreateWakeUpBudgetPool("renderer-wake-up-pool"); I wonder if we should use a builder for WakeUpBudgetPool? It feels odd having these two methods which are only supposed to be called during initialisation.
PTAL https://codereview.chromium.org/2778123003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.cc (right): https://codereview.chromium.org/2778123003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.cc:311: base::TimeDelta::FromMicroseconds(1); On 2017/04/26 13:09:08, Sami wrote: > Would it be less arbitrary to subtract base::TimeDelta::FromInternalValue(1)? The current approach is easier to test. https://codereview.chromium.org/2778123003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.cc:316: if (!last_wakeup_) On 2017/04/26 13:09:08, Sami wrote: > If we haven't had a wake up, should we allow everything to run? (same question > below) No, if we're throttled we should wait for a wake-up. In the beginning, when a task queue is throttled, wake-up is a scheduled and a queue should wait for the wake-up. https://codereview.chromium.org/2778123003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.h (right): https://codereview.chromium.org/2778123003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.h:49: // Returns true at a task can be run immediately at the given time. On 2017/04/26 13:09:08, Sami wrote: > s/at/if/, also "immediately at the given time" seems contradictory, maybe drop > the "immediately"? Done. https://codereview.chromium.org/2778123003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.h:77: // become enabled immediately, but schedules a wakeup if needed. On 2017/04/26 13:09:08, Sami wrote: > nit: "a wake up is scheduled if needed" (also let's be consistent about the > spelling of "wake up" since we just fixed all instances? :) I'm thinking about a presubmit check actually... https://codereview.chromium.org/2778123003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/2778123003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:1901: task_queue_throttler()->CreateWakeUpBudgetPool("renderer-wake-up-pool"); On 2017/04/26 13:31:55, alex clarke wrote: > I wonder if we should use a builder for WakeUpBudgetPool? It feels odd having > these two methods which are only supposed to be called during initialisation. I'd like to have an ability to change wakeup rate and wake-up duration dynamically. With new UpdateQueueThrottlingState this will be very easy to do, actually. https://codereview.chromium.org/2778123003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc (right): https://codereview.chromium.org/2778123003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:220: budget_pool->OnTaskQueueHasWork(queue, now, next_wake_up); On 2017/04/26 13:09:08, Sami wrote: > Could we also call this OnQueueNextWakeUpChanged because the semantic seems to > be the same as this callback and I can't think of a reason why the name should > be different? Done. https://codereview.chromium.org/2778123003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:225: // TODO(altimin): Remove after moving to budget pools completely. On 2017/04/26 13:09:08, Sami wrote: > Could you expand this comment? I'm not sure what it means. Done. https://codereview.chromium.org/2778123003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:341: // to allow new tasks to run immediately. On 2017/04/26 13:09:08, Sami wrote: > This is a little confusing: the next task doesn't want to run until > |next_desired_run_time|, so why are we allowing new tasks to run immediately? > Are we actually just allowing the next wake up to happen when removing the > fence? Yes. Also there can be new tasks coming from different task queues, which should be run if they are inside the wake-up window. https://codereview.chromium.org/2778123003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:347: time_domain_->SetNextTask(next_desired_run_time.value()); On 2017/04/26 13:09:08, Sami wrote: > SetNextTaskRunTime? (also, why doesn't this just schedule a normal wake up in > the time domain?) ThrottledTimeDomain does not schedule wake-ups (do prevent unnecessary ones). https://codereview.chromium.org/2778123003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:358: base::Optional<base::TimeTicks> next_wake_up = On 2017/04/26 13:09:08, Sami wrote: > What's the difference between next_desired_run_time and next_wake_up? next_desired_run_time takes into account tasks that can run now, next_wake_up tells us when we should wake up to enqueue a new delayed task. https://codereview.chromium.org/2778123003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:398: // GetQueueBlockType() == QueueBlockType::kNewTasksOnly On 2017/04/26 13:09:08, Sami wrote: > DCHECK? Done. https://codereview.chromium.org/2778123003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:399: has_new_tasks_only_block = true; On 2017/04/26 13:09:08, Sami wrote: > break? No, we can still encounter QueueBlockType::kAllTasks and this takes precedence. https://codereview.chromium.org/2778123003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler_unittest.cc (right): https://codereview.chromium.org/2778123003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler_unittest.cc:1105: base::Bind(&RunChainedTask, 10, timer_queue_, clock_.get(), On 2017/04/26 13:09:08, Sami wrote: > Should we make this 10 larger to check that we only run the allowed set of > tasks? I'm not quite sure what you mean here. There tasks take 0 time and will not be throttled. See the next test if it is the thing you want. https://codereview.chromium.org/2778123003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler_unittest.cc:1139: // TODO(altimin): Add fence mechanism to block immediate tasks. On 2017/04/26 13:09:08, Sami wrote: > Just curious, how do interleaved immediate tasks work in the current design? > They just run immediately? The fence is installed, but it is too late, so one immediate task can run (at 1012 and 2012 milliseconds in this example).
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 unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2778123003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.h (right): https://codereview.chromium.org/2778123003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.h:220: class BLINK_PLATFORM_EXPORT WakeupBudgetPool : public BudgetPool { On 2017/04/25 13:22:35, altimin wrote: > On 2017/04/21 09:14:19, alex clarke wrote: > > I think we should try to keep to 1 major class per .cc / .h file (I know we > > don't always do that, but files end up getting very long otherwise). > > I was thinking that these classes are too similar and share a lot of code (in > BudgetPool), so it makes sense to keep them here for now (but if we were to add > one more budget pool here, I'd agree that we had to separate them). Sorry to be a pain but I think we should separate this. Smaller files are good. https://codereview.chromium.org/2778123003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.cc (right): https://codereview.chromium.org/2778123003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.cc:9: #include <iostream> // FIXME Lets remove this :) https://codereview.chromium.org/2778123003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.cc (right): https://codereview.chromium.org/2778123003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.cc:56: void BudgetPool::UnregisterQueue(TaskQueue* queue) { I wonder if this and DissociateQueue and TaskQueueThrottler::RemoveQueueFromBudgetPool should be made to take a const TaskQueue in a follow on patch. https://codereview.chromium.org/2778123003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.cc:208: if (current_budget_level_.InSecondsF() < 0) { nit: Elsewhere we tend not to use curly braces for single line ifs. https://codereview.chromium.org/2778123003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.h (right): https://codereview.chromium.org/2778123003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.h:112: void DissociateQueue(TaskQueue* queue); Why do we need this and UnregisterQueue? https://codereview.chromium.org/2778123003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc (right): https://codereview.chromium.org/2778123003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:320: void TaskQueueThrottler::UpdateQueueThrottlingStateInternal(base::TimeTicks now, Should this function emit a TaskQueueThrottler::PumpThrottledTasks_ExpensiveTaskThrottled trace?
PTAL https://codereview.chromium.org/2778123003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.h (right): https://codereview.chromium.org/2778123003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.h:220: class BLINK_PLATFORM_EXPORT WakeupBudgetPool : public BudgetPool { On 2017/04/27 10:28:14, alex clarke wrote: > On 2017/04/25 13:22:35, altimin wrote: > > On 2017/04/21 09:14:19, alex clarke wrote: > > > I think we should try to keep to 1 major class per .cc / .h file (I know we > > > don't always do that, but files end up getting very long otherwise). > > > > I was thinking that these classes are too similar and share a lot of code (in > > BudgetPool), so it makes sense to keep them here for now (but if we were to > add > > one more budget pool here, I'd agree that we had to separate them). > > Sorry to be a pain but I think we should separate this. Smaller files are good. Done. https://codereview.chromium.org/2778123003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.cc (right): https://codereview.chromium.org/2778123003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.cc:9: #include <iostream> // FIXME On 2017/04/27 10:28:14, alex clarke wrote: > Lets remove this :) Done. https://codereview.chromium.org/2778123003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.cc (right): https://codereview.chromium.org/2778123003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.cc:56: void BudgetPool::UnregisterQueue(TaskQueue* queue) { On 2017/04/27 10:28:14, alex clarke wrote: > I wonder if this and DissociateQueue and > TaskQueueThrottler::RemoveQueueFromBudgetPool should be made to take a const > TaskQueue in a follow on patch. I'll look into it, but it may be non-trivial due to map with non-const TaskQueues. https://codereview.chromium.org/2778123003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.cc:208: if (current_budget_level_.InSecondsF() < 0) { On 2017/04/27 10:28:14, alex clarke wrote: > nit: Elsewhere we tend not to use curly braces for single line ifs. Done. https://codereview.chromium.org/2778123003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.h (right): https://codereview.chromium.org/2778123003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.h:112: void DissociateQueue(TaskQueue* queue); On 2017/04/27 10:28:14, alex clarke wrote: > Why do we need this and UnregisterQueue? > I believe that it's confusing to call UnregisterQueue from RemoveQueue, so I moved out the common code from UnregisterQueue and RemoveQueue to a separate function (and at this point common code is the same as UnregisterQueue itself, yes). https://codereview.chromium.org/2778123003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc (right): https://codereview.chromium.org/2778123003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:320: void TaskQueueThrottler::UpdateQueueThrottlingStateInternal(base::TimeTicks now, On 2017/04/27 10:28:14, alex clarke wrote: > Should this function emit a > TaskQueueThrottler::PumpThrottledTasks_ExpensiveTaskThrottled trace? Done.
Thanks, couple of more questions below. Maybe I just need some ASCII art or a quick whiteboard session :) https://codereview.chromium.org/2778123003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.cc (right): https://codereview.chromium.org/2778123003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.cc:316: if (!last_wakeup_) On 2017/04/26 14:31:29, altimin wrote: > On 2017/04/26 13:09:08, Sami wrote: > > If we haven't had a wake up, should we allow everything to run? (same question > > below) > > No, if we're throttled we should wait for a wake-up. In the beginning, when a > task queue is throttled, wake-up is a scheduled and a queue should wait for the > wake-up. Okay, could you add a comment about that here please? https://codereview.chromium.org/2778123003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc (right): https://codereview.chromium.org/2778123003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:339: if (CanRunTasksUntil(queue, now, next_desired_run_time.value())) { Reading this more, I guess I'm still a bit confused about the three different cases here. Maybe it would be more natural to set a fence for the point (in time) at which we want to stop running new tasks? Isn't that effectively what we're doing here? https://codereview.chromium.org/2778123003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:341: // to allow new tasks to run immediately. On 2017/04/26 14:31:29, altimin wrote: > On 2017/04/26 13:09:08, Sami wrote: > > This is a little confusing: the next task doesn't want to run until > > |next_desired_run_time|, so why are we allowing new tasks to run immediately? > > Are we actually just allowing the next wake up to happen when removing the > > fence? > > Yes. Also there can be new tasks coming from different task queues, which should > be run if they are inside the wake-up window. I see. I wonder if a fence with a timestamp would be a cleaner way of solving this problem? Maybe we don't need to do that right now though. https://codereview.chromium.org/2778123003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:377: DCHECK(block_type); On 2017/04/26 13:09:08, Sami wrote: > nit: DCHECK before actually using |block_type| Missed this? https://codereview.chromium.org/2778123003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:399: has_new_tasks_only_block = true; On 2017/04/26 14:31:29, altimin wrote: > On 2017/04/26 13:09:08, Sami wrote: > > break? > > No, we can still encounter QueueBlockType::kAllTasks and this takes precedence. Ah got it. https://codereview.chromium.org/2778123003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler_unittest.cc (right): https://codereview.chromium.org/2778123003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler_unittest.cc:1105: base::Bind(&RunChainedTask, 10, timer_queue_, clock_.get(), On 2017/04/26 14:31:30, altimin wrote: > On 2017/04/26 13:09:08, Sami wrote: > > Should we make this 10 larger to check that we only run the allowed set of > > tasks? > > I'm not quite sure what you mean here. There tasks take 0 time and will not be > throttled. See the next test if it is the thing you want. Ah yes, you're right.
The CQ bit was checked by altimin@chromium.org to run a CQ dry run
https://codereview.chromium.org/2778123003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc (right): https://codereview.chromium.org/2778123003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:339: if (CanRunTasksUntil(queue, now, next_desired_run_time.value())) { On 2017/04/27 12:12:30, Sami wrote: > Reading this more, I guess I'm still a bit confused about the three different > cases here. Maybe it would be more natural to set a fence for the point (in > time) at which we want to stop running new tasks? Isn't that effectively what > we're doing here? Yes, but it will be at least as complex as fence is enqueue-order based, not timestamp-based. We will have the same problem with delayed tasks and ensuring that fence blocks them but does not block immediate tasks (or delayed tasks coming before the fence). https://codereview.chromium.org/2778123003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:341: // to allow new tasks to run immediately. On 2017/04/27 12:12:30, Sami wrote: > On 2017/04/26 14:31:29, altimin wrote: > > On 2017/04/26 13:09:08, Sami wrote: > > > This is a little confusing: the next task doesn't want to run until > > > |next_desired_run_time|, so why are we allowing new tasks to run > immediately? > > > Are we actually just allowing the next wake up to happen when removing the > > > fence? > > > > Yes. Also there can be new tasks coming from different task queues, which > should > > be run if they are inside the wake-up window. > > I see. I wonder if a fence with a timestamp would be a cleaner way of solving > this problem? Maybe we don't need to do that right now though. See comment above. https://codereview.chromium.org/2778123003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:377: DCHECK(block_type); On 2017/04/27 12:12:30, Sami wrote: > On 2017/04/26 13:09:08, Sami wrote: > > nit: DCHECK before actually using |block_type| > > Missed this? Done, forgot to reply.
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 unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
So it sounded like you guys converged on this approach after all? I think lg2me now with some nits, but Alex please approve too. https://codereview.chromium.org/2778123003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/renderer/cpu_time_budget_pool.cc (right): https://codereview.chromium.org/2778123003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/cpu_time_budget_pool.cc:92: return CanRunTasksAt(now, false); I think this is slightly misleading: I would expect CanRanTasksUntil to return true until the point at which the budget becomes negative. To do that we would need to look at |moment| which we're not doing here. Should we do that? https://codereview.chromium.org/2778123003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/renderer/throttled_time_domain.cc (right): https://codereview.chromium.org/2778123003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/throttled_time_domain.cc:30: next_task_ = run_time; nit: next_task_run_time_
https://codereview.chromium.org/2778123003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.h (right): https://codereview.chromium.org/2778123003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.h:265: base::Optional<EnqueueOrder> current_enqueue_order; On 2017/04/25 13:22:34, altimin wrote: > On 2017/04/21 09:14:18, alex clarke wrote: > > This doesn't seem to be used? > > Thanks, removed. It's still there? :) https://codereview.chromium.org/2778123003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc (right): https://codereview.chromium.org/2778123003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:343: queue->RemoveFence(); Can we DCHECK(queue->HasFence()) or is that going to fail for newly added queues? https://codereview.chromium.org/2778123003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:371: // Ensure that correct type of a fence is blocking queue which can't run. Could you please re-word this comment? It doesn't make much sense to me. https://codereview.chromium.org/2778123003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:375: if (block_type == QueueBlockType::kAllTasks) { Are you planning on adding more QueueBlockTypes? If so then this should be a switch because you'll get a compile error if you forget to handle the new type here. https://codereview.chromium.org/2778123003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:388: } So just to make sure I understad, if block_type == QueueBlockType::kNewTasksOnly and there already is a fence we don't need to do anything. Does it matter of the pre-existing fence is InsertFencePosition::BEGINNING_OF_TIME? https://codereview.chromium.org/2778123003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:394: base::Optional<QueueBlockType> TaskQueueThrottler::GetQueueBlockType( Do we need this function? Could we instead expose QueueBlockType via TaskQueueThrottler::CanRunTasksAt. They seem to do a lot of the same work.
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...
ptal https://codereview.chromium.org/2778123003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.h (right): https://codereview.chromium.org/2778123003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.h:265: base::Optional<EnqueueOrder> current_enqueue_order; On 2017/05/02 10:51:54, alex clarke wrote: > On 2017/04/25 13:22:34, altimin wrote: > > On 2017/04/21 09:14:18, alex clarke wrote: > > > This doesn't seem to be used? > > > > Thanks, removed. > > It's still there? :) Finally gone. https://codereview.chromium.org/2778123003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/renderer/cpu_time_budget_pool.cc (right): https://codereview.chromium.org/2778123003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/cpu_time_budget_pool.cc:92: return CanRunTasksAt(now, false); On 2017/04/29 17:43:02, Sami wrote: > I think this is slightly misleading: I would expect CanRanTasksUntil to return > true until the point at which the budget becomes negative. To do that we would > need to look at |moment| which we're not doing here. Should we do that? Scrapped CanRunTasksUntil completely. https://codereview.chromium.org/2778123003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc (right): https://codereview.chromium.org/2778123003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:343: queue->RemoveFence(); On 2017/05/02 10:51:54, alex clarke wrote: > Can we DCHECK(queue->HasFence()) or is that going to fail for newly added > queues? It is going to fail when the fence is removed and new work is coming from a different thread during this period. https://codereview.chromium.org/2778123003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:371: // Ensure that correct type of a fence is blocking queue which can't run. On 2017/05/02 10:51:54, alex clarke wrote: > Could you please re-word this comment? It doesn't make much sense to me. Done. https://codereview.chromium.org/2778123003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:375: if (block_type == QueueBlockType::kAllTasks) { On 2017/05/02 10:51:54, alex clarke wrote: > Are you planning on adding more QueueBlockTypes? If so then this should be a > switch because you'll get a compile error if you forget to handle the new type > here. Agreed. https://codereview.chromium.org/2778123003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/renderer/throttled_time_domain.cc (right): https://codereview.chromium.org/2778123003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/throttled_time_domain.cc:30: next_task_ = run_time; On 2017/04/29 17:43:02, Sami wrote: > nit: next_task_run_time_ Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Couple of more questions and then I promise I will stop :) https://codereview.chromium.org/2778123003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.h (right): https://codereview.chromium.org/2778123003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.h:49: virtual bool CanRunTasksAt(base::TimeTicks moment, bool is_wake_up) const = 0; The various implementations call the first parameter |now| which is a little misleading. Could you please make sure they all say |moment|? https://codereview.chromium.org/2778123003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/2778123003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:1891: task_queue_throttler()->CreateWakeUpBudgetPool("renderer-wake-up-pool"); uber-nit: let's use underscores instead of hyphens since I think we name most other things like that. https://codereview.chromium.org/2778123003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc (right): https://codereview.chromium.org/2778123003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:340: // We can run up until the next task uninterrupted. Remove the fence Wondering about the wording here ("uninterrupted") -- if the next wake up is far in the future, then we might well run out of budget before we make it that far if we run tasks uninterrupted. I guess CanRunTasksAt() assumes no other tasks will appear after it was called? Maybe CouldRunTasksAt() would convey this uncertainty better? https://codereview.chromium.org/2778123003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/renderer/wake_up_budget_pool.cc (right): https://codereview.chromium.org/2778123003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/wake_up_budget_pool.cc:66: if (!last_wakeup_) Is it fair to say the policy is that the current wake up is allowed to continue indefinitely, but new wake ups are only allowed within the wake-up window? If so, please add a comment :)
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/2778123003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.h (right): https://codereview.chromium.org/2778123003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.h:49: virtual bool CanRunTasksAt(base::TimeTicks moment, bool is_wake_up) const = 0; On 2017/05/03 16:53:05, Sami wrote: > The various implementations call the first parameter |now| which is a little > misleading. Could you please make sure they all say |moment|? That's the problem with iterative development: old iterations tend to pop up in unexpected places! Done. https://codereview.chromium.org/2778123003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/2778123003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:1891: task_queue_throttler()->CreateWakeUpBudgetPool("renderer-wake-up-pool"); On 2017/05/03 16:53:05, Sami wrote: > uber-nit: let's use underscores instead of hyphens since I think we name most > other things like that. Done. https://codereview.chromium.org/2778123003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc (right): https://codereview.chromium.org/2778123003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:340: // We can run up until the next task uninterrupted. Remove the fence On 2017/05/03 16:53:05, Sami wrote: > Wondering about the wording here ("uninterrupted") -- if the next wake up is far > in the future, then we might well run out of budget before we make it that far > if we run tasks uninterrupted. I guess CanRunTasksAt() assumes no other tasks > will appear after it was called? Maybe CouldRunTasksAt() would convey this > uncertainty better? Made the comment more explicit. https://codereview.chromium.org/2778123003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/renderer/wake_up_budget_pool.cc (right): https://codereview.chromium.org/2778123003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/wake_up_budget_pool.cc:66: if (!last_wakeup_) On 2017/05/03 16:53:05, Sami wrote: > Is it fair to say the policy is that the current wake up is allowed to continue > indefinitely, but new wake ups are only allowed within the wake-up window? If > so, please add a comment :) Added a comment.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm, thanks! Alex I assume you're happy too? https://codereview.chromium.org/2778123003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/renderer/wake_up_budget_pool.cc (right): https://codereview.chromium.org/2778123003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/wake_up_budget_pool.cc:71: // and allow only one task to run. We're not allowing just one task to run, but all tasks which were eligible to run at the beginning of the wake-up, right? That's the old behavior.
Yeah LGTM.
altimin@chromium.org changed reviewers: + vollick@chromium.org
+vollick@ for platform/BUILD.gn
altimin@chromium.org changed reviewers: + jochen@chromium.org
+jochen@, can you stamp platform/BUILD.gn please?
lgtm
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...
CQ is committing da patch.
Bot data: {"patchset_id": 160001, "attempt_start_ts": 1494002837235870,
"parent_rev": "c58cdb27a355fff2a49368760d3b7100d8e73df6", "commit_rev":
"76f72f35b579b9c190cd796b1487385f3effd73f"}
CQ is committing da patch.
Bot data: {"patchset_id": 160001, "attempt_start_ts": 1494002837235870,
"parent_rev": "63979b76bb24d1e5d3407ca8f469bc8ab99002ed", "commit_rev":
"db65a6534a5e4ee5bdc0e5d31bdec2d95bc65ca9"}
Message was sent while issue was closed.
Description was changed from ========== [scheduler] Add WakeupBudgetPool. WakeupBudgetPool allows a short chain of fast continuation tasks to when immediately even when throttled (old throttling mechanism introduced a second delay between each pair). Please note that full task queue throttler integration is still missing (task queue throttler still inserts fences unconditionally) and will be added in a later patch. R=skyostil@chromium.org,alexclarke@chromium.org BUG=699541 ========== to ========== [scheduler] Add WakeupBudgetPool. WakeupBudgetPool allows a short chain of fast continuation tasks to when immediately even when throttled (old throttling mechanism introduced a second delay between each pair). Please note that full task queue throttler integration is still missing (task queue throttler still inserts fences unconditionally) and will be added in a later patch. R=skyostil@chromium.org,alexclarke@chromium.org BUG=699541 Review-Url: https://codereview.chromium.org/2778123003 Cr-Commit-Position: refs/heads/master@{#469736} Committed: https://chromium.googlesource.com/chromium/src/+/db65a6534a5e4ee5bdc0e5d31bde... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://chromium.googlesource.com/chromium/src/+/db65a6534a5e4ee5bdc0e5d31bde... |
