Revert "base: Require tasks to state their thread affinity"

This reverts commit 6a5cc7e. Reason for revert: breaking windows perf builders Links to build failures: https://ci.chromium.org/p/chrome/builders/ci/win32-builder-perf/102474 Original change's description: > base: Require tasks to state their thread affinity > > This patch makes it a requirement for all task traits to specify their > thread affinity. Previously task traits that did not name a specific > thread would implicitly run on the thread pool, which could be > surprising. To make this more obvious, thread pool tasks must now use > the base::ThreadPool() trait. > > We also modify the base::HasTrait() template helper to not require > passing arguments because 1) that makes it possible to use the helper > in a constant (compile-time) expression and 2) the helper wasn't using > the argument values for anything anyway. > > TBR=mackay@chromium.org > > Bug: 968047 > Change-Id: I2950c632772b341fd0eebb85f14f31dbcb158715 > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1635827 > Reviewed-by: Sami Kyöstilä <skyostil@chromium.org> > Reviewed-by: François Doray <fdoray@chromium.org> > Reviewed-by: Kenneth MacKay <kmackay@chromium.org> > Reviewed-by: Avi Drissman <avi@chromium.org> > Reviewed-by: Sylvain Defresne <sdefresne@chromium.org> > Reviewed-by: Nico Weber <thakis@chromium.org> > Reviewed-by: Gabriel Charette <gab@chromium.org> > Commit-Queue: Sami Kyöstilä <skyostil@chromium.org> > Auto-Submit: Sami Kyöstilä <skyostil@chromium.org> > Cr-Commit-Position: refs/heads/master@{#686824} TBR=avi@chromium.org,gab@chromium.org,thakis@chromium.org,fdoray@chromium.org,fdoray@google.com,sdefresne@chromium.org,skyostil@chromium.org,kmackay@chromium.org,mackay@chromium.org Change-Id: I919e47a12967b26fe941e971989f33493e448a2d No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 968047 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1754519 Reviewed-by: Sadrul Chowdhury <sadrul@chromium.org> Commit-Queue: Sadrul Chowdhury <sadrul@chromium.org> Cr-Commit-Position: refs/heads/master@{#686944}
Denger-Network · Aug 14, 2019 · bc02f86 · bc02f86
1 parent 7650e7a
commit bc02f86
Show file tree

Hide file tree

