Skip to content

GitLab

T
tangled
Commit 04e1e5ca authored by kbr's avatar kbr Committed by Commit bot
Browse files
Options

Revert of Prevent redundant DoWorks due to canceled delayed tasks (patchset... 

Revert of Prevent redundant DoWorks due to canceled delayed tasks  (patchset #6 id:100001 of https://codereview.chromium.org/2320403003/ )

Reason for revert:
Suspect that this CL caused breakage in Gmail and Hangouts per http://crbug.com/647484 .

Original issue's description:
> Prevent redundant DoWorks due to canceled delayed tasks
>
> To achieve this we make a few changes:
>
> 1. We only register the next wakeup with the TimeDomain, rather than all
> of them.
> 2. MoveReadyDelayedTasksToDelayedWorkQueue now registers the next
> wakeup (if any).  Since it removes all canceled delayed tasks from the
> front of the priority queue this has the effect of not scheduling
> wakeups for cancelled tasks.
> 3. Tweaking the TaskQueueManager level delayed DoWork de-duplication
> logic to only post a delayed DoWork if the task is meant to run before
> any previously registered delayed DoWorks.
>
> BUG=638542, 605718
>
> Committed: https://crrev.com/929cbb9f92b5570867c3842c80778243db81a013
> Cr-Commit-Position: refs/heads/master@{#418556}

TBR=skyostil@chromium.org,alexclarke@chromium.org
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG=638542, 605718

Review-Url: https://codereview.chromium.org/2353473003
Cr-Commit-Position: refs/heads/master@{#419542}
parent 03b81153
Hide whitespace changes
Inline Side-by-side
Showing with 27 additions and 226 deletions
third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.cc
View file @ 04e1e5ca
......@@ -217,25 +217,11 @@ void TaskQueueImpl::PushOntoDelayedIncomingQueueFromMainThread(
Task pending_task, base::TimeTicks now) {
main_thread_only().task_queue_manager->DidQueueTask(pending_task);
// Schedule a later call to MoveReadyDelayedTasksToDelayedWorkQueue.
base::TimeTicks delayed_run_time = pending_task.delayed_run_time;
bool queue_was_empty = main_thread_only().delayed_incoming_queue.empty();
base::TimeTicks previous_next_run_time;
if (!queue_was_empty) {
previous_next_run_time =
main_thread_only().delayed_incoming_queue.top().delayed_run_time;
}
main_thread_only().delayed_incoming_queue.push(std::move(pending_task));
// Make sure the next wakeup (and only the next wakeup) is registered with the
// TimeDomain.
if (queue_was_empty || delayed_run_time < previous_next_run_time) {
main_thread_only().time_domain->ScheduleDelayedWork(this, delayed_run_time,
now);
if (!queue_was_empty) {
main_thread_only().time_domain->CancelDelayedWork(this,
previous_next_run_time);
}
}
main_thread_only().time_domain->ScheduleDelayedWork(this, delayed_run_time,
now);
TraceQueueSize(false);
}
......@@ -254,11 +240,11 @@ void TaskQueueImpl::PushOntoDelayedIncomingQueueLocked(Task pending_task) {
}
void TaskQueueImpl::PushOntoImmediateIncomingQueueLocked(
const tracked_objects::Location& posted_from,
const base::Closure& task,
base::TimeTicks desired_run_time,
EnqueueOrder sequence_number,
bool nestable) {
const tracked_objects::Location& posted_from,
const base::Closure& task,
base::TimeTicks desired_run_time,
EnqueueOrder sequence_number,
bool nestable) {
if (any_thread().immediate_incoming_queue.empty())
any_thread().time_domain->RegisterAsUpdatableTaskQueue(this);
// If the |immediate_incoming_queue| is empty we need a DoWork posted to make
......@@ -283,19 +269,17 @@ void TaskQueueImpl::ScheduleDelayedWorkTask(Task pending_task) {
DCHECK(main_thread_checker_.CalledOnValidThread());
base::TimeTicks delayed_run_time = pending_task.delayed_run_time;
base::TimeTicks time_domain_now = main_thread_only().time_domain->Now();
// Make sure |delayed_run_time| isn't in the past.
if (delayed_run_time < time_domain_now) {
// If |delayed_run_time| is in the past then push it onto the work queue
// immediately. To ensure the right task ordering we need to temporarily
// push it onto the |delayed_incoming_queue|.
delayed_run_time = time_domain_now;
pending_task.delayed_run_time = time_domain_now;
main_thread_only().delayed_incoming_queue.push(std::move(pending_task));
LazyNow lazy_now(time_domain_now);
WakeUpForDelayedWork(&lazy_now);
MoveReadyDelayedTasksToDelayedWorkQueue(&lazy_now);
} else {
// If |delayed_run_time| is in the future we can queue it as normal.
PushOntoDelayedIncomingQueueFromMainThread(std::move(pending_task),
time_domain_now);
main_thread_only().delayed_incoming_queue.push(std::move(pending_task));
main_thread_only().time_domain->ScheduleDelayedWork(
this, delayed_run_time, main_thread_only().time_domain->Now());
}
TraceQueueSize(false);
}
......@@ -350,7 +334,7 @@ bool TaskQueueImpl::HasPendingImmediateWork() const {
return !any_thread().immediate_incoming_queue.empty();
}
void TaskQueueImpl::WakeUpForDelayedWork(LazyNow* lazy_now) {
void TaskQueueImpl::MoveReadyDelayedTasksToDelayedWorkQueue(LazyNow* lazy_now) {
// Enqueue all delayed tasks that should be running now, skipping any that
// have been canceled.
while (!main_thread_only().delayed_incoming_queue.empty()) {
......@@ -368,13 +352,6 @@ void TaskQueueImpl::WakeUpForDelayedWork(LazyNow* lazy_now) {
main_thread_only().delayed_work_queue->Push(std::move(task));
main_thread_only().delayed_incoming_queue.pop();
}
// Make sure the next wake up is scheduled.
if (!main_thread_only().delayed_incoming_queue.empty()) {
main_thread_only().time_domain->ScheduleDelayedWork(
this, main_thread_only().delayed_incoming_queue.top().delayed_run_time,
lazy_now->Now());
}
}
bool TaskQueueImpl::MaybeUpdateImmediateWorkQueues() {
......@@ -475,8 +452,6 @@ void TaskQueueImpl::AsValueInto(base::trace_event::TracedValue* state) const {
state->SetDouble("delay_to_next_task_ms",
delay_to_next_task.InMillisecondsF());
}
if (main_thread_only().current_fence)
state->SetInteger("current_fence", main_thread_only().current_fence);
if (verbose_tracing_enabled) {
state->BeginArray("immediate_incoming_queue");
QueueAsValueInto(any_thread().immediate_incoming_queue, state);
......
third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.h
View file @ 04e1e5ca
......@@ -172,9 +172,8 @@ class BLINK_PLATFORM_EXPORT TaskQueueImpl final : public TaskQueue {
}
// Enqueues any delayed tasks which should be run now on the
// |delayed_work_queue|. It also schedules the next wake up with the
// TimeDomain. Must be called from the main thread.
void WakeUpForDelayedWork(LazyNow* lazy_now);
// |delayed_work_queue|. Must be called from the main thread.
void MoveReadyDelayedTasksToDelayedWorkQueue(LazyNow* lazy_now);
private:
friend class WorkQueue;
......
third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc
View file @ 04e1e5ca
......@@ -190,14 +190,10 @@ void TaskQueueManager::MaybeScheduleDelayedWork(
return;
}
// Only request the wake up if, either there are no wake ups scheduled, or if
// its sooner than any previously requested wake up.
// De-duplicate DoWork posts.
base::TimeTicks run_time = now + delay;
if (!main_thread_pending_wakeups_.empty() &&
*main_thread_pending_wakeups_.begin() <= run_time) {
if (!main_thread_pending_wakeups_.insert(run_time).second)
return;
}
main_thread_pending_wakeups_.insert(run_time);
delegate_->PostDelayedTask(
from_here, base::Bind(&TaskQueueManager::DoWork,
weak_factory_.GetWeakPtr(), run_time, true),
......
third_party/WebKit/Source/platform/scheduler/base/task_queue_manager_unittest.cc
View file @ 04e1e5ca
......@@ -118,26 +118,6 @@ class TaskQueueManagerTest : public testing::Test {
manager_->UpdateWorkQueues(lazy_now);
}
// Runs all immediate tasks until there is no more work to do and advances
// time if there is a pending delayed task. |per_run_time_callback| is called
// when the clock advances.
void RunUntilIdle(base::Closure per_run_time_callback) {
for (;;) {
// Advance time if we've run out of immediate work to do.
if (manager_->selector_.EnabledWorkQueuesEmpty()) {
base::TimeTicks run_time;
if (manager_->real_time_domain()->NextScheduledRunTime(&run_time)) {
now_src_->SetNowTicks(run_time);
per_run_time_callback.Run();
} else {
break;
}
}
test_task_runner_->RunPendingTasks();
}
}
std::unique_ptr<base::MessageLoop> message_loop_;
std::unique_ptr<base::SimpleTestTickClock> now_src_;
scoped_refptr<TaskQueueManagerDelegateForTest> main_task_runner_;
......@@ -1863,150 +1843,5 @@ TEST_F(TaskQueueManagerTestWithTracing, BlameContextAttribution) {
EXPECT_EQ(2u, events.size());
}
class CancelableTask {
public:
CancelableTask() : weak_factory_(this) {}
void NopTask() {}
base::WeakPtrFactory<CancelableTask> weak_factory_;
};
TEST_F(TaskQueueManagerTest, NoWakeUpsForCanceledDelayedTasks) {
Initialize(1u);
base::TimeTicks start_time = manager_->delegate()->NowTicks();
CancelableTask task1;
CancelableTask task2;
CancelableTask task3;
CancelableTask task4;
base::TimeDelta delay1(base::TimeDelta::FromSeconds(5));
base::TimeDelta delay2(base::TimeDelta::FromSeconds(10));
base::TimeDelta delay3(base::TimeDelta::FromSeconds(15));
base::TimeDelta delay4(base::TimeDelta::FromSeconds(30));
runners_[0]->PostDelayedTask(
FROM_HERE,
base::Bind(&CancelableTask::NopTask, task1.weak_factory_.GetWeakPtr()),
delay1);
runners_[0]->PostDelayedTask(
FROM_HERE,
base::Bind(&CancelableTask::NopTask, task2.weak_factory_.GetWeakPtr()),
delay2);
runners_[0]->PostDelayedTask(
FROM_HERE,
base::Bind(&CancelableTask::NopTask, task3.weak_factory_.GetWeakPtr()),
delay3);
runners_[0]->PostDelayedTask(
FROM_HERE,
base::Bind(&CancelableTask::NopTask, task4.weak_factory_.GetWeakPtr()),
delay4);
task2.weak_factory_.InvalidateWeakPtrs();
task3.weak_factory_.InvalidateWeakPtrs();
std::set<base::TimeTicks> run_times;
RunUntilIdle(base::Bind([](std::set<base::TimeTicks>* run_times,
base::SimpleTestTickClock* clock) {
run_times->insert(clock->NowTicks());
}, &run_times, now_src_.get()));
EXPECT_THAT(run_times, ElementsAre(start_time + delay1, start_time + delay4));
}
TEST_F(TaskQueueManagerTest, NoWakeUpsForCanceledDelayedTasksReversePostOrder) {
Initialize(1u);
base::TimeTicks start_time = manager_->delegate()->NowTicks();
CancelableTask task1;
CancelableTask task2;
CancelableTask task3;
CancelableTask task4;
base::TimeDelta delay1(base::TimeDelta::FromSeconds(5));
base::TimeDelta delay2(base::TimeDelta::FromSeconds(10));
base::TimeDelta delay3(base::TimeDelta::FromSeconds(15));
base::TimeDelta delay4(base::TimeDelta::FromSeconds(30));
runners_[0]->PostDelayedTask(
FROM_HERE,
base::Bind(&CancelableTask::NopTask, task4.weak_factory_.GetWeakPtr()),
delay4);
runners_[0]->PostDelayedTask(
FROM_HERE,
base::Bind(&CancelableTask::NopTask, task3.weak_factory_.GetWeakPtr()),
delay3);
runners_[0]->PostDelayedTask(
FROM_HERE,
base::Bind(&CancelableTask::NopTask, task2.weak_factory_.GetWeakPtr()),
delay2);
runners_[0]->PostDelayedTask(
FROM_HERE,
base::Bind(&CancelableTask::NopTask, task1.weak_factory_.GetWeakPtr()),
delay1);
task2.weak_factory_.InvalidateWeakPtrs();
task3.weak_factory_.InvalidateWeakPtrs();
std::set<base::TimeTicks> run_times;
RunUntilIdle(base::Bind([](std::set<base::TimeTicks>* run_times,
base::SimpleTestTickClock* clock) {
run_times->insert(clock->NowTicks());
}, &run_times, now_src_.get()));
EXPECT_THAT(run_times, ElementsAre(start_time + delay1, start_time + delay4));
}
TEST_F(TaskQueueManagerTest, TimeDomainWakeUpOnlyCancelledIfAllUsesCancelled) {
Initialize(1u);
base::TimeTicks start_time = manager_->delegate()->NowTicks();
CancelableTask task1;
CancelableTask task2;
CancelableTask task3;
CancelableTask task4;
base::TimeDelta delay1(base::TimeDelta::FromSeconds(5));
base::TimeDelta delay2(base::TimeDelta::FromSeconds(10));
base::TimeDelta delay3(base::TimeDelta::FromSeconds(15));
base::TimeDelta delay4(base::TimeDelta::FromSeconds(30));
runners_[0]->PostDelayedTask(
FROM_HERE,
base::Bind(&CancelableTask::NopTask, task1.weak_factory_.GetWeakPtr()),
delay1);
runners_[0]->PostDelayedTask(
FROM_HERE,
base::Bind(&CancelableTask::NopTask, task2.weak_factory_.GetWeakPtr()),
delay2);
runners_[0]->PostDelayedTask(
FROM_HERE,
base::Bind(&CancelableTask::NopTask, task3.weak_factory_.GetWeakPtr()),
delay3);
runners_[0]->PostDelayedTask(
FROM_HERE,
base::Bind(&CancelableTask::NopTask, task4.weak_factory_.GetWeakPtr()),
delay4);
// Post a non-canceled task with |delay3|. So we should still get a wakeup at
// |delay3| even though we cancel |task3|.
runners_[0]->PostDelayedTask(
FROM_HERE, base::Bind(&CancelableTask::NopTask, base::Unretained(&task3)),
delay3);
task2.weak_factory_.InvalidateWeakPtrs();
task3.weak_factory_.InvalidateWeakPtrs();
std::set<base::TimeTicks> run_times;
RunUntilIdle(base::Bind([](std::set<base::TimeTicks>* run_times,
base::SimpleTestTickClock* clock) {
run_times->insert(clock->NowTicks());
}, &run_times, now_src_.get()));
EXPECT_THAT(run_times, ElementsAre(start_time + delay1, start_time + delay3,
start_time + delay4));
}
} // namespace scheduler
} // namespace blink
third_party/WebKit/Source/platform/scheduler/base/time_domain.cc
View file @ 04e1e5ca
......@@ -198,7 +198,7 @@ void TimeDomain::WakeupReadyDelayedQueues(LazyNow* lazy_now) {
// in which EnqueueTaskLocks is called is respected when choosing which
// queue to execute a task from.
if (dedup_set.insert(next_wakeup->second).second) {
next_wakeup->second->WakeUpForDelayedWork(lazy_now);
next_wakeup->second->MoveReadyDelayedTasksToDelayedWorkQueue(lazy_now);
}
delayed_wakeup_multimap_.erase(next_wakeup);
}
......
third_party/WebKit/Source/platform/scheduler/base/time_domain.h
View file @ 04e1e5ca
......@@ -24,17 +24,13 @@ class TaskQueueImpl;
class TaskQueueManager;
class TaskQueueManagerDelegate;
// The TimeDomain's job is to wake task queues up when their next delayed tasks
// are due to fire. TaskQueues request a wake up via ScheduleDelayedWork, when
// the WakeUp is due the TimeDomain calls TaskQueue::WakeUpForDelayedWork which
// schedules the next non-canceled wakeup.
// The TimeDomain's job is to keep track of moments when delayed tasks have been
// scheduled to fire and to notify their TaskQueues via UpdateDelayedWorkQueue.
//
// To prevent spurious wake-ups for canceled tasks the TaskQueue should only
// have a single wake up registered with its TimeDomain. If should call
// CancelDelayedWork as needed to ensure this. The TimeDomain communicates with
// the TaskQueueManager to actually schedule the wake-ups on the underlying
// base::MessageLoop. Various levels of de-duping are employed to prevent
// unnecessary posting of TaskQueueManager::DoWork.
// The time domain keeps track of the next wakeup required to pump delayed tasks
// and issues |RequestWakeup| calls to the subclass as needed. Where possible
// it tried to de-dupe these wakeups. Ideally it would be possible to cancel
// them, but that's not currently supported by the base message loop.
//
// The clock itself is provided by subclasses of the TimeDomain and it may be
// the real wall clock or a synthetic (virtual) time base.
......@@ -93,13 +89,13 @@ class BLINK_PLATFORM_EXPORT TimeDomain {
// UpdateWorkQueue on.
void RegisterAsUpdatableTaskQueue(internal::TaskQueueImpl* queue);
// Schedules a call to TaskQueueImpl::WakeUpForDelayedWork
// Schedules a call to TaskQueueImpl::MoveReadyDelayedTasksToDelayedWorkQueue
// when this TimeDomain reaches |delayed_run_time|.
void ScheduleDelayedWork(internal::TaskQueueImpl* queue,
base::TimeTicks delayed_run_time,
base::TimeTicks now);
// Cancels a call to TaskQueueImpl::WakeUpForDelayedWork
// Cancels a call to TaskQueueImpl::MoveReadyDelayedTasksToDelayedWorkQueue
// previously requested with ScheduleDelayedWork. Note this only works if
// delayed_run_time is _not_ the next scheduled run time.
void CancelDelayedWork(internal::TaskQueueImpl* queue,
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment