Re: [scheduler] Add WakeupBudgetPool. (issue 2778123003 by altimin@chromium.org)

[alt...@chromium.org]

Reviewers: alex clarke, Sami
CL: https://codereview.chromium.org/2778123003/

Message:
PTAL.

(I'll add some more tests tomorrow)

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=skyo...@chromium.org,alexc...@chromium.org
BUG=699541

Affected files (+478, -77 lines):
M third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.h
M third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.cc
M third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.h
M third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.cc
M third_party/WebKit/Source/platform/scheduler/renderer/budget_pool_unittest.cc
M third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.h
M third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc
M third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h
M third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc
M third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler_unittest.cc
M third_party/WebKit/public/platform/scheduler/base/task_queue.h

[alexc...@chromium.org]

https://codereview.chromium.org/2778123003/diff/40001/third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.h
File third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.h
(right):

https://codereview.chromium.org/2778123003/diff/40001/third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.h#newcode265
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/Source/platform/scheduler/renderer/budget_pool.cc
File
third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.cc
(right):

https://codereview.chromium.org/2778123003/diff/40001/third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.cc#newcode75
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/Source/platform/scheduler/renderer/budget_pool.cc#newcode324
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/Source/platform/scheduler/renderer/budget_pool.cc#newcode330
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/Source/platform/scheduler/renderer/budget_pool.cc#newcode338
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/Source/platform/scheduler/renderer/budget_pool.cc#newcode354
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/Source/platform/scheduler/renderer/budget_pool.h
File third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.h
(right):

https://codereview.chromium.org/2778123003/diff/40001/third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.h#newcode44
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/Source/platform/scheduler/renderer/budget_pool.h#newcode74
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/Source/platform/scheduler/renderer/budget_pool.h#newcode220
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/Source/platform/scheduler/renderer/budget_pool.h#newcode225
third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.h:225:
~WakeupBudgetPool();
override?

https://codereview.chromium.org/2778123003/diff/40001/third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.h#newcode227
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/Source/platform/scheduler/renderer/budget_pool.h#newcode229
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/Source/platform/scheduler/renderer/budget_pool_unittest.cc
File
third_party/WebKit/Source/platform/scheduler/renderer/budget_pool_unittest.cc
(right):

https://codereview.chromium.org/2778123003/diff/40001/third_party/WebKit/Source/platform/scheduler/renderer/budget_pool_unittest.cc#newcode41
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/Source/platform/scheduler/renderer/budget_pool_unittest.cc#newcode49
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/Source/platform/scheduler/renderer/task_queue_throttler.cc
File
third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc
(right):

https://codereview.chromium.org/2778123003/diff/40001/third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc#newcode149
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/Source/platform/scheduler/renderer/task_queue_throttler.cc#newcode215
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/Source/platform/scheduler/renderer/task_queue_throttler.cc#newcode237
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/Source/platform/scheduler/renderer/task_queue_throttler.cc#newcode251
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/Source/platform/scheduler/renderer/task_queue_throttler.cc#newcode252
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/Source/platform/scheduler/renderer/task_queue_throttler.cc#newcode449
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/Source/platform/scheduler/renderer/task_queue_throttler.cc#newcode477
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/Source/platform/scheduler/renderer/task_queue_throttler.h
File
third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h
(right):

https://codereview.chromium.org/2778123003/diff/40001/third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h#newcode35
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/Source/platform/scheduler/renderer/task_queue_throttler.h#newcode35
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/public/platform/scheduler/base/task_queue.h
File third_party/WebKit/public/platform/scheduler/base/task_queue.h
(right):

https://codereview.chromium.org/2778123003/diff/40001/third_party/WebKit/public/platform/scheduler/base/task_queue.h#newcode225
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.

https://codereview.chromium.org/2778123003/

[alt...@chromium.org]

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/Source/platform/scheduler/base/task_queue_impl.h
File third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.h
(right):

https://codereview.chromium.org/2778123003/diff/40001/third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.h#newcode265
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/Source/platform/scheduler/renderer/budget_pool.cc
File
third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.cc
(right):

https://codereview.chromium.org/2778123003/diff/40001/third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.cc#newcode75
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/Source/platform/scheduler/renderer/budget_pool.cc#newcode324
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/Source/platform/scheduler/renderer/budget_pool.cc#newcode330
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/Source/platform/scheduler/renderer/budget_pool.cc#newcode338
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/Source/platform/scheduler/renderer/budget_pool.cc#newcode354
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/Source/platform/scheduler/renderer/budget_pool.h
File third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.h
(right):

https://codereview.chromium.org/2778123003/diff/40001/third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.h#newcode44
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/Source/platform/scheduler/renderer/budget_pool.h#newcode74
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/Source/platform/scheduler/renderer/budget_pool.h#newcode220
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/Source/platform/scheduler/renderer/budget_pool.h#newcode225
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/Source/platform/scheduler/renderer/budget_pool.h#newcode227
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/Source/platform/scheduler/renderer/budget_pool.h#newcode229
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/Source/platform/scheduler/renderer/budget_pool_unittest.cc
File
third_party/WebKit/Source/platform/scheduler/renderer/budget_pool_unittest.cc
(right):

https://codereview.chromium.org/2778123003/diff/40001/third_party/WebKit/Source/platform/scheduler/renderer/budget_pool_unittest.cc#newcode41
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/Source/platform/scheduler/renderer/budget_pool_unittest.cc#newcode49
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/Source/platform/scheduler/renderer/task_queue_throttler.cc
File
third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc
(right):

https://codereview.chromium.org/2778123003/diff/40001/third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc#newcode149
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/Source/platform/scheduler/renderer/task_queue_throttler.cc#newcode215
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/Source/platform/scheduler/renderer/task_queue_throttler.cc#newcode237
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/Source/platform/scheduler/renderer/task_queue_throttler.cc#newcode251
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/Source/platform/scheduler/renderer/task_queue_throttler.cc#newcode252
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/Source/platform/scheduler/renderer/task_queue_throttler.cc#newcode449
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/Source/platform/scheduler/renderer/task_queue_throttler.cc#newcode477
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/Source/platform/scheduler/renderer/task_queue_throttler.h
File
third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h
(right):

https://codereview.chromium.org/2778123003/diff/40001/third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h#newcode35
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/public/platform/scheduler/base/task_queue.h
File third_party/WebKit/public/platform/scheduler/base/task_queue.h
(right):

https://codereview.chromium.org/2778123003/diff/40001/third_party/WebKit/public/platform/scheduler/base/task_queue.h#newcode225
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.

https://codereview.chromium.org/2778123003/

[skyostil@chromium.org via codereview.chromium.org]

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/Source/platform/scheduler/renderer/budget_pool.cc
File
third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.cc
(right):

https://codereview.chromium.org/2778123003/diff/80001/third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.cc#newcode311
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/Source/platform/scheduler/renderer/budget_pool.cc#newcode316
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/Source/platform/scheduler/renderer/budget_pool.h
File third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.h
(right):

https://codereview.chromium.org/2778123003/diff/80001/third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.h#newcode49
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/Source/platform/scheduler/renderer/budget_pool.h#newcode77
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/Source/platform/scheduler/renderer/task_queue_throttler.cc
File
third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc
(right):

https://codereview.chromium.org/2778123003/diff/80001/third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc#newcode220
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/Source/platform/scheduler/renderer/task_queue_throttler.cc#newcode225
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/Source/platform/scheduler/renderer/task_queue_throttler.cc#newcode341
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/Source/platform/scheduler/renderer/task_queue_throttler.cc#newcode347
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/Source/platform/scheduler/renderer/task_queue_throttler.cc#newcode358
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/Source/platform/scheduler/renderer/task_queue_throttler.cc#newcode377
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/Source/platform/scheduler/renderer/task_queue_throttler.cc#newcode398
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/Source/platform/scheduler/renderer/task_queue_throttler.cc#newcode399
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/Source/platform/scheduler/renderer/task_queue_throttler_unittest.cc
File
third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler_unittest.cc
(right):

https://codereview.chromium.org/2778123003/diff/80001/third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler_unittest.cc#newcode1105
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/Source/platform/scheduler/renderer/task_queue_throttler_unittest.cc#newcode1139
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/

[alexc...@chromium.org]

https://codereview.chromium.org/2778123003/diff/80001/third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc
File
third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc
(right):

https://codereview.chromium.org/2778123003/diff/80001/third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc#newcode1901
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.

https://codereview.chromium.org/2778123003/

[alt...@chromium.org]

PTAL

https://codereview.chromium.org/2778123003/diff/80001/third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.cc
File
third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.cc
(right):

https://codereview.chromium.org/2778123003/diff/80001/third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.cc#newcode311
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/Source/platform/scheduler/renderer/budget_pool.cc#newcode316
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/Source/platform/scheduler/renderer/budget_pool.h
File third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.h
(right):

https://codereview.chromium.org/2778123003/diff/80001/third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.h#newcode49
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/Source/platform/scheduler/renderer/budget_pool.h#newcode77
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/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc
File
third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc
(right):

https://codereview.chromium.org/2778123003/diff/80001/third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc#newcode1901
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/Source/platform/scheduler/renderer/task_queue_throttler.cc
File
third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc
(right):

https://codereview.chromium.org/2778123003/diff/80001/third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc#newcode220
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/Source/platform/scheduler/renderer/task_queue_throttler.cc#newcode225
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/Source/platform/scheduler/renderer/task_queue_throttler.cc#newcode341
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/Source/platform/scheduler/renderer/task_queue_throttler.cc#newcode347
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/Source/platform/scheduler/renderer/task_queue_throttler.cc#newcode358
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/Source/platform/scheduler/renderer/task_queue_throttler.cc#newcode398
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/Source/platform/scheduler/renderer/task_queue_throttler.cc#newcode399
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/Source/platform/scheduler/renderer/task_queue_throttler_unittest.cc
File
third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler_unittest.cc
(right):

https://codereview.chromium.org/2778123003/diff/80001/third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler_unittest.cc#newcode1105
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/Source/platform/scheduler/renderer/task_queue_throttler_unittest.cc#newcode1139
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).

