From a1c7eb7f23d0a275d223144a1cffbf3714d8982f Mon Sep 17 00:00:00 2001 From: skyostil Date: Fri, 12 Feb 2016 09:14:08 -0800 Subject: Revert of Refactor signaling in RWP (patchset #9 id:180001 of https://codereview.chromium.org/1666283002/ ) Reason for revert: Broke the thread_times.tough_scrolling_cases test (verified with a local revert): https://build.chromium.org/p/chromium.perf/builders/Linux%20Perf%20%281%29/builds/10247 Original issue's description: > Refactor signaling in RWP > > A previous patch had moved RasterWorkerPool to make heavy use of > broadcast in order to simplify logic a bit. It turns out that broadcast > is not ideal in performance critical situations, as it increases lock > contention (see > https://code.google.com/p/chromium/codesearch#chromium/src/base/synchronization/condition_variable.h&l=34). > > This change removes all uses of broadcast other than in Shutdown, where > performance is less of a concern and we actually need all threads to > wake up. > > To achieve this, we need to re-factor RWP to use: > - N foreground threads > - 1 background thread > rather than N threads which may be able to run foreground/background tasks. > > In order to ensure that we don't overload the system, this change > introduces a limiting system where the background priority thread can only > run tasks if no other threads are doing so. > > BUG= > CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel TBR=reveman@chromium.org,sievers@chromium.org,avi.rohit@gmail.com,ericrk@chromium.org # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG= Review URL: https://codereview.chromium.org/1690023005 Cr-Commit-Position: refs/heads/master@{#375195} --- cc/raster/task_graph_work_queue.cc | 35 ++++++++++++++--------------------- cc/raster/task_graph_work_queue.h | 25 ++++--------------------- 2 files changed, 18 insertions(+), 42 deletions(-) (limited to 'cc/raster') diff --git a/cc/raster/task_graph_work_queue.cc b/cc/raster/task_graph_work_queue.cc index 0b59822..2947402 100644 --- a/cc/raster/task_graph_work_queue.cc +++ b/cc/raster/task_graph_work_queue.cc @@ -97,12 +97,9 @@ void TaskGraphWorkQueue::ScheduleTasks(NamespaceToken token, TaskGraph* graph) { continue; // Skip if already running. - const auto& running_tasks_for_category = - task_namespace.running_tasks.find(node.category); - if (running_tasks_for_category != task_namespace.running_tasks.cend() && - std::find(running_tasks_for_category->second.cbegin(), - running_tasks_for_category->second.cend(), - node.task) != running_tasks_for_category->second.cend()) + if (std::find(task_namespace.running_tasks.begin(), + task_namespace.running_tasks.end(), + node.task) != task_namespace.running_tasks.end()) continue; task_namespace.ready_to_run_tasks[node.category].push_back(PrioritizedTask( @@ -130,12 +127,9 @@ void TaskGraphWorkQueue::ScheduleTasks(NamespaceToken token, TaskGraph* graph) { continue; // Skip if already running. - const auto& running_tasks_for_category = - task_namespace.running_tasks.find(node.category); - if (running_tasks_for_category != task_namespace.running_tasks.cend() && - std::find(running_tasks_for_category->second.cbegin(), - running_tasks_for_category->second.cend(), - node.task) != running_tasks_for_category->second.cend()) + if (std::find(task_namespace.running_tasks.begin(), + task_namespace.running_tasks.end(), + node.task) != task_namespace.running_tasks.end()) continue; DCHECK(std::find(task_namespace.completed_tasks.begin(), @@ -201,7 +195,7 @@ TaskGraphWorkQueue::PrioritizedTask TaskGraphWorkQueue::GetNextTaskToRun( } // Add task to |running_tasks|. - task_namespace->running_tasks[category].push_back(task.task); + task_namespace->running_tasks.push_back(task.task); return task; } @@ -209,15 +203,13 @@ TaskGraphWorkQueue::PrioritizedTask TaskGraphWorkQueue::GetNextTaskToRun( void TaskGraphWorkQueue::CompleteTask(const PrioritizedTask& completed_task) { TaskNamespace* task_namespace = completed_task.task_namespace; scoped_refptr task(completed_task.task); - uint16_t category = completed_task.category; // Remove task from |running_tasks|. - auto& running_tasks_for_category = task_namespace->running_tasks[category]; - auto it = std::find(running_tasks_for_category.begin(), - running_tasks_for_category.end(), task); - DCHECK(it != running_tasks_for_category.end()); - std::swap(*it, running_tasks_for_category.back()); - running_tasks_for_category.pop_back(); + auto it = std::find(task_namespace->running_tasks.begin(), + task_namespace->running_tasks.end(), task); + DCHECK(it != task_namespace->running_tasks.end()); + std::swap(*it, task_namespace->running_tasks.back()); + task_namespace->running_tasks.pop_back(); // Now iterate over all dependents to decrement dependencies and check if they // are ready to run. @@ -285,7 +277,8 @@ void TaskGraphWorkQueue::CollectCompletedTasks(NamespaceToken token, // Remove namespace if finished running tasks. DCHECK_EQ(0u, task_namespace.completed_tasks.size()); - DCHECK(HasFinishedRunningTasksInNamespace(&task_namespace)); + DCHECK(!HasReadyToRunTasksInNamespace(&task_namespace)); + DCHECK_EQ(0u, task_namespace.running_tasks.size()); namespaces_.erase(it); } diff --git a/cc/raster/task_graph_work_queue.h b/cc/raster/task_graph_work_queue.h index ed1af18..95d3c77 100644 --- a/cc/raster/task_graph_work_queue.h +++ b/cc/raster/task_graph_work_queue.h @@ -60,11 +60,11 @@ class CC_EXPORT TaskGraphWorkQueue { // category. std::map ready_to_run_tasks; - // This set contains all currently running tasks. - std::map running_tasks; - // Completed tasks not yet collected by origin thread. Task::Vector completed_tasks; + + // This set contains all currently running tasks. + Task::Vector running_tasks; }; TaskGraphWorkQueue(); @@ -112,12 +112,7 @@ class CC_EXPORT TaskGraphWorkQueue { static bool HasFinishedRunningTasksInNamespace( const TaskNamespace* task_namespace) { - return std::all_of( - task_namespace->running_tasks.cbegin(), - task_namespace->running_tasks.cend(), - [](const std::pair& tasks_entry) { - return tasks_entry.second.empty(); - }) && + return task_namespace->running_tasks.empty() && !HasReadyToRunTasksInNamespace(task_namespace); } @@ -150,18 +145,6 @@ class CC_EXPORT TaskGraphWorkQueue { return ready_to_run_namespaces_; } - size_t NumRunningTasksForCategory(uint16_t category) const { - size_t count = 0; - for (const auto& task_namespace_entry : namespaces_) { - const auto& running_tasks = task_namespace_entry.second.running_tasks; - const auto& running_tasks_for_category = running_tasks.find(category); - if (running_tasks_for_category != running_tasks.cend()) { - count += running_tasks_for_category->second.size(); - } - } - return count; - } - // Helper function which ensures that graph dependencies were correctly // configured. static bool DependencyMismatch(const TaskGraph* graph); -- cgit v1.1