diff --git a/common/settings.h b/common/settings.h index d5304e47681f6..e3ad8589443f1 100644 --- a/common/settings.h +++ b/common/settings.h @@ -104,7 +104,8 @@ struct Settings { bool enable_checked_mode = false; bool start_paused = false; bool trace_skia = false; - std::string trace_allowlist; + std::vector trace_allowlist; + std::vector trace_skia_allowlist; bool trace_startup = false; bool trace_systrace = false; bool dump_skp_on_shader_compilation = false; diff --git a/shell/common/shell.cc b/shell/common/shell.cc index 35180e27a43e5..90dd535563b0c 100644 --- a/shell/common/shell.cc +++ b/shell/common/shell.cc @@ -68,16 +68,6 @@ std::unique_ptr CreateEngine( volatile_path_tracker); } -void Tokenize(const std::string& input, - std::vector* results, - char delimiter) { - std::istringstream ss(input); - std::string token; - while (std::getline(ss, token, delimiter)) { - results->push_back(token); - } -} - // Though there can be multiple shells, some settings apply to all components in // the process. These have to be set up before the shell or any of its // sub-components can be initialized. In a perfect world, this would be empty. @@ -103,13 +93,11 @@ void PerformInitializationTasks(Settings& settings) { [](const char* message) { FML_LOG(ERROR) << message; }); if (settings.trace_skia) { - InitSkiaEventTracer(settings.trace_skia); + InitSkiaEventTracer(settings.trace_skia, settings.trace_skia_allowlist); } if (!settings.trace_allowlist.empty()) { - std::vector prefixes; - Tokenize(settings.trace_allowlist, &prefixes, ','); - fml::tracing::TraceSetAllowlist(prefixes); + fml::tracing::TraceSetAllowlist(settings.trace_allowlist); } if (!settings.skia_deterministic_rendering_on_cpu) { diff --git a/shell/common/skia_event_tracer_impl.cc b/shell/common/skia_event_tracer_impl.cc index 053df0993c33b..79723c618c9c9 100644 --- a/shell/common/skia_event_tracer_impl.cc +++ b/shell/common/skia_event_tracer_impl.cc @@ -5,6 +5,8 @@ #include "flutter/shell/common/skia_event_tracer_impl.h" #define TRACE_EVENT_HIDE_MACROS +#include +#include #include #include "flutter/fml/logging.h" @@ -42,6 +44,10 @@ namespace flutter { namespace { +// Skia prepends this string to the category names of its trace events. +// Defined in Skia's src/core/SkTraceEvent.h. +constexpr std::string_view kTraceCategoryPrefix = "disabled-by-default-"; + #if defined(OS_FUCHSIA) template inline T BitCast(const U& u) { @@ -61,7 +67,12 @@ class FlutterEventTracer : public SkEventTracer { static constexpr uint8_t kYes = 1; static constexpr uint8_t kNo = 0; - FlutterEventTracer(bool enabled) : enabled_(enabled ? kYes : kNo){}; + FlutterEventTracer(bool enabled, const std::vector& allowlist) + : enabled_(enabled ? kYes : kNo) { + for (const std::string& category : allowlist) { + allowlist_.insert(std::string(kTraceCategoryPrefix) + category); + } + }; SkEventTracer::Handle addTraceEvent(char phase, const uint8_t* category_enabled_flag, @@ -221,39 +232,43 @@ class FlutterEventTracer : public SkEventTracer { } const uint8_t* getCategoryGroupEnabled(const char* name) override { - return &enabled_; + // Skia will only use long-lived string literals as event names. + auto flag_it = category_flag_map_.find(name); + if (flag_it == category_flag_map_.end()) { + bool allowed; + if (enabled_) { + allowed = + allowlist_.empty() || allowlist_.find(name) != allowlist_.end(); + } else { + allowed = false; + } + flag_it = category_flag_map_.insert(std::make_pair(name, allowed)).first; + reverse_flag_map_.insert(std::make_pair(&flag_it->second, name)); + } + return &flag_it->second; } const char* getCategoryGroupName( const uint8_t* category_enabled_flag) override { - return kSkiaTag; + auto reverse_it = reverse_flag_map_.find(category_enabled_flag); + if (reverse_it != reverse_flag_map_.end()) { + return reverse_it->second; + } else { + return kSkiaTag; + } } - void enable() { enabled_ = kYes; } - private: uint8_t enabled_; + std::set allowlist_; + std::map category_flag_map_; + std::map reverse_flag_map_; FML_DISALLOW_COPY_AND_ASSIGN(FlutterEventTracer); }; -bool enableSkiaTracingCallback(const char* method, - const char** param_keys, - const char** param_values, - intptr_t num_params, - void* user_data, - const char** json_object) { - FlutterEventTracer* tracer = static_cast(user_data); - tracer->enable(); - *json_object = fml::strdup("{\"type\":\"Success\"}"); - return true; -} - -void InitSkiaEventTracer(bool enabled) { - // TODO(chinmaygarde): Leaked https://github.com/flutter/flutter/issues/30808. - auto tracer = new FlutterEventTracer(enabled); - Dart_RegisterRootServiceRequestCallback("_flutter.enableSkiaTracing", - enableSkiaTracingCallback, - static_cast(tracer)); +void InitSkiaEventTracer(bool enabled, + const std::vector& allowlist) { + auto tracer = new FlutterEventTracer(enabled, allowlist); // Initialize the binding to Skia's tracing events. Skia will // take ownership of and clean up the memory allocated here. SkEventTracer::SetInstance(tracer); diff --git a/shell/common/skia_event_tracer_impl.h b/shell/common/skia_event_tracer_impl.h index 02f4e25d17a9f..cdd66f095d2c8 100644 --- a/shell/common/skia_event_tracer_impl.h +++ b/shell/common/skia_event_tracer_impl.h @@ -5,9 +5,13 @@ #ifndef FLUTTER_SHELL_COMMON_SKIA_EVENT_TRACER_IMPL_H_ #define FLUTTER_SHELL_COMMON_SKIA_EVENT_TRACER_IMPL_H_ +#include +#include + namespace flutter { -void InitSkiaEventTracer(bool enabled); +void InitSkiaEventTracer(bool enabled, + const std::vector& allowlist); } // namespace flutter diff --git a/shell/common/switches.cc b/shell/common/switches.cc index b5a15b1e1e2a1..240ebf074c343 100644 --- a/shell/common/switches.cc +++ b/shell/common/switches.cc @@ -155,6 +155,16 @@ const std::string_view FlagForSwitch(Switch swtch) { return std::string_view(); } +static std::vector ParseCommaDelimited(const std::string& input) { + std::istringstream ss(input); + std::vector result; + std::string token; + while (std::getline(ss, token, ',')) { + result.push_back(token); + } + return result; +} + static bool IsAllowedDartVMFlag(const std::string& flag) { for (uint32_t i = 0; i < fml::size(gAllowedDartFlags); ++i) { const std::string& allowed = gAllowedDartFlags[i]; @@ -289,17 +299,15 @@ Settings SettingsFromCommandLine(const fml::CommandLine& command_line) { settings.trace_skia = command_line.HasOption(FlagForSwitch(Switch::TraceSkia)); - command_line.GetOptionValue(FlagForSwitch(Switch::TraceAllowlist), - &settings.trace_allowlist); + std::string trace_skia_allowlist; + command_line.GetOptionValue(FlagForSwitch(Switch::TraceSkiaAllowlist), + &trace_skia_allowlist); + settings.trace_skia_allowlist = ParseCommaDelimited(trace_skia_allowlist); - if (settings.trace_allowlist.empty()) { - command_line.GetOptionValue(FlagForSwitch(Switch::TraceWhitelist), - &settings.trace_allowlist); - if (!settings.trace_allowlist.empty()) { - FML_LOG(INFO) - << "--trace-whitelist is deprecated. Use --trace-allowlist instead."; - } - } + std::string trace_allowlist; + command_line.GetOptionValue(FlagForSwitch(Switch::TraceAllowlist), + &trace_allowlist); + settings.trace_allowlist = ParseCommaDelimited(trace_allowlist); settings.trace_systrace = command_line.HasOption(FlagForSwitch(Switch::TraceSystrace)); @@ -384,11 +392,9 @@ Settings SettingsFromCommandLine(const fml::CommandLine& command_line) { std::string all_dart_flags; if (command_line.GetOptionValue(FlagForSwitch(Switch::DartFlags), &all_dart_flags)) { - std::stringstream stream(all_dart_flags); - std::string flag; - // Assume that individual flags are comma separated. - while (std::getline(stream, flag, ',')) { + std::vector flags = ParseCommaDelimited(all_dart_flags); + for (auto flag : flags) { if (!IsAllowedDartVMFlag(flag)) { FML_LOG(FATAL) << "Encountered disallowed Dart VM flag: " << flag; } diff --git a/shell/common/switches.h b/shell/common/switches.h index 08399b35129ad..5d3e05485c8bd 100644 --- a/shell/common/switches.h +++ b/shell/common/switches.h @@ -132,9 +132,10 @@ DEF_SWITCH(TraceSkia, "Trace Skia calls. This is useful when debugging the GPU threed." "By default, Skia tracing is not enabled to reduce the number of " "traced events") -DEF_SWITCH(TraceWhitelist, - "trace-whitelist", - "(deprecated) Use --trace-allowlist instead.") +DEF_SWITCH(TraceSkiaAllowlist, + "trace-skia-allowlist", + "Filters out all Skia trace event categories except those that are " + "specified in this comma separated list.") DEF_SWITCH( TraceAllowlist, "trace-allowlist",