https://codereview.chromium.org/2778123003/

[alexc...@chromium.org]

https://codereview.chromium.org/2778123003/diff/40001/third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.h
File third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.h
(right):

https://codereview.chromium.org/2778123003/diff/40001/third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.h#newcode220
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/Source/platform/scheduler/base/task_queue_impl.cc
File
third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.cc
(right):

https://codereview.chromium.org/2778123003/diff/100001/third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.cc#newcode9
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/Source/platform/scheduler/renderer/budget_pool.cc
File
third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.cc
(right):

https://codereview.chromium.org/2778123003/diff/100001/third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.cc#newcode56
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/Source/platform/scheduler/renderer/budget_pool.cc#newcode208
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/Source/platform/scheduler/renderer/budget_pool.h
File third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.h
(right):

https://codereview.chromium.org/2778123003/diff/100001/third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.h#newcode112
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/Source/platform/scheduler/renderer/task_queue_throttler.cc
File
third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc
(right):

https://codereview.chromium.org/2778123003/diff/100001/third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc#newcode320
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?

https://codereview.chromium.org/2778123003/

[alt...@chromium.org]

PTAL

https://codereview.chromium.org/2778123003/diff/40001/third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.h
File third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.h
(right):

https://codereview.chromium.org/2778123003/diff/40001/third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.h#newcode220
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/Source/platform/scheduler/base/task_queue_impl.cc
File
third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.cc
(right):

