[blink scheduler] Refactor tracing helper categories to work with c++17

Previously, the category names for TracingHelper had to meet 2 criteria:
 1. Be constexpr
 2. Have a single unique address per category

This was implemented using struct-level static constexpr const char*s,
but in in c++17 these are implicitly inline and have different addresses
in different shared libraries.

(2) was only required to eliminate multiple template instantiations for
the same category (due to the same string having different addresses).
We get around this by making an enum class of permitted trace categories
and using that as the template parameter instead of the char*. We use an
inline constexpr function to convert the enum value to the trace
category string at compile time, which is required by the tracing
macros.

Bug: 1275221, 752720
Change-Id: Idf82752a4bb389ce9e04d859eec93e022255992f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3313482
Commit-Queue: Scott Haseley <shaseley@chromium.org>
Reviewed-by: Alexander Timin <altimin@chromium.org>
Reviewed-by: Nico Weber <thakis@chromium.org>
Cr-Commit-Position: refs/heads/main@{#948132}

third_party/blink/renderer/platform/scheduler/common/back_forward_cache_disabling_feature_tracker.h

@@ -80,7 +80,7 @@ class PLATFORM_EXPORT BackForwardCacheDisablingFeatureTracker {
       back_forward_cache_disabling_feature_counts_{};
   std::bitset<static_cast<size_t>(SchedulingPolicy::Feature::kMaxValue) + 1>
       back_forward_cache_disabling_features_{};
-  TraceableState<bool, TracingCategoryName::kInfo>
+  TraceableState<bool, TracingCategory::kInfo>
       opted_out_from_back_forward_cache_;
 
   // The last set of features passed to FrameOrWorkerScheduler::Delegate::

third_party/blink/renderer/platform/scheduler/common/throttling/cpu_time_budget_pool.h

@@ -92,7 +92,7 @@ class PLATFORM_EXPORT CPUTimeBudgetPool : public BudgetPool {
   // that at least one task will be run every max_throttling_delay.
   absl::optional<base::TimeDelta> max_throttling_delay_;
 
-  TraceableCounter<base::TimeDelta, TracingCategoryName::kInfo>
+  TraceableCounter<base::TimeDelta, TracingCategory::kInfo>
       current_budget_level_;
   base::TimeTicks last_checkpoint_;
   double cpu_percentage_;

third_party/blink/renderer/platform/scheduler/common/tracing_helper.cc

@@ -11,26 +11,6 @@ namespace scheduler {
 
 using perfetto::protos::pbzero::RendererMainThreadTaskExecution;
 
-constexpr const char TracingCategoryName::kTopLevel[];
-constexpr const char TracingCategoryName::kDefault[];
-constexpr const char TracingCategoryName::kInfo[];
-constexpr const char TracingCategoryName::kDebug[];
-
-namespace internal {
-
-void ValidateTracingCategory(const char* category) {
-  // Category must be a constant defined in tracing helper because there's no
-  // portable way to use string literals as a template argument.
-  // Unfortunately, static_assert won't work with templates either because
-  // inequality (!=) of linker symbols is undefined in compile-time.
-  DCHECK(category == TracingCategoryName::kTopLevel ||
-         category == TracingCategoryName::kDefault ||
-         category == TracingCategoryName::kInfo ||
-         category == TracingCategoryName::kDebug);
-}
-
-}  // namespace internal
-
 double TimeDeltaToMilliseconds(const base::TimeDelta& value) {
   return value.InMillisecondsF();
 }

third_party/blink/renderer/platform/scheduler/common/tracing_helper.h

@@ -24,23 +24,25 @@
 namespace blink {
 namespace scheduler {
 
-// DISCLAIMER
-// Using these constants in TRACE_EVENTs is discouraged nor should you pass any
-// non-literal string as a category, unless familiar with tracing internals.
-// The constants are implemented as static members of a class to have an unique
-// address and not violate ODR.
-struct PLATFORM_EXPORT TracingCategoryName {
-  static constexpr const char kTopLevel[] = "toplevel";
-  static constexpr const char kDefault[] = "renderer.scheduler";
-  static constexpr const char kInfo[] =
-      TRACE_DISABLED_BY_DEFAULT("renderer.scheduler");
-  static constexpr const char kDebug[] =
-      TRACE_DISABLED_BY_DEFAULT("renderer.scheduler.debug");
-};
+// Available scheduler tracing categories for use with `StateTracer` and
+// friends.
+enum class TracingCategory { kTopLevel, kDefault, kInfo, kDebug };
 
 namespace internal {
 
-PLATFORM_EXPORT void ValidateTracingCategory(const char* category);
+constexpr const char* TracingCategoryToString(TracingCategory category) {
+  switch (category) {
+    case TracingCategory::kTopLevel:
+      return "toplevel";
+    case TracingCategory::kDefault:
+      return "renderer.scheduler";
+    case TracingCategory::kInfo:
+      return TRACE_DISABLED_BY_DEFAULT("renderer.scheduler");
+    case TracingCategory::kDebug:
+      return TRACE_DISABLED_BY_DEFAULT("renderer.scheduler.debug");
+  }
+  return nullptr;
+}
 
 }  // namespace internal
 
@@ -95,20 +97,22 @@ class TraceableVariable {
 // of category. Hence, we need distinct version for each category in order to
 // prevent unintended leak of state.
 
-template <const char* category>
+template <TracingCategory category>
 class StateTracer {
   DISALLOW_NEW();
 
  public:
-  explicit StateTracer(const char* name) : name_(name), slice_is_open_(false) {
-    internal::ValidateTracingCategory(category);
-  }
+  explicit StateTracer(const char* name) : name_(name), slice_is_open_(false) {}
+
   StateTracer(const StateTracer&) = delete;
   StateTracer& operator=(const StateTracer&) = delete;
 
   ~StateTracer() {
-    if (slice_is_open_)
-      TRACE_EVENT_NESTABLE_ASYNC_END0(category, name_, TRACE_ID_LOCAL(this));
+    if (slice_is_open_) {
+      TRACE_EVENT_NESTABLE_ASYNC_END0(
+          internal::TracingCategoryToString(category), name_,
+          TRACE_ID_LOCAL(this));
+    }
   }
 
   // String will be copied before leaving this function.
@@ -123,25 +127,30 @@ class StateTracer {
  protected:
   bool is_enabled() const {
     bool result = false;
-    TRACE_EVENT_CATEGORY_GROUP_ENABLED(category, &result);  // Cached.
+    TRACE_EVENT_CATEGORY_GROUP_ENABLED(
+        internal::TracingCategoryToString(category), &result);  // Cached.
     return result;
   }
 
  private:
   void TraceImpl(const char* state, bool need_copy) {
     if (slice_is_open_) {
-      TRACE_EVENT_NESTABLE_ASYNC_END0(category, name_, TRACE_ID_LOCAL(this));
+      TRACE_EVENT_NESTABLE_ASYNC_END0(
+          internal::TracingCategoryToString(category), name_,
+          TRACE_ID_LOCAL(this));
       slice_is_open_ = false;
     }
     if (!state || !is_enabled())
       return;
 
     if (need_copy) {
-      TRACE_EVENT_NESTABLE_ASYNC_BEGIN1(category, name_, TRACE_ID_LOCAL(this),
-                                        "state", TRACE_STR_COPY(state));
+      TRACE_EVENT_NESTABLE_ASYNC_BEGIN1(
+          internal::TracingCategoryToString(category), name_,
+          TRACE_ID_LOCAL(this), "state", TRACE_STR_COPY(state));
     } else {
-      TRACE_EVENT_NESTABLE_ASYNC_BEGIN1(category, name_, TRACE_ID_LOCAL(this),
-                                        "state", state);
+      TRACE_EVENT_NESTABLE_ASYNC_BEGIN1(
+          internal::TracingCategoryToString(category), name_,
+          TRACE_ID_LOCAL(this), "state", state);
     }
     slice_is_open_ = true;
   }
@@ -155,7 +164,7 @@ class StateTracer {
 
 // TODO(kraynov): Rename to something less generic and reflecting
 // the enum nature of such variables.
-template <typename T, const char* category>
+template <typename T, TracingCategory category>
 class TraceableState : public TraceableVariable, private StateTracer<category> {
  public:
   // Converter must return compile-time defined const strings because tracing
@@ -230,35 +239,34 @@ class TraceableState : public TraceableVariable, private StateTracer<category> {
   T state_;
 };
 
-template <const char* category, typename TypedValue>
+template <TracingCategory category, typename TypedValue>
 class ProtoStateTracer {
   DISALLOW_NEW();
 
  public:
   explicit ProtoStateTracer(const char* name)
-      : name_(name), slice_is_open_(false) {
-    internal::ValidateTracingCategory(category);
-  }
+      : name_(name), slice_is_open_(false) {}
 
   ProtoStateTracer(const ProtoStateTracer&) = delete;
   ProtoStateTracer& operator=(const ProtoStateTracer&) = delete;
 
   ~ProtoStateTracer() {
     if (slice_is_open_) {
-      TRACE_EVENT_END(category, track());
+      TRACE_EVENT_END(internal::TracingCategoryToString(category), track());
     }
   }
 
   void TraceProto(TypedValue* value) {
     const auto trace_track = track();
     if (slice_is_open_) {
-      TRACE_EVENT_END(category, trace_track);
+      TRACE_EVENT_END(internal::TracingCategoryToString(category), trace_track);
       slice_is_open_ = false;
     }
     if (!is_enabled())
       return;
 
-    TRACE_EVENT_BEGIN(category, perfetto::StaticString{name_}, trace_track,
+    TRACE_EVENT_BEGIN(internal::TracingCategoryToString(category),
+                      perfetto::StaticString{name_}, trace_track,
                       [value](perfetto::EventContext ctx) {
                         value->AsProtozeroInto(ctx.event());
                       });
@@ -269,7 +277,8 @@ class ProtoStateTracer {
  protected:
   bool is_enabled() const {
     bool result = false;
-    TRACE_EVENT_CATEGORY_GROUP_ENABLED(category, &result);  // Cached.
+    TRACE_EVENT_CATEGORY_GROUP_ENABLED(
+        internal::TracingCategoryToString(category), &result);  // Cached.
     return result;
   }
 
@@ -289,7 +298,7 @@ template <typename T>
 using InitializeProtoFuncPtr =
     void (*)(perfetto::protos::pbzero::TrackEvent* event, T e);
 
-template <typename T, const char* category>
+template <typename T, TracingCategory category>
 class TraceableObjectState
     : public TraceableVariable,
       public ProtoStateTracer<category, TraceableObjectState<T, category>> {
@@ -342,15 +351,16 @@ class TraceableObjectState
 
   bool is_enabled() const {
     bool result = false;
-    TRACE_EVENT_CATEGORY_GROUP_ENABLED(category, &result);  // Cached.
+    TRACE_EVENT_CATEGORY_GROUP_ENABLED(
+        internal::TracingCategoryToString(category), &result);  // Cached.
     return result;
   }
 
   T state_;
   InitializeProtoFuncPtr<T> proto_init_func_;
 };
 
-template <typename T, const char* category>
+template <typename T, TracingCategory category>
 class TraceableCounter : public TraceableVariable {
  public:
   using ConverterFuncPtr = double (*)(const T&);
@@ -363,7 +373,6 @@ class TraceableCounter : public TraceableVariable {
         name_(name),
         converter_(converter),
         value_(initial_value) {
-    internal::ValidateTracingCategory(category);
     Trace();
   }
 
@@ -405,7 +414,8 @@ class TraceableCounter : public TraceableVariable {
   void OnTraceLogEnabled() final { Trace(); }
 
   void Trace() const {
-    TRACE_COUNTER_ID1(category, name_, this, converter_(value_));
+    TRACE_COUNTER_ID1(internal::TracingCategoryToString(category), name_, this,
+                      converter_(value_));
   }
 
  private:
@@ -417,54 +427,54 @@ class TraceableCounter : public TraceableVariable {
 
 // Add operators when it's needed.
 
-template <typename T, const char* category>
+template <typename T, TracingCategory category>
 constexpr T operator-(const TraceableCounter<T, category>& counter) {
   return -counter.value();
 }
 
-template <typename T, const char* category>
+template <typename T, TracingCategory category>
 constexpr T operator/(const TraceableCounter<T, category>& lhs, const T& rhs) {
   return lhs.value() / rhs;
 }
 
-template <typename T, const char* category>
+template <typename T, TracingCategory category>
 constexpr bool operator>(const TraceableCounter<T, category>& lhs,
                          const T& rhs) {
   return lhs.value() > rhs;
 }
 
-template <typename T, const char* category>
+template <typename T, TracingCategory category>
 constexpr bool operator<(const TraceableCounter<T, category>& lhs,
                          const T& rhs) {
   return lhs.value() < rhs;
 }
 
-template <typename T, const char* category>
+template <typename T, TracingCategory category>
 constexpr bool operator!=(const TraceableCounter<T, category>& lhs,
                           const T& rhs) {
   return lhs.value() != rhs;
 }
 
-template <typename T, const char* category>
+template <typename T, TracingCategory category>
 constexpr T operator++(TraceableCounter<T, category>& counter) {
   counter = counter.value() + 1;
   return counter.value();
 }
 
-template <typename T, const char* category>
+template <typename T, TracingCategory category>
 constexpr T operator--(TraceableCounter<T, category>& counter) {
   counter = counter.value() - 1;
   return counter.value();
 }
 
-template <typename T, const char* category>
+template <typename T, TracingCategory category>
 constexpr T operator++(TraceableCounter<T, category>& counter, int) {
   T value = counter.value();
   counter = value + 1;
   return value;
 }
 
-template <typename T, const char* category>
+template <typename T, TracingCategory category>
 constexpr T operator--(TraceableCounter<T, category>& counter, int) {
   T value = counter.value();
   counter = value - 1;

third_party/blink/renderer/platform/scheduler/common/tracing_helper_unittest.cc

@@ -34,7 +34,7 @@ const char* SignOfInt(int value) {
 }
 
 class TraceableStateForTest
-    : public TraceableState<int, TracingCategoryName::kDefault> {
+    : public TraceableState<int, TracingCategory::kDefault> {
  public:
   TraceableStateForTest(TraceableVariableController* controller)
       : TraceableState(0, "State", controller, SignOfInt) {
@@ -58,13 +58,7 @@ class TraceableStateForTest
 
 // TODO(kraynov): TraceableCounter tests.
 
-#if defined(COMPONENT_BUILD)
-// TODO(https://crbug.com/1275221): Figure out, re-enable.
-#define MAYBE_TraceableState DISABLED_TraceableState
-#else
-#define MAYBE_TraceableState TraceableState
-#endif
-TEST(TracingHelperTest, MAYBE_TraceableState) {
+TEST(TracingHelperTest, TraceableState) {
   TraceableVariableController controller;
   TraceableStateForTest state(&controller);
   controller.OnTraceLogEnabled();
@@ -77,18 +71,12 @@ TEST(TracingHelperTest, MAYBE_TraceableState) {
   ExpectTraced("negative");
 }
 
-#if defined(COMPONENT_BUILD)
-// TODO(https://crbug.com/1275221): Figure out, re-enable.
-#define MAYBE_TraceableStateOperators DISABLED_TraceableStateOperators
-#else
-#define MAYBE_TraceableStateOperators TraceableStateOperators
-#endif
-TEST(TracingHelperTest, MAYBE_TraceableStateOperators) {
+TEST(TracingHelperTest, TraceableStateOperators) {
   TraceableVariableController controller;
-  TraceableState<int, TracingCategoryName::kDebug> x(-1, "X", &controller,
-                                                     SignOfInt);
-  TraceableState<int, TracingCategoryName::kDebug> y(1, "Y", &controller,
-                                                     SignOfInt);
+  TraceableState<int, TracingCategory::kDebug> x(-1, "X", &controller,
+                                                 SignOfInt);
+  TraceableState<int, TracingCategory::kDebug> y(1, "Y", &controller,
+                                                 SignOfInt);
   EXPECT_EQ(0, x + y);
   EXPECT_FALSE(x == y);
   EXPECT_TRUE(x != y);