Showing 22 changed files with 45 additions and 71 deletions.
diff --git a/base/task/task_traits.h b/base/task/task_traits.h
@@ -212,15 +212,13 @@ class BASE_EXPORT TaskTraits {
   // WithBaseSyncPrimitives in any order to the constructor.
   //
   // E.g.
-  // constexpr base::TaskTraits default_traits = {base::ThreadPool()};
-  // constexpr base::TaskTraits user_visible_traits = {
-  //     base::ThreadPool(), base::TaskPriority::USER_VISIBLE};
+  // constexpr base::TaskTraits default_traits = {};
+  // constexpr base::TaskTraits user_visible_traits =
+  //     {base::TaskPriority::USER_VISIBLE};
   // constexpr base::TaskTraits user_visible_may_block_traits = {
-  //     base::ThreadPool(), base::TaskPriority::USER_VISIBLE, base::MayBlock()
-  // };
+  //     base::TaskPriority::USER_VISIBLE, base::MayBlock()};
   // constexpr base::TaskTraits other_user_visible_may_block_traits = {
-  //     base::ThreadPool(), base::MayBlock(), base::TaskPriority::USER_VISIBLE
-  // };
+  //     base::MayBlock(), base::TaskPriority::USER_VISIBLE};
   template <class... ArgTypes,
             class CheckArgumentsAreValid = std::enable_if_t<
                 trait_helpers::AreValidTraits<ValidTrait, ArgTypes...>::value ||
@@ -233,38 +231,27 @@ class BASE_EXPORT TaskTraits {
             static_cast<uint8_t>(
                 trait_helpers::GetEnum<TaskPriority,
                                        TaskPriority::USER_BLOCKING>(args...)) |
-            (trait_helpers::HasTrait<TaskPriority, ArgTypes...>()
-                 ? kIsExplicitFlag
-                 : 0)),
+            (trait_helpers::HasTrait<TaskPriority>(args...) ? kIsExplicitFlag
+                                                            : 0)),
         shutdown_behavior_(
             static_cast<uint8_t>(
                 trait_helpers::GetEnum<TaskShutdownBehavior,
                                        TaskShutdownBehavior::SKIP_ON_SHUTDOWN>(
                     args...)) |
-            (trait_helpers::HasTrait<TaskShutdownBehavior, ArgTypes...>()
+            (trait_helpers::HasTrait<TaskShutdownBehavior>(args...)
                  ? kIsExplicitFlag
                  : 0)),
         thread_policy_(
             static_cast<uint8_t>(
                 trait_helpers::GetEnum<ThreadPolicy,
                                        ThreadPolicy::PREFER_BACKGROUND>(
                     args...)) |
-            (trait_helpers::HasTrait<ThreadPolicy, ArgTypes...>()
-                 ? kIsExplicitFlag
-                 : 0)),
-        may_block_(trait_helpers::HasTrait<MayBlock, ArgTypes...>()),
+            (trait_helpers::HasTrait<ThreadPolicy>(args...) ? kIsExplicitFlag
+                                                            : 0)),
+        may_block_(trait_helpers::HasTrait<MayBlock>(args...)),
         with_base_sync_primitives_(
-            trait_helpers::HasTrait<WithBaseSyncPrimitives, ArgTypes...>()),
-        use_thread_pool_(trait_helpers::HasTrait<ThreadPool, ArgTypes...>()) {
-    constexpr bool has_thread_pool =
-        trait_helpers::HasTrait<ThreadPool, ArgTypes...>();
-    constexpr bool has_extension =
-        !trait_helpers::AreValidTraits<ValidTrait, ArgTypes...>::value;
-    static_assert(
-        has_thread_pool ^ has_extension,
-        "Traits must explicitly specify a destination (e.g. ThreadPool or a "
-        "named thread like BrowserThread)");
-  }
+            trait_helpers::HasTrait<WithBaseSyncPrimitives>(args...)),
+        use_thread_pool_(trait_helpers::HasTrait<ThreadPool>(args...)) {}
 
   constexpr TaskTraits(const TaskTraits& other) = default;
   TaskTraits& operator=(const TaskTraits& other) = default;