https://codereview.chromium.org/2778123003/diff/100001/third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.cc#newcode9
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/Source/platform/scheduler/renderer/budget_pool.cc
File
third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.cc
(right):

https://codereview.chromium.org/2778123003/diff/100001/third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.cc#newcode56
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/Source/platform/scheduler/renderer/budget_pool.cc#newcode208
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/Source/platform/scheduler/renderer/budget_pool.h
File third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.h
(right):

https://codereview.chromium.org/2778123003/diff/100001/third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.h#newcode112
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/Source/platform/scheduler/renderer/task_queue_throttler.cc
File
third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc
(right):

https://codereview.chromium.org/2778123003/diff/100001/third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc#newcode320
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.

https://codereview.chromium.org/2778123003/

[skyostil@chromium.org via codereview.chromium.org]

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/Source/platform/scheduler/renderer/budget_pool.cc
File
third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.cc
(right):

https://codereview.chromium.org/2778123003/diff/80001/third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.cc#newcode316
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/Source/platform/scheduler/renderer/task_queue_throttler.cc
File
third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc
(right):

https://codereview.chromium.org/2778123003/diff/80001/third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc#newcode339
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/Source/platform/scheduler/renderer/task_queue_throttler.cc#newcode341
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/Source/platform/scheduler/renderer/task_queue_throttler.cc#newcode377
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/Source/platform/scheduler/renderer/task_queue_throttler.cc#newcode399
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/Source/platform/scheduler/renderer/task_queue_throttler_unittest.cc
File
third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler_unittest.cc
(right):

