From d933590e27715b14b13b7474c5cbef71d890ac33 Mon Sep 17 00:00:00 2001 From: Scott Haseley Date: Fri, 3 Dec 2021 20:27:38 +0000 Subject: [PATCH] [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 Reviewed-by: Alexander Timin Reviewed-by: Nico Weber Cr-Commit-Position: refs/heads/main@{#948132} --- ..._forward_cache_disabling_feature_tracker.h | 2 +- .../common/throttling/cpu_time_budget_pool.h | 2 +- .../scheduler/common/tracing_helper.cc | 20 ---- .../scheduler/common/tracing_helper.h | 108 ++++++++++-------- .../common/tracing_helper_unittest.cc | 26 ++--- .../main_thread/frame_scheduler_impl.h | 32 +++--- .../main_thread/main_thread_scheduler_impl.h | 65 +++++------ 7 files changed, 113 insertions(+), 142 deletions(-) diff --git a/third_party/blink/renderer/platform/scheduler/common/back_forward_cache_disabling_feature_tracker.h b/third_party/blink/renderer/platform/scheduler/common/back_forward_cache_disabling_feature_tracker.h index a8c997bc9d4fd..73e19a90c6496 100644 --- a/third_party/blink/renderer/platform/scheduler/common/back_forward_cache_disabling_feature_tracker.h +++ b/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(SchedulingPolicy::Feature::kMaxValue) + 1> back_forward_cache_disabling_features_{}; - TraceableState + TraceableState opted_out_from_back_forward_cache_; // The last set of features passed to FrameOrWorkerScheduler::Delegate:: diff --git a/third_party/blink/renderer/platform/scheduler/common/throttling/cpu_time_budget_pool.h b/third_party/blink/renderer/platform/scheduler/common/throttling/cpu_time_budget_pool.h index e9e6721570a28..b70640851b9dc 100644 --- a/third_party/blink/renderer/platform/scheduler/common/throttling/cpu_time_budget_pool.h +++ b/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 max_throttling_delay_; - TraceableCounter + TraceableCounter current_budget_level_; base::TimeTicks last_checkpoint_; double cpu_percentage_; diff --git a/third_party/blink/renderer/platform/scheduler/common/tracing_helper.cc b/third_party/blink/renderer/platform/scheduler/common/tracing_helper.cc index 804fc5b2f3f56..33cdfe94bf22f 100644 --- a/third_party/blink/renderer/platform/scheduler/common/tracing_helper.cc +++ b/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(); } diff --git a/third_party/blink/renderer/platform/scheduler/common/tracing_helper.h b/third_party/blink/renderer/platform/scheduler/common/tracing_helper.h index bcd92a996a292..d11d32021855a 100644 --- a/third_party/blink/renderer/platform/scheduler/common/tracing_helper.h +++ b/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 +template 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 +template class TraceableState : public TraceableVariable, private StateTracer { public: // Converter must return compile-time defined const strings because tracing @@ -230,35 +239,34 @@ class TraceableState : public TraceableVariable, private StateTracer { T state_; }; -template +template 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 using InitializeProtoFuncPtr = void (*)(perfetto::protos::pbzero::TrackEvent* event, T e); -template +template class TraceableObjectState : public TraceableVariable, public ProtoStateTracer> { @@ -342,7 +351,8 @@ 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; } @@ -350,7 +360,7 @@ class TraceableObjectState InitializeProtoFuncPtr proto_init_func_; }; -template +template 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 +template constexpr T operator-(const TraceableCounter& counter) { return -counter.value(); } -template +template constexpr T operator/(const TraceableCounter& lhs, const T& rhs) { return lhs.value() / rhs; } -template +template constexpr bool operator>(const TraceableCounter& lhs, const T& rhs) { return lhs.value() > rhs; } -template +template constexpr bool operator<(const TraceableCounter& lhs, const T& rhs) { return lhs.value() < rhs; } -template +template constexpr bool operator!=(const TraceableCounter& lhs, const T& rhs) { return lhs.value() != rhs; } -template +template constexpr T operator++(TraceableCounter& counter) { counter = counter.value() + 1; return counter.value(); } -template +template constexpr T operator--(TraceableCounter& counter) { counter = counter.value() - 1; return counter.value(); } -template +template constexpr T operator++(TraceableCounter& counter, int) { T value = counter.value(); counter = value + 1; return value; } -template +template constexpr T operator--(TraceableCounter& counter, int) { T value = counter.value(); counter = value - 1; diff --git a/third_party/blink/renderer/platform/scheduler/common/tracing_helper_unittest.cc b/third_party/blink/renderer/platform/scheduler/common/tracing_helper_unittest.cc index 2717b2e6e5a43..f13c817567045 100644 --- a/third_party/blink/renderer/platform/scheduler/common/tracing_helper_unittest.cc +++ b/third_party/blink/renderer/platform/scheduler/common/tracing_helper_unittest.cc @@ -34,7 +34,7 @@ const char* SignOfInt(int value) { } class TraceableStateForTest - : public TraceableState { + : public TraceableState { 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 x(-1, "X", &controller, - SignOfInt); - TraceableState y(1, "Y", &controller, - SignOfInt); + TraceableState x(-1, "X", &controller, + SignOfInt); + TraceableState y(1, "Y", &controller, + SignOfInt); EXPECT_EQ(0, x + y); EXPECT_FALSE(x == y); EXPECT_TRUE(x != y); diff --git a/third_party/blink/renderer/platform/scheduler/main_thread/frame_scheduler_impl.h b/third_party/blink/renderer/platform/scheduler/main_thread/frame_scheduler_impl.h index 650bcb166ec00..c2b0b8576d4f4 100644 --- a/third_party/blink/renderer/platform/scheduler/main_thread/frame_scheduler_impl.h +++ b/third_party/blink/renderer/platform/scheduler/main_thread/frame_scheduler_impl.h @@ -327,19 +327,18 @@ class PLATFORM_EXPORT FrameSchedulerImpl : public FrameScheduler, FrameScheduler::Delegate* delegate_; // NOT OWNED base::trace_event::BlameContext* blame_context_; // NOT OWNED SchedulingLifecycleState throttling_state_; - TraceableState frame_visible_; - TraceableState frame_paused_; - TraceableState - frame_origin_type_; - TraceableState subresource_loading_paused_; - StateTracer url_tracer_; - TraceableState task_queues_throttled_; + TraceableState frame_visible_; + TraceableState frame_paused_; + TraceableState frame_origin_type_; + TraceableState subresource_loading_paused_; + StateTracer url_tracer_; + TraceableState task_queues_throttled_; Vector throttled_task_queue_handles_; - TraceableState + TraceableState preempted_for_cooperative_scheduling_; // TODO(https://crbug.com/827113): Trace the count of opt-outs. int aggressive_throttling_opt_out_count_; - TraceableState + TraceableState opted_out_from_aggressive_throttling_; size_t subresource_loading_pause_count_; @@ -353,17 +352,14 @@ class PLATFORM_EXPORT FrameSchedulerImpl : public FrameScheduler, // These are the states of the Page. // They should be accessed via GetPageScheduler()->SetPageState(). // they are here because we don't support page-level tracing yet. - TraceableState page_frozen_for_tracing_; - TraceableState + TraceableState page_frozen_for_tracing_; + TraceableState page_visibility_for_tracing_; - TraceableState - waiting_for_dom_content_loaded_; - TraceableState - waiting_for_contentful_paint_; - TraceableState - waiting_for_meaningful_paint_; - TraceableState waiting_for_load_; + TraceableState waiting_for_dom_content_loaded_; + TraceableState waiting_for_contentful_paint_; + TraceableState waiting_for_meaningful_paint_; + TraceableState waiting_for_load_; std::unique_ptr loading_power_mode_voter_; diff --git a/third_party/blink/renderer/platform/scheduler/main_thread/main_thread_scheduler_impl.h b/third_party/blink/renderer/platform/scheduler/main_thread/main_thread_scheduler_impl.h index 41d76f6eb3038..4a7681f39b715 100644 --- a/third_party/blink/renderer/platform/scheduler/main_thread/main_thread_scheduler_impl.h +++ b/third_party/blink/renderer/platform/scheduler/main_thread/main_thread_scheduler_impl.h @@ -820,54 +820,53 @@ class PLATFORM_EXPORT MainThreadSchedulerImpl bool IsInNestedRunloop(); IdleTimeEstimator idle_time_estimator; - TraceableState current_use_case; + TraceableState current_use_case; Policy current_policy; base::TimeTicks current_policy_expiration_time; base::TimeTicks estimated_next_frame_begin; base::TimeTicks current_task_start_time; base::TimeDelta compositor_frame_interval; - TraceableCounter + TraceableCounter longest_jank_free_task_duration; - TraceableCounter + TraceableCounter renderer_pause_count; // Renderer is paused if non-zero. TraceableObjectState + TracingCategory::kTopLevel> rail_mode_for_tracing; // Don't use except for tracing. - TraceableObjectState renderer_hidden; + TraceableObjectState renderer_hidden; absl::optional renderer_hidden_metadata; - TraceableObjectState + TraceableObjectState renderer_backgrounded; - TraceableState + TraceableState blocking_input_expected_soon; - TraceableState + TraceableState have_reported_blocking_intervention_in_current_policy; - TraceableState + TraceableState have_reported_blocking_intervention_since_navigation; - TraceableState + TraceableState has_visible_render_widget_with_touch_handler; - TraceableState - in_idle_period_for_testing; - TraceableState use_virtual_time; - TraceableState is_audio_playing; - TraceableState + TraceableState in_idle_period_for_testing; + TraceableState use_virtual_time; + TraceableState is_audio_playing; + TraceableState compositor_will_send_main_frame_not_expected; - TraceableState has_navigated; - TraceableState pause_timers_for_webview; + TraceableState has_navigated; + TraceableState pause_timers_for_webview; base::TimeTicks background_status_changed_at; HashSet page_schedulers; // Not owned. base::ObserverList::Unchecked rail_mode_observers; // Not owned. MainThreadMetricsHelper metrics_helper; - TraceableState + TraceableState process_type; TraceableState, - TracingCategoryName::kInfo> + TracingCategory::kInfo> task_description_for_tracing; // Don't use except for tracing. TraceableState< absl::optional, - TracingCategoryName::kInfo> + TracingCategory::kInfo> task_priority_for_tracing; // Only used for tracing. base::Time initial_virtual_time; base::TimeTicks initial_virtual_time_ticks; @@ -898,7 +897,7 @@ class PLATFORM_EXPORT MainThreadSchedulerImpl // High-priority for compositing events after input. This will cause // compositing events get a higher priority until the start of the next // animation frame. - TraceableState + TraceableState prioritize_compositing_after_input; // List of callbacks to execute after the current task. @@ -910,7 +909,7 @@ class PLATFORM_EXPORT MainThreadSchedulerImpl // kNormalPriority and is updated via UpdateCompositorTaskQueuePriority(). // After 100ms with nothing running from this queue, the compositor will // be set to kVeryHighPriority until a frame is run. - TraceableState + TraceableState compositor_priority; base::TimeTicks last_frame_time; bool should_prioritize_compositor_task_queue_after_delay; @@ -929,23 +928,21 @@ class PLATFORM_EXPORT MainThreadSchedulerImpl base::TimeTicks last_idle_period_end_time; base::TimeTicks fling_compositor_escalation_deadline; UserModel user_model; - TraceableState - awaiting_touch_start_response; - TraceableState in_idle_period; - TraceableState + TraceableState awaiting_touch_start_response; + TraceableState in_idle_period; + TraceableState begin_main_frame_on_critical_path; - TraceableState + TraceableState last_gesture_was_compositor_driven; - TraceableState default_gesture_prevented; - TraceableState - have_seen_a_blocking_gesture; - TraceableState + TraceableState default_gesture_prevented; + TraceableState have_seen_a_blocking_gesture; + TraceableState waiting_for_any_main_frame_contentful_paint; - TraceableState + TraceableState waiting_for_any_main_frame_meaningful_paint; - TraceableState + TraceableState have_seen_input_since_navigation; - TraceableCounter + TraceableCounter begin_main_frame_scheduled_count; };