diff --git a/base/task/test_task_traits_extension.h b/base/task/test_task_traits_extension.h
@@ -34,8 +34,7 @@ class TestTaskTraitsExtension {
       : enum_trait_(
             trait_helpers::GetEnum<TestExtensionEnumTrait,
                                    TestExtensionEnumTrait::kA>(args...)),
-        bool_trait_(
-            trait_helpers::HasTrait<TestExtensionBoolTrait, ArgTypes...>()) {}
+        bool_trait_(trait_helpers::HasTrait<TestExtensionBoolTrait>(args...)) {}
 
   constexpr TaskTraitsExtensionStorage Serialize() const {
     return {kExtensionId, {{static_cast<uint8_t>(enum_trait_), bool_trait_}}};

diff --git a/base/test/launcher/test_launcher.cc b/base/test/launcher/test_launcher.cc
@@ -630,7 +630,7 @@ void TestRunner::Run(const std::vector<std::string>& test_names) {
   task_runners_.clear();
   for (size_t i = 0; i < runner_count_; i++) {
     task_runners_.push_back(CreateSequencedTaskRunnerWithTraits(
-        {ThreadPool(), MayBlock(), TaskShutdownBehavior::BLOCK_SHUTDOWN}));
+        {MayBlock(), TaskShutdownBehavior::BLOCK_SHUTDOWN}));
     ThreadTaskRunnerHandle::Get()->PostTask(
         FROM_HERE,
         BindOnce(&TestRunner::LaunchNextTask, weak_ptr_factory_.GetWeakPtr(),

diff --git a/base/test/scoped_task_environment.h b/base/test/scoped_task_environment.h
@@ -166,8 +166,7 @@ class ScopedTaskEnvironment {
                                    ThreadPoolExecutionMode::DEFAULT>(args...),
             trait_helpers::GetEnum<ThreadingMode, ThreadingMode::DEFAULT>(
                 args...),
-            trait_helpers::HasTrait<SubclassCreatesDefaultTaskRunner,
-                                    ArgTypes...>(),
+            trait_helpers::HasTrait<SubclassCreatesDefaultTaskRunner>(args...),
             trait_helpers::NotATraitTag()) {}
 
   // Waits until no undelayed ThreadPool tasks remain. Then, unregisters the

diff --git a/base/traits_bag.h b/base/traits_bag.h
@@ -47,8 +47,7 @@
 //                 trait_helpers::AreValidTraits<ValidTraits,
 //                                               ArgTypes...>::value>>
 //   constexpr void DoSomethingAwesome(ArgTypes... args)
-//      : enable_feature_x(
-//            trait_helpers::HasTrait<EnableFeatureX, ArgTypes...>()),
+//      : enable_feature_x(trait_helpers::HasTrait<EnableFeatureX>(args...)),
 //        color(trait_helpers::GetEnum<Color, EnumTraitA::BLUE>(args...)) {}
 
 namespace base {
@@ -247,7 +246,7 @@ static constexpr Optional<Enum> GetOptionalEnum(Args... args) {
 
 // Helper to make checking for the presence of a trait more readable.
 template <typename Trait, typename... Args>
-static constexpr bool HasTrait() {
+static constexpr bool HasTrait(Args... args) {
   static_assert(
       count({std::is_constructible<Trait, Args>::value...}, true) <= 1,
       "The traits bag contains multiple traits of the same type.");

diff --git a/base/traits_bag_unittest.cc b/base/traits_bag_unittest.cc
@@ -30,7 +30,7 @@ struct TestTraits {
             class CheckArgumentsAreValid = std::enable_if_t<
                 trait_helpers::AreValidTraits<ValidTrait, ArgTypes...>::value>>
   constexpr TestTraits(ArgTypes... args)
-      : has_example_trait(trait_helpers::HasTrait<ExampleTrait, ArgTypes...>()),
+      : has_example_trait(trait_helpers::HasTrait<ExampleTrait>(args...)),
         enum_trait_a(
             trait_helpers::GetEnum<EnumTraitA, EnumTraitA::A>(args...)),
         enum_trait_b(

diff --git a/chrome/browser/apps/app_service/app_icon_factory.cc b/chrome/browser/apps/app_service/app_icon_factory.cc
@@ -190,9 +190,8 @@ void RunCallbackWithCompressedDataFromExtension(
   }
 
   // Try and load data from the resource file.
-  base::PostTaskAndReplyWithResult(
-      FROM_HERE,
-      {base::ThreadPool(), base::MayBlock(), base::TaskPriority::USER_VISIBLE},
+  base::PostTaskWithTraitsAndReplyWithResult(
+      FROM_HERE, {base::MayBlock(), base::TaskPriority::USER_VISIBLE},
       base::BindOnce(&CompressedDataFromResource, std::move(ext_resource)),
       base::BindOnce(&RunCallbackWithCompressedData, size_hint_in_dip,
                      default_icon_resource, is_placeholder_icon,
@@ -231,9 +230,7 @@ void RunCallbackWithImageSkia(int size_hint_in_dip,
 
     processed_image.MakeThreadSafe();
     base::PostTaskWithTraitsAndReplyWithResult(
-        FROM_HERE,
-        {base::ThreadPool(), base::MayBlock(),
-         base::TaskPriority::USER_VISIBLE},
+        FROM_HERE, {base::MayBlock(), base::TaskPriority::USER_VISIBLE},
         base::BindOnce(&EncodeImage, processed_image),
         base::BindOnce(&RunCallbackWithCompressedData, size_hint_in_dip,
                        default_icon_resource, is_placeholder_icon, icon_effects,

diff --git a/chrome/browser/chromeos/file_manager/file_manager_jstest_base.cc b/chrome/browser/chromeos/file_manager/file_manager_jstest_base.cc
@@ -40,9 +40,7 @@ class TestFilesDataSource : public content::URLDataSource {
       const content::WebContents::Getter& wc_getter,
       const content::URLDataSource::GotDataCallback& callback) override {
     base::PostTaskWithTraits(
-        FROM_HERE,
-        {base::ThreadPool(), base::MayBlock(),
-         base::TaskPriority::USER_BLOCKING},
+        FROM_HERE, {base::MayBlock(), base::TaskPriority::USER_BLOCKING},
         base::BindOnce(&TestFilesDataSource::ReadFile, base::Unretained(this),
                        path, callback));
   }

diff --git a/chrome/browser/metrics/perf/heap_collector.cc b/chrome/browser/metrics/perf/heap_collector.cc
@@ -418,7 +418,7 @@ void HeapCollector::ParseAndSaveProfile(
   auto task_runner = base::SequencedTaskRunnerHandle::Get();
   base::PostTask(
       FROM_HERE,
-      {base::ThreadPool(), base::MayBlock(), base::TaskPriority::BEST_EFFORT,
+      {base::MayBlock(), base::TaskPriority::BEST_EFFORT,
        base::TaskShutdownBehavior::SKIP_ON_SHUTDOWN},
       base::BindOnce(&HeapCollector::ParseProfileOnThreadPool, task_runner,
                      weak_factory_.GetWeakPtr(), std::move(parser),

diff --git a/chrome/browser/metrics/perf/metric_provider.cc b/chrome/browser/metrics/perf/metric_provider.cc
@@ -38,8 +38,8 @@ MetricProvider::MetricProvider(std::unique_ptr<MetricCollector> collector)
       // times when the system is not busy. The work performed on the dedicated
       // sequence is short and infrequent. Expensive parsing operations are
       // executed asynchronously on the thread pool.
-      collector_task_runner_(base::CreateSequencedTaskRunner(
-          {base::ThreadPool(), base::TaskPriority::USER_VISIBLE})),
+      collector_task_runner_(
+          base::CreateSequencedTaskRunner({base::TaskPriority::USER_VISIBLE})),
       metric_collector_(std::move(collector)),
       weak_factory_(this) {
   metric_collector_->set_profile_done_callback(base::BindRepeating(

diff --git a/chrome/browser/renderer_host/chrome_extension_message_filter.cc b/chrome/browser/renderer_host/chrome_extension_message_filter.cc
@@ -164,7 +164,7 @@ void ChromeExtensionMessageFilter::OnGetExtMessageBundle(
 
   // This blocks tab loading. Priority is inherited from the calling context.
   base::PostTaskWithTraits(
-      FROM_HERE, {base::ThreadPool(), base::MayBlock()},
+      FROM_HERE, {base::MayBlock()},
       base::BindOnce(&ChromeExtensionMessageFilter::OnGetExtMessageBundleAsync,
                      this, paths_to_load, extension_id, default_locale,
                      reply_msg));

diff --git a/chrome/test/base/browser_with_test_window_test.h b/chrome/test/base/browser_with_test_window_test.h
@@ -98,7 +98,7 @@ class BrowserWithTestWindowTest : public testing::Test {
                     args)...),
             base::trait_helpers::GetEnum<Browser::Type, Browser::TYPE_TABBED>(
                 args...),
-            base::trait_helpers::HasTrait<HostedApp, ArgTypes...>()) {}
+            base::trait_helpers::HasTrait<HostedApp>(args...)) {}
 
   ~BrowserWithTestWindowTest() override;
 

diff --git a/chromecast/media/cma/backend/cast_audio_json.cc b/chromecast/media/cma/backend/cast_audio_json.cc
@@ -67,9 +67,9 @@ base::FilePath CastAudioJson::GetFilePathForTuning() {
 }
 
 CastAudioJsonProviderImpl::CastAudioJsonProviderImpl()
-    : cast_audio_watcher_(base::SequenceBound<FileWatcher>(
-          base::CreateSequencedTaskRunner({base::ThreadPool(), base::MayBlock(),
-                                           base::TaskPriority::LOWEST}))) {}
+    : cast_audio_watcher_(
+          base::SequenceBound<FileWatcher>(base::CreateSequencedTaskRunner(
+              {base::MayBlock(), base::TaskPriority::LOWEST}))) {}
 
 CastAudioJsonProviderImpl::~CastAudioJsonProviderImpl() = default;
 

diff --git a/components/feed/core/feed_content_database.cc b/components/feed/core/feed_content_database.cc
@@ -53,9 +53,8 @@ FeedContentDatabase::FeedContentDatabase(
     leveldb_proto::ProtoDatabaseProvider* proto_database_provider,
     const base::FilePath& database_folder)
     : database_status_(InitStatus::kNotInitialized),
-      task_runner_(
-          base::CreateSequencedTaskRunner({base::ThreadPool(), base::MayBlock(),
-                                           base::TaskPriority::USER_VISIBLE})),
+      task_runner_(base::CreateSequencedTaskRunnerWithTraits(
+          {base::MayBlock(), base::TaskPriority::USER_VISIBLE})),
       storage_database_(proto_database_provider->GetDB<ContentStorageProto>(
           leveldb_proto::ProtoDbType::FEED_CONTENT_DATABASE,
           database_folder.AppendASCII(kContentDatabaseFolder),

diff --git a/components/feed/core/feed_content_database_unittest.cc b/components/feed/core/feed_content_database_unittest.cc
@@ -58,8 +58,7 @@ class FeedContentDatabaseTest : public testing::Test {
         std::make_unique<FakeDB<ContentStorageProto>>(&content_db_storage_);
 
     task_runner_ = base::CreateSequencedTaskRunnerWithTraits(
-        {base::ThreadPool(), base::MayBlock(),
-         base::TaskPriority::USER_VISIBLE});
+        {base::MayBlock(), base::TaskPriority::USER_VISIBLE});
 
     content_db_ = storage_db.get();
     feed_db_ = std::make_unique<FeedContentDatabase>(std::move(storage_db),

diff --git a/components/feed/core/feed_journal_database.cc b/components/feed/core/feed_journal_database.cc
@@ -38,9 +38,8 @@ FeedJournalDatabase::FeedJournalDatabase(
     leveldb_proto::ProtoDatabaseProvider* proto_database_provider,
     const base::FilePath& database_folder)
     : database_status_(InitStatus::kNotInitialized),
-      task_runner_(
-          base::CreateSequencedTaskRunner({base::ThreadPool(), base::MayBlock(),
-                                           base::TaskPriority::USER_VISIBLE})),
+      task_runner_(base::CreateSequencedTaskRunnerWithTraits(
+          {base::MayBlock(), base::TaskPriority::USER_VISIBLE})),
       storage_database_(proto_database_provider->GetDB<JournalStorageProto>(
           leveldb_proto::ProtoDbType::FEED_JOURNAL_DATABASE,
           database_folder.AppendASCII(kJournalDatabaseFolder),

diff --git a/components/feed/core/feed_journal_database_unittest.cc b/components/feed/core/feed_journal_database_unittest.cc
@@ -64,8 +64,7 @@ class FeedJournalDatabaseTest : public testing::Test {
         std::make_unique<FakeDB<JournalStorageProto>>(&journal_db_storage_);
 
     task_runner_ = base::CreateSequencedTaskRunnerWithTraits(
-        {base::ThreadPool(), base::MayBlock(),
-         base::TaskPriority::USER_VISIBLE});
+        {base::MayBlock(), base::TaskPriority::USER_VISIBLE});
 
     journal_db_ = storage_db.get();
     feed_db_ = std::make_unique<FeedJournalDatabase>(std::move(storage_db),

diff --git a/components/services/heap_profiling/heap_profiling_service.cc b/components/services/heap_profiling/heap_profiling_service.cc
@@ -121,8 +121,7 @@ mojo::PendingRemote<mojom::ProfilingService> LaunchService(
         helper) {
   mojo::PendingRemote<mojom::ProfilingService> remote;
   auto task_runner = base::CreateSingleThreadTaskRunner(
-      {base::ThreadPool(), base::TaskPriority::BEST_EFFORT,
-       base::WithBaseSyncPrimitives()},
+      {base::TaskPriority::BEST_EFFORT, base::WithBaseSyncPrimitives()},
       base::SingleThreadTaskRunnerThreadMode::DEDICATED);
   task_runner->PostTask(
       FROM_HERE,

diff --git a/components/ui_devtools/agent_util.cc b/components/ui_devtools/agent_util.cc
@@ -39,7 +39,7 @@ bool GetSourceCode(std::string path, std::string* source_code) {
   bool return_value;
   base::PostTaskAndReplyWithResult(
       FROM_HERE,
-      {base::ThreadPool(), base::MayBlock(), base::TaskPriority::USER_VISIBLE,
+      {base::MayBlock(), base::TaskPriority::USER_VISIBLE,
        base::TaskShutdownBehavior::SKIP_ON_SHUTDOWN},
       base::BindOnce(&base::ReadFileToString, src_dir, source_code),
       base::BindOnce(&OnSourceFile, run_loop.QuitClosure(), &return_value));

diff --git a/content/public/browser/browser_task_traits.h b/content/public/browser/browser_task_traits.h
@@ -84,7 +84,7 @@ class CONTENT_EXPORT BrowserTaskTraitsExtension {
         task_type_(
             base::trait_helpers::GetEnum<BrowserTaskType,
                                          BrowserTaskType::kDefault>(args...)),
-        nestable_(!base::trait_helpers::HasTrait<NonNestable, ArgTypes...>()) {}
+        nestable_(!base::trait_helpers::HasTrait<NonNestable>(args...)) {}
 
   // Keep in sync with UiThreadTaskTraits.java
   constexpr base::TaskTraitsExtensionStorage Serialize() const {

diff --git a/ios/web/public/thread/web_task_traits.h b/ios/web/public/thread/web_task_traits.h
@@ -57,7 +57,7 @@ class WebTaskTraitsExtension {
           base::trait_helpers::AreValidTraits<ValidTrait, ArgTypes...>::value>>
   constexpr WebTaskTraitsExtension(ArgTypes... args)
       : web_thread_(base::trait_helpers::GetEnum<WebThread::ID>(args...)),
-        nestable_(!base::trait_helpers::HasTrait<NonNestable, ArgTypes...>()) {}
+        nestable_(!base::trait_helpers::HasTrait<NonNestable>(args...)) {}
 
   constexpr base::TaskTraitsExtensionStorage Serialize() const {
     static_assert(8 == sizeof(WebTaskTraitsExtension),

diff --git a/third_party/blink/renderer/platform/scheduler/public/scheduling_policy.h b/third_party/blink/renderer/platform/scheduler/public/scheduling_policy.h
@@ -36,11 +36,11 @@ struct PLATFORM_EXPORT SchedulingPolicy {
                                                     ArgTypes...>::value>>
   constexpr SchedulingPolicy(ArgTypes... args)
       : disable_aggressive_throttling(
-            base::trait_helpers::HasTrait<DisableAggressiveThrottling,
-                                          ArgTypes...>()),
+            base::trait_helpers::HasTrait<DisableAggressiveThrottling>(
+                args...)),
         disable_back_forward_cache(
-            base::trait_helpers::HasTrait<RecordMetricsForBackForwardCache,
-                                          ArgTypes...>()) {}
+            base::trait_helpers::HasTrait<RecordMetricsForBackForwardCache>(
+                args...)) {}
 
   SchedulingPolicy() {}