https://codereview.chromium.org/2778123003/diff/80001/third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler_unittest.cc#newcode1105
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.

https://codereview.chromium.org/2778123003/

[alt...@chromium.org]

https://codereview.chromium.org/2778123003/diff/80001/third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc
File
third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc
(right):

https://codereview.chromium.org/2778123003/diff/80001/third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc#newcode339
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/Source/platform/scheduler/renderer/task_queue_throttler.cc#newcode341
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/Source/platform/scheduler/renderer/task_queue_throttler.cc#newcode377
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.

https://codereview.chromium.org/2778123003/

[skyostil@chromium.org via codereview.chromium.org]

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/Source/platform/scheduler/renderer/cpu_time_budget_pool.cc
File
third_party/WebKit/Source/platform/scheduler/renderer/cpu_time_budget_pool.cc
(right):

https://codereview.chromium.org/2778123003/diff/120001/third_party/WebKit/Source/platform/scheduler/renderer/cpu_time_budget_pool.cc#newcode92
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/Source/platform/scheduler/renderer/throttled_time_domain.cc
File
third_party/WebKit/Source/platform/scheduler/renderer/throttled_time_domain.cc
(right):

https://codereview.chromium.org/2778123003/diff/120001/third_party/WebKit/Source/platform/scheduler/renderer/throttled_time_domain.cc#newcode30
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/

[alexc...@chromium.org]

https://codereview.chromium.org/2778123003/diff/40001/third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.h
File third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.h
(right):

https://codereview.chromium.org/2778123003/diff/40001/third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.h#newcode265
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/Source/platform/scheduler/renderer/task_queue_throttler.cc
File
third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc
(right):

https://codereview.chromium.org/2778123003/diff/120001/third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc#newcode343
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/Source/platform/scheduler/renderer/task_queue_throttler.cc#newcode371
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/Source/platform/scheduler/renderer/task_queue_throttler.cc#newcode375
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/Source/platform/scheduler/renderer/task_queue_throttler.cc#newcode388
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/Source/platform/scheduler/renderer/task_queue_throttler.cc#newcode394
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.

https://codereview.chromium.org/2778123003/

[alt...@chromium.org]

ptal

https://codereview.chromium.org/2778123003/diff/40001/third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.h
File third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.h
(right):

https://codereview.chromium.org/2778123003/diff/40001/third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.h#newcode265
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/Source/platform/scheduler/renderer/cpu_time_budget_pool.cc
File
third_party/WebKit/Source/platform/scheduler/renderer/cpu_time_budget_pool.cc
(right):

https://codereview.chromium.org/2778123003/diff/120001/third_party/WebKit/Source/platform/scheduler/renderer/cpu_time_budget_pool.cc#newcode92
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/Source/platform/scheduler/renderer/task_queue_throttler.cc
File
third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc
(right):

