Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(347)

Issues Search
    My Issues | Starred     Open | Closed | All

Unified Diff: components/scheduler/base/task_queue_selector.cc

Issue 1685093002: Fix bug with TaskQueueSelector and blocked queues (Closed) Base URL: https://chromium.googlesource.com/chromium/src.git@master
Patch Set: Improve the dcheck Created 4 years, 10 months ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View side-by-side diff with in-line comments
Download patch
« no previous file with comments | « components/scheduler/base/task_queue_selector.h ('k') | components/scheduler/base/task_queue_selector_unittest.cc » ('j') | components/scheduler/base/work_queue_sets.cc » ('J')
Expand Comments ('e') | Collapse Comments ('c') | Show Comments Hide Comments ('s')
Index: components/scheduler/base/task_queue_selector.cc
diff --git a/components/scheduler/base/task_queue_selector.cc b/components/scheduler/base/task_queue_selector.cc
index 312023246048d50d30a1c2285073c9da8cb78676..be164dc970f2bcab85f6fd8c09ae68e5efa4b5e4 100644
--- a/components/scheduler/base/task_queue_selector.cc
+++ b/components/scheduler/base/task_queue_selector.cc
@@ -13,8 +13,8 @@ namespace scheduler {
namespace internal {
TaskQueueSelector::TaskQueueSelector()
- : enabled_selector_(this),
- blocked_selector_(this),
+ : enabled_selector_(this, "enabled"),
+ blocked_selector_(this, "blocked"),
immediate_starvation_count_(0),
high_priority_starvation_count_(0),
num_blocked_queues_to_report_(0),
@@ -25,17 +25,27 @@ TaskQueueSelector::~TaskQueueSelector() {}
void TaskQueueSelector::AddQueue(internal::TaskQueueImpl* queue) {
DCHECK(main_thread_checker_.CalledOnValidThread());
DCHECK(queue->IsQueueEnabled());
- SetQueuePriority(queue, TaskQueue::NORMAL_PRIORITY);
+ enabled_selector_.AddQueue(queue, TaskQueue::NORMAL_PRIORITY);
}
void TaskQueueSelector::RemoveQueue(internal::TaskQueueImpl* queue) {
DCHECK(main_thread_checker_.CalledOnValidThread());
if (queue->IsQueueEnabled()) {
enabled_selector_.RemoveQueue(queue);
+ DCHECK(!blocked_selector_.immediate_work_queue_sets()
Sami 2016/02/10 13:15:47 How about a helper on the selector that combines t
alex clarke (OOO till 29th) 2016/02/10 14:37:12 Done.
+ ->ContainsWorkQueueForTest(queue->immediate_work_queue()));
+ DCHECK(
+ !blocked_selector_.delayed_work_queue_sets()->ContainsWorkQueueForTest(
+ queue->delayed_work_queue()));
} else if (queue->should_report_when_execution_blocked()) {
DCHECK_GT(num_blocked_queues_to_report_, 0u);
num_blocked_queues_to_report_--;
blocked_selector_.RemoveQueue(queue);
+ DCHECK(!enabled_selector_.immediate_work_queue_sets()
+ ->ContainsWorkQueueForTest(queue->immediate_work_queue()));
+ DCHECK(
+ !enabled_selector_.delayed_work_queue_sets()->ContainsWorkQueueForTest(
+ queue->delayed_work_queue()));
}
}
@@ -45,9 +55,9 @@ void TaskQueueSelector::EnableQueue(internal::TaskQueueImpl* queue) {
if (queue->should_report_when_execution_blocked()) {
DCHECK_GT(num_blocked_queues_to_report_, 0u);
num_blocked_queues_to_report_--;
+ blocked_selector_.RemoveQueue(queue);
}
- blocked_selector_.RemoveQueue(queue);
- enabled_selector_.AssignQueueToSet(queue, queue->GetQueuePriority());
+ enabled_selector_.AddQueue(queue, queue->GetQueuePriority());
if (task_queue_selector_observer_)
task_queue_selector_observer_->OnTaskQueueEnabled(queue);
}
@@ -56,9 +66,10 @@ void TaskQueueSelector::DisableQueue(internal::TaskQueueImpl* queue) {
DCHECK(main_thread_checker_.CalledOnValidThread());
DCHECK(!queue->IsQueueEnabled());
enabled_selector_.RemoveQueue(queue);
- blocked_selector_.AssignQueueToSet(queue, queue->GetQueuePriority());
- if (queue->should_report_when_execution_blocked())
+ if (queue->should_report_when_execution_blocked()) {
+ blocked_selector_.AddQueue(queue, queue->GetQueuePriority());
num_blocked_queues_to_report_++;
+ }
}
void TaskQueueSelector::SetQueuePriority(internal::TaskQueueImpl* queue,
@@ -66,9 +77,12 @@ void TaskQueueSelector::SetQueuePriority(internal::TaskQueueImpl* queue,
DCHECK_LT(priority, TaskQueue::QUEUE_PRIORITY_COUNT);
DCHECK(main_thread_checker_.CalledOnValidThread());
if (queue->IsQueueEnabled()) {
- enabled_selector_.AssignQueueToSet(queue, priority);
+ enabled_selector_.ChangeSetIndex(queue, priority);
+ } else if (queue->should_report_when_execution_blocked()) {
+ blocked_selector_.ChangeSetIndex(queue, priority);
} else {
- blocked_selector_.AssignQueueToSet(queue, priority);
+ queue->delayed_work_queue()->AssignSetIndex(priority);
Sami 2016/02/10 13:15:47 Mind adding a comment here why we need to do this
alex clarke (OOO till 29th) 2016/02/10 14:37:12 Done.
+ queue->immediate_work_queue()->AssignSetIndex(priority);
}
DCHECK_EQ(priority, queue->GetQueuePriority());
}
@@ -80,41 +94,63 @@ TaskQueue::QueuePriority TaskQueueSelector::NextPriority(
}
TaskQueueSelector::PrioritizingSelector::PrioritizingSelector(
- TaskQueueSelector* task_queue_selector)
+ TaskQueueSelector* task_queue_selector,
+ const char* name)
: task_queue_selector_(task_queue_selector),
- delayed_work_queue_sets_(TaskQueue::QUEUE_PRIORITY_COUNT),
- immediate_work_queue_sets_(TaskQueue::QUEUE_PRIORITY_COUNT) {}
+ delayed_work_queue_sets_(TaskQueue::QUEUE_PRIORITY_COUNT, name),
+ immediate_work_queue_sets_(TaskQueue::QUEUE_PRIORITY_COUNT, name) {}
-void TaskQueueSelector::PrioritizingSelector::AssignQueueToSet(
+void TaskQueueSelector::PrioritizingSelector::AddQueue(
internal::TaskQueueImpl* queue,
TaskQueue::QueuePriority priority) {
- delayed_work_queue_sets_.AssignQueueToSet(queue->delayed_work_queue(),
+// The #if DCHECK_IS_ON() shouldn't be necessary but this doesn't compile on
+// chromeos bots without it :(
+#if DCHECK_IS_ON()
+ DCHECK(!delayed_work_queue_sets_.ContainsWorkQueueForTest(
+ queue->delayed_work_queue()));
+ DCHECK(!immediate_work_queue_sets_.ContainsWorkQueueForTest(
+ queue->immediate_work_queue()));
+#endif
+ delayed_work_queue_sets_.AddQueue(queue->delayed_work_queue(), priority);
+ immediate_work_queue_sets_.AddQueue(queue->immediate_work_queue(), priority);
+#if DCHECK_IS_ON()
+ DCHECK(delayed_work_queue_sets_.ContainsWorkQueueForTest(
+ queue->delayed_work_queue()));
+ DCHECK(immediate_work_queue_sets_.ContainsWorkQueueForTest(
+ queue->immediate_work_queue()));
+#endif
+}
+
+void TaskQueueSelector::PrioritizingSelector::ChangeSetIndex(
+ internal::TaskQueueImpl* queue,
+ TaskQueue::QueuePriority priority) {
+// The #if DCHECK_IS_ON() shouldn't be necessary but this doesn't compile on
Sami 2016/02/10 13:15:47 nit: might not need to repeat this comment since i
+// chromeos bots without it :(
+#if DCHECK_IS_ON()
+ DCHECK(delayed_work_queue_sets_.ContainsWorkQueueForTest(
+ queue->delayed_work_queue()));
+ DCHECK(immediate_work_queue_sets_.ContainsWorkQueueForTest(
+ queue->immediate_work_queue()));
+#endif
+ delayed_work_queue_sets_.ChangeSetIndex(queue->delayed_work_queue(),
+ priority);
+ immediate_work_queue_sets_.ChangeSetIndex(queue->immediate_work_queue(),
priority);
- immediate_work_queue_sets_.AssignQueueToSet(queue->immediate_work_queue(),
- priority);
- // The #if DCHECK_IS_ON() shouldn't be necessary but this doesn't compile on
- // chromeos bots without it :(
#if DCHECK_IS_ON()
- DCHECK_EQ(queue->delayed_work_queue()->Empty(),
- !delayed_work_queue_sets_.ContainsWorkQueueForTest(
- queue->delayed_work_queue()));
- DCHECK_EQ(queue->immediate_work_queue()->Empty(),
- !immediate_work_queue_sets_.ContainsWorkQueueForTest(
- queue->immediate_work_queue()));
+ DCHECK(delayed_work_queue_sets_.ContainsWorkQueueForTest(
+ queue->delayed_work_queue()));
+ DCHECK(immediate_work_queue_sets_.ContainsWorkQueueForTest(
+ queue->immediate_work_queue()));
#endif
}
void TaskQueueSelector::PrioritizingSelector::RemoveQueue(
internal::TaskQueueImpl* queue) {
#if DCHECK_IS_ON()
- DCHECK_EQ(queue->delayed_work_queue()->Empty(),
- !delayed_work_queue_sets_.ContainsWorkQueueForTest(
- queue->delayed_work_queue()))
- << " Did you try to RemoveQueue twice? Thats not supported!";
- DCHECK_EQ(queue->immediate_work_queue()->Empty(),
- !immediate_work_queue_sets_.ContainsWorkQueueForTest(
- queue->immediate_work_queue()))
- << " Did you try to RemoveQueue twice? Thats not supported!";
+ DCHECK(delayed_work_queue_sets_.ContainsWorkQueueForTest(
+ queue->delayed_work_queue()));
+ DCHECK(immediate_work_queue_sets_.ContainsWorkQueueForTest(
+ queue->immediate_work_queue()));
#endif
delayed_work_queue_sets_.RemoveQueue(queue->delayed_work_queue());
immediate_work_queue_sets_.RemoveQueue(queue->immediate_work_queue());
@@ -123,7 +159,7 @@ void TaskQueueSelector::PrioritizingSelector::RemoveQueue(
DCHECK(!delayed_work_queue_sets_.ContainsWorkQueueForTest(
queue->delayed_work_queue()));
DCHECK(!immediate_work_queue_sets_.ContainsWorkQueueForTest(
- queue->delayed_work_queue()));
+ queue->immediate_work_queue()));
#endif
}
@@ -239,6 +275,7 @@ bool TaskQueueSelector::SelectWorkQueueToService(WorkQueue** out_work_queue) {
}
void TaskQueueSelector::TrySelectingBlockedQueue() {
+ DCHECK(main_thread_checker_.CalledOnValidThread());
if (!num_blocked_queues_to_report_ || !task_queue_selector_observer_)
return;
WorkQueue* chosen_blocked_queue;
@@ -255,6 +292,7 @@ void TaskQueueSelector::TrySelectingBlockedQueue() {
void TaskQueueSelector::TrySelectingBlockedQueueOverEnabledQueue(
const WorkQueue& chosen_enabled_queue) {
+ DCHECK(main_thread_checker_.CalledOnValidThread());
if (!num_blocked_queues_to_report_ || !task_queue_selector_observer_)
return;
@@ -325,6 +363,7 @@ void TaskQueueSelector::SetTaskQueueSelectorObserver(Observer* observer) {
}
bool TaskQueueSelector::EnabledWorkQueuesEmpty() const {
+ DCHECK(main_thread_checker_.CalledOnValidThread());
for (TaskQueue::QueuePriority priority = TaskQueue::CONTROL_PRIORITY;
priority < TaskQueue::QUEUE_PRIORITY_COUNT;
priority = NextPriority(priority)) {
« no previous file with comments | « components/scheduler/base/task_queue_selector.h ('k') | components/scheduler/base/task_queue_selector_unittest.cc » ('j') | components/scheduler/base/work_queue_sets.cc » ('J')

Powered by Google App Engine
This is Rietveld 408576698