[Task Scheduler]: Make task traits more robust and simple.

Main issue: TaskTraits constructor is not robust; any
signature is part of the overload set, even if it may not compile.
Because of that, it may not possible to hold TaskTrait in a tuple (depending on implementation).
Fix: Expose a single constructor that checks if all traits are valid,
or if an extension can accept all traits.
Caveat: An extension has to redefine base TaskTrait::ValidTraits as valid
(through inheritance).
This CL also simplifies querying traits thanks to C++14 relaxed constant expressions.
This reduces compile time complexity of TaskTraits constructor from O(n^3) to O(n^2).


Cq-Include-Trybots: luci.chromium.try:ios-simulator-cronet;luci.chromium.try:ios-simulator-full-configs
Change-Id: I73f993f6be837f1ac665b4b09c78d9d7272435a8
Reviewed-on: https://chromium-review.googlesource.com/c/1194687Reviewed-by: Eugene But <eugenebut@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: François Doray <fdoray@chromium.org>
Commit-Queue: Etienne Pierre-Doray <etiennep@chromium.org>
Cr-Commit-Position: refs/heads/master@{#596341}

base/task/task_traits.h

@@ -123,15 +123,24 @@ struct WithBaseSyncPrimitives {};
 // Describes immutable metadata for a single task or a group of tasks.
 class BASE_EXPORT TaskTraits {
  private:
+  using TaskPriorityFilter =
+      trait_helpers::EnumTraitFilter<TaskPriority, TaskPriority::USER_VISIBLE>;
+  using MayBlockFilter = trait_helpers::BooleanTraitFilter<MayBlock>;
+  using TaskShutdownBehaviorFilter =
+      trait_helpers::EnumTraitFilter<TaskShutdownBehavior,
+                                     TaskShutdownBehavior::SKIP_ON_SHUTDOWN>;
+  using WithBaseSyncPrimitivesFilter =
+      trait_helpers::BooleanTraitFilter<WithBaseSyncPrimitives>;
+
+ public:
   // ValidTrait ensures TaskTraits' constructor only accepts appropriate types.
   struct ValidTrait {
-    ValidTrait(TaskPriority) {}
-    ValidTrait(TaskShutdownBehavior) {}
-    ValidTrait(MayBlock) {}
-    ValidTrait(WithBaseSyncPrimitives) {}
+    ValidTrait(TaskPriority);
+    ValidTrait(TaskShutdownBehavior);
+    ValidTrait(MayBlock);
+    ValidTrait(WithBaseSyncPrimitives);
   };
 
- public:
   // Invoking this constructor without arguments produces TaskTraits that are
   // appropriate for tasks that
   //     (1) don't block (ref. MayBlock() and WithBaseSyncPrimitives()),
@@ -153,49 +162,26 @@ class BASE_EXPORT TaskTraits {
   // constexpr base::TaskTraits other_user_visible_may_block_traits = {
   //     base::MayBlock(), base::TaskPriority::USER_VISIBLE};
   template <class... ArgTypes,
-            class CheckArgumentsAreValidBaseTraits = std::enable_if_t<
-                trait_helpers::AreValidTraits<ValidTrait, ArgTypes...>::value>>
+            class CheckArgumentsAreValid = std::enable_if_t<
+                trait_helpers::AreValidTraits<ValidTrait, ArgTypes...>::value ||
+                trait_helpers::AreValidTraitsForExtension<ArgTypes...>::value>>
   constexpr TaskTraits(ArgTypes... args)
-      : priority_(trait_helpers::GetValueFromArgList(
-            trait_helpers::EnumArgGetter<TaskPriority,
-                                         TaskPriority::USER_VISIBLE>(),
-            args...)),
-        shutdown_behavior_(trait_helpers::GetValueFromArgList(
-            trait_helpers::EnumArgGetter<
-                TaskShutdownBehavior,
-                TaskShutdownBehavior::SKIP_ON_SHUTDOWN>(),
+      : extension_(trait_helpers::GetTaskTraitsExtension(
+            trait_helpers::AreValidTraits<ValidTrait, ArgTypes...>{},
             args...)),
+        priority_(
+            trait_helpers::GetTraitFromArgList<TaskPriorityFilter>(args...)),
+        shutdown_behavior_(
+            trait_helpers::GetTraitFromArgList<TaskShutdownBehaviorFilter>(
+                args...)),
         priority_set_explicitly_(
-            trait_helpers::HasArgOfType<TaskPriority, ArgTypes...>::value),
+            trait_helpers::TraitIsDefined<TaskPriorityFilter>(args...)),
         shutdown_behavior_set_explicitly_(
-            trait_helpers::HasArgOfType<TaskShutdownBehavior,
-                                        ArgTypes...>::value),
-        may_block_(trait_helpers::GetValueFromArgList(
-            trait_helpers::BooleanArgGetter<MayBlock>(),
-            args...)),
-        with_base_sync_primitives_(trait_helpers::GetValueFromArgList(
-            trait_helpers::BooleanArgGetter<WithBaseSyncPrimitives>(),
-            args...)) {}
-
-  // Construct TaskTraits with extension traits. See task_traits_extension.h.
-  template <class... ArgTypes,
-            class AvoidConstructorRedeclaration = void,
-            class CheckArgsContainNonBaseTrait = std::enable_if_t<
-                !trait_helpers::AreValidTraits<ValidTrait, ArgTypes...>::value>>
-  constexpr TaskTraits(ArgTypes... args)
-      // Select those arguments that are valid base TaskTraits and forward them
-      // to the above constructor via a helper constructor.
-      : TaskTraits(std::forward_as_tuple(args...),
-                   trait_helpers::SelectIndices<
-                       trait_helpers::ValidTraitTester<ValidTrait>::IsValid,
-                       ArgTypes...>{}) {
-    // Select all other arguments and try to create an extension with them.
-    extension_ = MakeTaskTraitsExtensionHelper(
-        std::forward_as_tuple(args...),
-        trait_helpers::SelectIndices<
-            trait_helpers::ValidTraitTester<ValidTrait>::IsInvalid,
-            ArgTypes...>{});
-  }
+            trait_helpers::TraitIsDefined<TaskShutdownBehaviorFilter>(args...)),
+        may_block_(trait_helpers::GetTraitFromArgList<MayBlockFilter>(args...)),
+        with_base_sync_primitives_(
+            trait_helpers::GetTraitFromArgList<WithBaseSyncPrimitivesFilter>(
+                args...)) {}
 
   constexpr TaskTraits(const TaskTraits& other) = default;
   TaskTraits& operator=(const TaskTraits& other) = default;
@@ -279,16 +265,6 @@ class BASE_EXPORT TaskTraits {
         with_base_sync_primitives_(left.with_base_sync_primitives_ ||
                                    right.with_base_sync_primitives_) {}
 
-  // Helper constructor which selects those arguments from |args| that are
-  // indicated by the index_sequence and forwards them to the public
-  // constructor. Due to filtering, the indices may be non-contiguous.
-  template <class... ArgTypes, std::size_t... Indices>
-  constexpr TaskTraits(std::tuple<ArgTypes...> args,
-                       std::index_sequence<Indices...>)
-      : TaskTraits(
-            std::get<Indices>(std::forward<std::tuple<ArgTypes...>>(args))...) {
-  }
-
   // Ordered for packing.
   TaskTraitsExtensionStorage extension_;
   TaskPriority priority_;

base/task/task_traits_extension.h

@@ -12,6 +12,7 @@
 #include <utility>
 
 #include "base/base_export.h"
+#include "base/task/task_traits_details.h"
 
 namespace base {
 
@@ -41,10 +42,14 @@ namespace base {
 // as its extension traits:
 //  (3) -- template <...>
 //      -- constexpr base::TaskTraitsExtensionStorage MakeTaskTraitsExtension(
-//      --     ArgTypes&&... args).
+//      --     ArgTypes... args).
 //      Constructs and serializes an extension with the given arguments into
-//      a TaskTraitsExtensionStorage and returns it. Should only accept valid
-//      arguments for the extension.
+//      a TaskTraitsExtensionStorage and returns it. When the extension is used,
+//      all traits, including the base ones, are passed to this function in
+//      order make sure TaskTraits constructor only participates in overload
+//      resolution if all traits are valid. As such, this function should only
+//      accept valid task traits recognised by the extension and the base task
+//      traits.
 //
 // EXAMPLE (see also base/task/test_task_traits_extension.h):
 // --------
@@ -57,20 +62,24 @@ namespace base {
 //   static constexpr uint8_t kExtensionId =
 //       TaskTraitsExtensionStorage::kFirstEmbedderExtensionId;
 //
-//   struct ValidTrait {
-//     ValidTrait(MyExtensionTrait) {}
+//   struct ValidTrait : public TaskTraits::ValidTrait {
+//     // Accept base traits in MakeTaskTraitsExtension (see above).
+//     using TaskTraits::ValidTrait::ValidTrait;
+//
+//     ValidTrait(MyExtensionTrait);
 //   };
 //
+//   using MyExtensionTraitFilter =
+//     trait_helpers::EnumTraitFilter<MyExtensionTrait, MyExtensionTrait::kA>;
+//
 //   // Constructor that accepts only valid traits as specified by ValidTraits.
 //   template <class... ArgTypes,
 //             class CheckArgumentsAreValid = std::enable_if_t<
 //                 base::trait_helpers::AreValidTraits<
 //                     ValidTrait, ArgTypes...>::value>>
 //   constexpr MyTaskTraitsExtension(ArgTypes... args)
-//       : my_trait_(base::trait_helpers::GetValueFromArgList(
-//             base::trait_helpers::EnumArgGetter<MyExtensionTrait,
-//                                                MyExtensionTrait::kValue1>(),
-//             args...)) {}
+//       : my_trait_(trait_helpers::GetTraitFromArgList<MyExtensionTraitFilter>(
+//                      args...)) {}
 //
 //   // Serializes MyTaskTraitsExtension into a storage object and returns it.
 //   constexpr base::TaskTraitsExtensionStorage Serialize() const {
@@ -100,8 +109,8 @@ namespace base {
 //               base::trait_helpers::AreValidTraits<
 //                   MyTaskTraitsExtension::ValidTrait, ArgTypes...>::value>>
 // constexpr base::TaskTraitsExtensionStorage MakeTaskTraitsExtension(
-//     ArgTypes&&... args) {
-//   return MyTaskTraitsExtension(std::forward<ArgTypes>(args)...).Serialize();
+//     ArgTypes... args) {
+//   return MyTaskTraitsExtension(args...).Serialize();
 // }
 // }  // namespace my_embedder
 //
@@ -171,6 +180,44 @@ inline constexpr TaskTraitsExtensionStorage::TaskTraitsExtensionStorage(
 inline constexpr TaskTraitsExtensionStorage::TaskTraitsExtensionStorage(
     const TaskTraitsExtensionStorage& other) = default;
 
+namespace trait_helpers {
+
+// Helper class whose constructor tests if an extension accepts a list of
+// argument types.
+struct TaskTraitsExtension {
+  template <class... ArgTypes,
+            class CheckCanMakeExtension =
+                decltype(MakeTaskTraitsExtension(std::declval<ArgTypes>()...))>
+  constexpr TaskTraitsExtension(ArgTypes... args) {}
+};
+
+// Tests that that a trait extension accepts all |ArgsTypes...|.
+template <class... ArgTypes>
+struct AreValidTraitsForExtension
+    : std::integral_constant<
+          bool,
+          std::is_constructible<TaskTraitsExtension, ArgTypes...>::value> {};
+
+// Helper function that returns the TaskTraitsExtensionStorage of a
+// serialized extension created with |args...| if there are arguments that are
+// not valid base traits, or a default constructed TaskTraitsExtensionStorage
+// otherwise.
+template <class... ArgTypes>
+constexpr TaskTraitsExtensionStorage GetTaskTraitsExtension(
+    std::true_type base_traits,
+    ArgTypes... args) {
+  return TaskTraitsExtensionStorage();
+}
+
+template <class... ArgTypes>
+constexpr TaskTraitsExtensionStorage GetTaskTraitsExtension(
+    std::false_type base_traits,
+    ArgTypes... args) {
+  return MakeTaskTraitsExtension(args...);
+}
+
+}  // namespace trait_helpers
+
 // TODO(eseckler): Default the comparison operator once C++20 arrives.
 inline bool TaskTraitsExtensionStorage::operator==(
     const TaskTraitsExtensionStorage& other) const {
@@ -180,25 +227,6 @@ inline bool TaskTraitsExtensionStorage::operator==(
   return extension_id == other.extension_id && data == other.data;
 }
 
-// Default implementation of MakeTaskTraitsExtension template function, which
-// doesn't accept any traits and does not create an extension.
-template <class Unused = void>
-constexpr TaskTraitsExtensionStorage MakeTaskTraitsExtension(
-    TaskTraitsExtensionStorage& storage) {
-  return TaskTraitsExtensionStorage();
-}
-
-// Forwards those arguments from |args| that are indicated by the index_sequence
-// to a MakeTaskTraitsExtension() template function, which is provided by the
-// embedder in an unknown namespace; its resolution relies on argument-dependent
-// lookup. Due to filtering, the provided indices may be non-contiguous.
-template <class... ArgTypes, std::size_t... Indices>
-constexpr TaskTraitsExtensionStorage MakeTaskTraitsExtensionHelper(
-    std::tuple<ArgTypes...> args,
-    std::index_sequence<Indices...>) {
-  return MakeTaskTraitsExtension(
-      std::get<Indices>(std::forward<std::tuple<ArgTypes...>>(args))...);
-}
 
 }  // namespace base
 

base/task/task_traits_extension_unittest.cc

@@ -9,6 +9,13 @@
 
 namespace base {
 
+TEST(TaskTraitsExtensionTest, NoExtension) {
+  constexpr TaskTraits traits = {};
+
+  EXPECT_EQ(traits.extension_id(),
+            TaskTraitsExtensionStorage::kInvalidExtensionId);
+}
+
 TEST(TaskTraitsExtensionTest, CreateWithOneExtensionTrait) {
   constexpr TaskTraits traits = {TestExtensionEnumTrait::kB};
 

base/task/task_traits_extension_unittest.nc

@@ -15,7 +15,7 @@ namespace base {
 constexpr TaskTraits traits = {MayBlock(), MayBlock()};
 #elif defined(NCTEST_TASK_TRAITS_EXTENSION_MULTIPLE_EXTENSION_TRAITS)  // [r"Multiple arguments of the same type were provided to the constructor of TaskTraits."]
 constexpr TaskTraits traits = {TestExtensionEnumTrait::kB, TestExtensionEnumTrait::kC};
-#elif defined(NCTEST_TASK_TRAITS_EXTENSION_INVALID_TYPE)  // [r"no matching function for call to 'MakeTaskTraitsExtension'"]
+#elif defined(NCTEST_TASK_TRAITS_EXTENSION_INVALID_TYPE)  // [r"no matching constructor for initialization of 'const base::TaskTraits'"]
 constexpr TaskTraits traits = {TestExtensionEnumTrait::kB, 123};
 #elif defined(NCTEST_TASK_TRAITS_EXTENSION_TOO_MUCH_DATA_FOR_STORAGE)  // [r"no matching constructor for initialization of 'base::TaskTraitsExtensionStorage'"]
 constexpr TaskTraitsExtensionStorage TestSerializeTaskTraitsWithTooMuchData() {

base/task/task_traits_unittest.nc

@@ -24,7 +24,7 @@ constexpr TaskTraits traits = {TaskShutdownBehavior::BLOCK_SHUTDOWN,
 constexpr TaskTraits traits = {TaskShutdownBehavior::BLOCK_SHUTDOWN,
                                MayBlock(),
                                TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN};
-#elif defined(NCTEST_TASK_TRAITS_INVALID_TYPE)  // [r"no matching function for call to 'MakeTaskTraitsExtension'"]
+#elif defined(NCTEST_TASK_TRAITS_INVALID_TYPE)  // [r"no matching constructor for initialization of 'const base::TaskTraits'"]
 constexpr TaskTraits traits = {TaskShutdownBehavior::BLOCK_SHUTDOWN, true};
 #endif
 

base/task/test_task_traits_extension.h

@@ -20,21 +20,26 @@ class TestTaskTraitsExtension {
   static constexpr uint8_t kExtensionId =
       TaskTraitsExtensionStorage::kFirstEmbedderExtensionId;
 
-  struct ValidTrait {
-    ValidTrait(TestExtensionEnumTrait) {}
-    ValidTrait(TestExtensionBoolTrait) {}
+  struct ValidTrait : public TaskTraits::ValidTrait {
+    using TaskTraits::ValidTrait::ValidTrait;
+
+    ValidTrait(TestExtensionEnumTrait);
+    ValidTrait(TestExtensionBoolTrait);
   };
 
+  using TestExtensionEnumFilter =
+      trait_helpers::EnumTraitFilter<TestExtensionEnumTrait,
+                                     TestExtensionEnumTrait::kA>;
+  using TestExtensionBoolFilter =
+      trait_helpers::BooleanTraitFilter<TestExtensionBoolTrait>;
+
   template <class... ArgTypes,
             class CheckArgumentsAreValid = std::enable_if_t<
                 trait_helpers::AreValidTraits<ValidTrait, ArgTypes...>::value>>
   constexpr TestTaskTraitsExtension(ArgTypes... args)
-      : enum_trait_(trait_helpers::GetValueFromArgList(
-            trait_helpers::EnumArgGetter<TestExtensionEnumTrait,
-                                         TestExtensionEnumTrait::kA>(),
+      : enum_trait_(trait_helpers::GetTraitFromArgList<TestExtensionEnumFilter>(
             args...)),
-        bool_trait_(trait_helpers::GetValueFromArgList(
-            trait_helpers::BooleanArgGetter<TestExtensionBoolTrait>(),
+        bool_trait_(trait_helpers::GetTraitFromArgList<TestExtensionBoolFilter>(
             args...)) {}
 
   constexpr TaskTraitsExtensionStorage Serialize() const {
@@ -65,8 +70,7 @@ template <class... ArgTypes,
           class = std::enable_if_t<
               trait_helpers::AreValidTraits<TestTaskTraitsExtension::ValidTrait,
                                             ArgTypes...>::value>>
-constexpr TaskTraitsExtensionStorage MakeTaskTraitsExtension(
-    ArgTypes&&... args) {
+constexpr TaskTraitsExtensionStorage MakeTaskTraitsExtension(ArgTypes... args) {
   return TestTaskTraitsExtension(std::forward<ArgTypes>(args)...).Serialize();
 }
 

content/public/browser/browser_task_traits.h

@@ -41,13 +41,20 @@ struct NonNestable {};
 // Posting to a BrowserThread must only be done after it was initialized (ref.
 // BrowserMainLoop::CreateThreads() phase).
 class CONTENT_EXPORT BrowserTaskTraitsExtension {
+  using BrowserThreadIDFilter =
+      base::trait_helpers::RequiredEnumTraitFilter<BrowserThread::ID>;
+  using NonNestableFilter =
+      base::trait_helpers::BooleanTraitFilter<NonNestable>;
+
  public:
   static constexpr uint8_t kExtensionId =
       base::TaskTraitsExtensionStorage::kFirstEmbedderExtensionId;
 
-  struct ValidTrait {
-    ValidTrait(BrowserThread::ID) {}
-    ValidTrait(NonNestable) {}
+  struct ValidTrait : public base::TaskTraits::ValidTrait {
+    using base::TaskTraits::ValidTrait::ValidTrait;
+
+    ValidTrait(BrowserThread::ID);
+    ValidTrait(NonNestable);
   };
 
   template <
@@ -55,11 +62,10 @@ class CONTENT_EXPORT BrowserTaskTraitsExtension {
       class CheckArgumentsAreValid = std::enable_if_t<
           base::trait_helpers::AreValidTraits<ValidTrait, ArgTypes...>::value>>
   constexpr BrowserTaskTraitsExtension(ArgTypes... args)
-      : browser_thread_(base::trait_helpers::GetValueFromArgList(
-            base::trait_helpers::RequiredEnumArgGetter<BrowserThread::ID>(),
-            args...)),
-        nestable_(!base::trait_helpers::GetValueFromArgList(
-            base::trait_helpers::BooleanArgGetter<NonNestable>(),
+      : browser_thread_(
+            base::trait_helpers::GetTraitFromArgList<BrowserThreadIDFilter>(
+                args...)),
+        nestable_(!base::trait_helpers::GetTraitFromArgList<NonNestableFilter>(
             args...)) {}
 
   constexpr base::TaskTraitsExtensionStorage Serialize() const {

content/public/browser/browser_task_traits_unittest.nc

@@ -10,7 +10,7 @@
 
 namespace content {
 
-#if defined(NCTEST_BROWSER_TASK_TRAITS_NO_THREAD)  // [r"no member named 'GetDefaultValue' in 'base::trait_helpers::RequiredEnumArgGetter<content::BrowserThread::ID>'"]
+#if defined(NCTEST_BROWSER_TASK_TRAITS_NO_THREAD)  // [r"TaskTraits contains a Trait that must be explicity initialized in its constructor."]
 constexpr base::TaskTraits traits = {NonNestable()};
 #elif defined(NCTEST_BROWSER_TASK_TRAITS_MULTIPLE_THREADS)  // [r"Multiple arguments of the same type were provided to the constructor of TaskTraits."]
 constexpr base::TaskTraits traits = {BrowserThread::UI,

ios/web/public/web_task_traits.h

@@ -40,13 +40,20 @@ struct NonNestable {};
 // Posting to a WebThread must only be done after it was initialized (ref.
 // WebMainLoop::CreateThreads() phase).
 class WebTaskTraitsExtension {
+  using WebThreadIDFilter =
+      base::trait_helpers::RequiredEnumTraitFilter<WebThread::ID>;
+  using NonNestableFilter =
+      base::trait_helpers::BooleanTraitFilter<NonNestable>;
+
  public:
   static constexpr uint8_t kExtensionId =
       base::TaskTraitsExtensionStorage::kFirstEmbedderExtensionId;
 
-  struct ValidTrait {
-    ValidTrait(WebThread::ID) {}
-    ValidTrait(NonNestable) {}
+  struct ValidTrait : public base::TaskTraits::ValidTrait {
+    using base::TaskTraits::ValidTrait::ValidTrait;
+
+    ValidTrait(WebThread::ID);
+    ValidTrait(NonNestable);
   };
 
   template <
@@ -54,11 +61,9 @@ class WebTaskTraitsExtension {
       class CheckArgumentsAreValid = std::enable_if_t<
           base::trait_helpers::AreValidTraits<ValidTrait, ArgTypes...>::value>>
   constexpr WebTaskTraitsExtension(ArgTypes... args)
-      : web_thread_(base::trait_helpers::GetValueFromArgList(
-            base::trait_helpers::RequiredEnumArgGetter<WebThread::ID>(),
+      : web_thread_(base::trait_helpers::GetTraitFromArgList<WebThreadIDFilter>(
             args...)),
-        nestable_(!base::trait_helpers::GetValueFromArgList(
-            base::trait_helpers::BooleanArgGetter<NonNestable>(),
+        nestable_(!base::trait_helpers::GetTraitFromArgList<NonNestableFilter>(
             args...)) {}
 
   constexpr base::TaskTraitsExtensionStorage Serialize() const {