https://codereview.chromium.org/2778123003/diff/120001/third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc#newcode343
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/Source/platform/scheduler/renderer/task_queue_throttler.cc#newcode371
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/Source/platform/scheduler/renderer/task_queue_throttler.cc#newcode375
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/Source/platform/scheduler/renderer/throttled_time_domain.cc
File
third_party/WebKit/Source/platform/scheduler/renderer/throttled_time_domain.cc
(right):

https://codereview.chromium.org/2778123003/diff/120001/third_party/WebKit/Source/platform/scheduler/renderer/throttled_time_domain.cc#newcode30
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.

https://codereview.chromium.org/2778123003/

[skyostil@chromium.org via codereview.chromium.org]

Couple of more questions and then I promise I will stop :)


https://codereview.chromium.org/2778123003/diff/140001/third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.h
File third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.h
(right):

https://codereview.chromium.org/2778123003/diff/140001/third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.h#newcode49
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/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc
File
third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc
(right):

https://codereview.chromium.org/2778123003/diff/140001/third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc#newcode1891
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/Source/platform/scheduler/renderer/task_queue_throttler.cc
File
third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc
(right):

https://codereview.chromium.org/2778123003/diff/140001/third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc#newcode340
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/Source/platform/scheduler/renderer/wake_up_budget_pool.cc
File
third_party/WebKit/Source/platform/scheduler/renderer/wake_up_budget_pool.cc
(right):

https://codereview.chromium.org/2778123003/diff/140001/third_party/WebKit/Source/platform/scheduler/renderer/wake_up_budget_pool.cc#newcode66
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 :)

https://codereview.chromium.org/2778123003/

[alt...@chromium.org]

https://codereview.chromium.org/2778123003/diff/140001/third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.h
File third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.h
(right):

https://codereview.chromium.org/2778123003/diff/140001/third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.h#newcode49
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/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc
File
third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc
(right):

https://codereview.chromium.org/2778123003/diff/140001/third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc#newcode1891
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/Source/platform/scheduler/renderer/task_queue_throttler.cc
File
third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc
(right):

https://codereview.chromium.org/2778123003/diff/140001/third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc#newcode340
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/Source/platform/scheduler/renderer/wake_up_budget_pool.cc
File
third_party/WebKit/Source/platform/scheduler/renderer/wake_up_budget_pool.cc
(right):

https://codereview.chromium.org/2778123003/diff/140001/third_party/WebKit/Source/platform/scheduler/renderer/wake_up_budget_pool.cc#newcode66
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.

https://codereview.chromium.org/2778123003/

[skyostil@chromium.org via codereview.chromium.org]

lgtm, thanks! Alex I assume you're happy too?


https://codereview.chromium.org/2778123003/diff/160001/third_party/WebKit/Source/platform/scheduler/renderer/wake_up_budget_pool.cc
File
third_party/WebKit/Source/platform/scheduler/renderer/wake_up_budget_pool.cc
(right):

https://codereview.chromium.org/2778123003/diff/160001/third_party/WebKit/Source/platform/scheduler/renderer/wake_up_budget_pool.cc#newcode71
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.

https://codereview.chromium.org/2778123003/

[alexc...@chromium.org]

Yeah LGTM.

https://codereview.chromium.org/2778123003/

[alt...@chromium.org]

+vollick@ for platform/BUILD.gn

https://codereview.chromium.org/2778123003/

[alt...@chromium.org]

+jochen@, can you stamp platform/BUILD.gn please?

https://codereview.chromium.org/2778123003/

[joc...@chromium.org]

lgtm



https://codereview.chromium.org/2778123003/

[commit-bot@chromium.org via codereview.chromium.org]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2778123003/160001

https://codereview.chromium.org/2778123003/

[commit-bot@chromium.org via codereview.chromium.org]

Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/db65a6534a5e4ee5bdc0e5d31bdec2d95bc65ca9

https://codereview.chromium.org/2778123003/