tp: Add RegisterSqlPackage() and start sunsetting RegisterSqlModule()

Change-Id: I98fa82c09d22139381fb066364f8cad39cc3d40d

CHANGELOG

@@ -6,9 +6,18 @@ Unreleased:
     * Increased watchdog timeout to 180s from 30s to make watchdog crashes
       much less likely when system is under heavy load.
   SQL Standard library:
-    *
+    * Improved CPU cycles calculation in `linux.cpu.utilization` modules:
+     `process`, `system` and `thread` by fixing a bug responsible for too high
+      CPU cycles values.
+    * Introduces functions responsible for calculating CPU cycles with
+      breakdown by CPU, thread, process and slice for a given interval.
   Trace Processor:
-    *
+    * Renamed Trace Processor's C++ method `RegisterSqlModule()` to
+     `RegisterSqlPackage()`, which better represents the module/package
+      relationship. Package is the top level grouping of modules, which are
+      objects being included with `INCLUDE PERFETTO MODULE`.
+      `RegisterSqlModule()` is still available and runs `RegisterSqlPackage()`.
+      `RegisterSqlModule()` will be deprecated in v50.0.
   UI:
     * Scheduling wakeup information now reflects whether the wakeup came
       from an interrupt context. The per-cpu scheduling tracks now show only

include/perfetto/trace_processor/basic_types.h

@@ -263,9 +263,9 @@ struct PERFETTO_EXPORT_COMPONENT SqlValue {
 };
 
 // Data used to register a new SQL package.
-struct SqlModule {
+struct SqlPackage {
   // Must be unique among modules, or can be used to override existing module if
-  // |allow_module_override| is set.
+  // |allow_override| is set.
   std::string name;
 
   // Pairs of strings used for |INCLUDE| with the contents of SQL files being
@@ -276,10 +276,18 @@ struct SqlModule {
   // run, with slashes replaced by dots and without the SQL extension. For
   // example, 'android/camera/jank.sql' would be included by
   // 'android.camera.jank'.
-  std::vector<std::pair<std::string, std::string>> files;
+  std::vector<std::pair<std::string, std::string>> modules;
 
   // If true, SqlPackage will override registered module with the same name. Can
   // only be set if enable_dev_features is true, otherwise will throw an error.
+  bool allow_override = false;
+};
+
+// Deprecated. Use only with |trace_processor->RegisterSqlModule()|. Alias of
+// |SqlPackage|.
+struct SqlModule {
+  std::string name;
+  std::vector<std::pair<std::string, std::string>> files;
   bool allow_module_override = false;
 };
 

include/perfetto/trace_processor/trace_processor.h

@@ -63,7 +63,7 @@ class PERFETTO_EXPORT_COMPONENT TraceProcessor : public TraceProcessorStorage {
   // PERFETTO MODULE camera.cpu.metrics". The first word of the string has to be
   // a package name and there can be only one package registered with a given
   // name.
-  virtual base::Status RegisterSqlModule(SqlModule) = 0;
+  virtual base::Status RegisterSqlPackage(SqlPackage) = 0;
 
   // Registers a metric at the given path which will run the specified SQL.
   virtual base::Status RegisterMetric(const std::string& path,
@@ -138,6 +138,11 @@ class PERFETTO_EXPORT_COMPONENT TraceProcessor : public TraceProcessorStorage {
   // loaded by trace processor shell at runtime. The message is encoded as
   // DescriptorSet, defined in perfetto/trace_processor/trace_processor.proto.
   virtual std::vector<uint8_t> GetMetricDescriptors() = 0;
+
+  // Deprecated. Use |RegisterSqlPackage()| instead, which is identical in
+  // functionality to |RegisterSqlModule()| and the only difference is in
+  // the argument, which is directly translatable to |SqlPackage|.
+  virtual base::Status RegisterSqlModule(SqlModule) = 0;
 };
 
 }  // namespace trace_processor

protos/perfetto/trace_processor/trace_processor.proto

@@ -88,21 +88,23 @@ message TraceProcessorRpc {
   optional string fatal_error = 5;
 
   enum TraceProcessorMethod {
+    reserved 4, 12;
+    reserved "TPM_QUERY_RAW_DEPRECATED";
+    reserved "TPM_REGISTER_SQL_MODULE";
+
     TPM_UNSPECIFIED = 0;
     TPM_APPEND_TRACE_DATA = 1;
     TPM_FINALIZE_TRACE_DATA = 2;
     TPM_QUERY_STREAMING = 3;
     // Previously: TPM_QUERY_RAW_DEPRECATED
-    reserved 4;
-    reserved "TPM_QUERY_RAW_DEPRECATED";
     TPM_COMPUTE_METRIC = 5;
     TPM_GET_METRIC_DESCRIPTORS = 6;
     TPM_RESTORE_INITIAL_TABLES = 7;
     TPM_ENABLE_METATRACE = 8;
     TPM_DISABLE_AND_READ_METATRACE = 9;
     TPM_GET_STATUS = 10;
     TPM_RESET_TRACE_PROCESSOR = 11;
-    TPM_REGISTER_SQL_MODULE = 12;
+    TPM_REGISTER_SQL_PACKAGE = 13;
   }
 
   oneof type {
@@ -134,8 +136,8 @@ message TraceProcessorRpc {
     EnableMetatraceArgs enable_metatrace_args = 106;
     // For TPM_RESET_TRACE_PROCESSOR.
     ResetTraceProcessorArgs reset_trace_processor_args = 107;
-    // For TPM_REGISTER_SQL_MODULE.
-    RegisterSqlModuleArgs register_sql_module_args = 108;
+    // For TPM_REGISTER_SQL_PACKAGE.
+    RegisterSqlPackageArgs register_sql_package_args = 108;
 
     // TraceProcessorMethod response args.
     // For TPM_APPEND_TRACE_DATA.
@@ -150,8 +152,8 @@ message TraceProcessorRpc {
     DisableAndReadMetatraceResult metatrace = 209;
     // For TPM_GET_STATUS.
     StatusResult status = 210;
-    // For TPM_REGISTER_SQL_MODULE.
-    RegisterSqlModuleResult register_sql_module_result = 211;
+    // For TPM_REGISTER_SQL_PACKAGE.
+    RegisterSqlPackageResult register_sql_package_result = 211;
   }
 
   // Previously: RawQueryArgs for TPM_QUERY_RAW_DEPRECATED
@@ -335,15 +337,16 @@ message ResetTraceProcessorArgs {
   optional bool ftrace_drop_until_all_cpus_valid = 4;
 }
 
-message RegisterSqlModuleArgs {
+message RegisterSqlPackageArgs {
   message Module {
     optional string name = 1;
     optional string sql = 2;
   }
-  optional string top_level_package_name = 1;
+  optional string package_name = 1;
   repeated Module modules = 2;
-  optional bool allow_module_override = 3;
+  optional bool allow_override = 3;
 }
-message RegisterSqlModuleResult {
+
+message RegisterSqlPackageResult {
   optional string error = 1;
 }

src/trace_processor/rpc/rpc.cc

@@ -324,10 +324,10 @@ void Rpc::ParseRpcRequest(const uint8_t* data, size_t len) {
       resp.Send(rpc_response_fn_);
       break;
     }
-    case RpcProto::TPM_REGISTER_SQL_MODULE: {
+    case RpcProto::TPM_REGISTER_SQL_PACKAGE: {
       Response resp(tx_seq_id_++, req_type);
-      base::Status status = RegisterSqlModule(req.register_sql_module_args());
-      auto* res = resp->set_register_sql_module_result();
+      base::Status status = RegisterSqlPackage(req.register_sql_package_args());
+      auto* res = resp->set_register_sql_package_result();
       if (!status.ok()) {
         res->set_error(status.message());
       }
@@ -410,16 +410,16 @@ void Rpc::ResetTraceProcessor(const uint8_t* args, size_t len) {
   ResetTraceProcessorInternal(config);
 }
 
-base::Status Rpc::RegisterSqlModule(protozero::ConstBytes bytes) {
-  protos::pbzero::RegisterSqlModuleArgs::Decoder args(bytes);
-  SqlModule package;
-  package.name = args.top_level_package_name().ToStdString();
-  package.allow_module_override = args.allow_module_override();
+base::Status Rpc::RegisterSqlPackage(protozero::ConstBytes bytes) {
+  protos::pbzero::RegisterSqlPackageArgs::Decoder args(bytes);
+  SqlPackage package;
+  package.name = args.package_name().ToStdString();
+  package.allow_override = args.allow_override();
   for (auto it = args.modules(); it; ++it) {
-    protos::pbzero::RegisterSqlModuleArgs::Module::Decoder m(*it);
-    package.files.emplace_back(m.name().ToStdString(), m.sql().ToStdString());
+    protos::pbzero::RegisterSqlPackageArgs::Module::Decoder m(*it);
+    package.modules.emplace_back(m.name().ToStdString(), m.sql().ToStdString());
   }
-  return trace_processor_->RegisterSqlModule(package);
+  return trace_processor_->RegisterSqlPackage(package);
 }
 
 void Rpc::MaybePrintProgress() {

src/trace_processor/rpc/rpc.h

@@ -131,7 +131,7 @@ class Rpc {
  private:
   void ParseRpcRequest(const uint8_t*, size_t);
   void ResetTraceProcessor(const uint8_t*, size_t);
-  base::Status RegisterSqlModule(protozero::ConstBytes);
+  base::Status RegisterSqlPackage(protozero::ConstBytes);
   void ResetTraceProcessorInternal(const Config&);
   void MaybePrintProgress();
   Iterator QueryInternal(const uint8_t*, size_t);

src/trace_processor/trace_database_integrationtest.cc

@@ -767,11 +767,11 @@ no such column: t)");
 }
 
 TEST_F(TraceProcessorIntegrationTest, ErrorMessageModule) {
-  SqlModule module;
+  SqlPackage module;
   module.name = "foo";
-  module.files.push_back(std::make_pair("foo.bar", "select t from slice"));
+  module.modules.push_back(std::make_pair("foo.bar", "select t from slice"));
 
-  ASSERT_TRUE(Processor()->RegisterSqlModule(module).ok());
+  ASSERT_TRUE(Processor()->RegisterSqlPackage(module).ok());
 
   auto it = Query("include perfetto module foo.bar;");
   ASSERT_FALSE(it.Next());

src/trace_processor/trace_processor_impl.cc

@@ -586,10 +586,10 @@ bool TraceProcessorImpl::IsRootMetricField(const std::string& metric_name) {
   return field_idx != nullptr;
 }
 
-base::Status TraceProcessorImpl::RegisterSqlModule(SqlModule sql_package) {
+base::Status TraceProcessorImpl::RegisterSqlPackage(SqlPackage sql_package) {
   sql_modules::RegisteredPackage new_package;
   std::string name = sql_package.name;
-  if (engine_->FindPackage(name) && !sql_package.allow_module_override) {
+  if (engine_->FindPackage(name) && !sql_package.allow_override) {
     return base::ErrStatus(
         "Package '%s' is already registered. Choose a different name.\n"
         "If you want to replace the existing package using trace processor "
@@ -598,7 +598,7 @@ base::Status TraceProcessorImpl::RegisterSqlModule(SqlModule sql_package) {
         "to pass the module path.",
         name.c_str());
   }
-  for (auto const& module_name_and_sql : sql_package.files) {
+  for (auto const& module_name_and_sql : sql_package.modules) {
     if (sql_modules::GetPackageName(module_name_and_sql.first) != name) {
       return base::ErrStatus(
           "Module name doesn't match the package name. First part of module "
@@ -608,7 +608,7 @@ base::Status TraceProcessorImpl::RegisterSqlModule(SqlModule sql_package) {
     new_package.modules.Insert(module_name_and_sql.first,
                                {module_name_and_sql.second, false});
   }
-  manually_registered_sql_packages_.push_back(SqlModule(sql_package));
+  manually_registered_sql_packages_.push_back(SqlPackage(sql_package));
   engine_->RegisterPackage(name, std::move(new_package));
   return base::OkStatus();
 }
@@ -861,8 +861,8 @@ void TraceProcessorImpl::InitPerfettoSqlEngine() {
   auto packages = GetStdlibPackages();
   for (auto package = packages.GetIterator(); package; ++package) {
     base::Status status =
-        RegisterSqlModule({/*name=*/package.key(), /*modules=*/package.value(),
-                           /*allow_package_override=*/false});
+        RegisterSqlPackage({/*name=*/package.key(), /*modules=*/package.value(),
+                            /*allow_override=*/false});
     if (!status.ok())
       PERFETTO_ELOG("%s", status.c_message());
   }
@@ -1088,7 +1088,7 @@ void TraceProcessorImpl::InitPerfettoSqlEngine() {
 
   // Reregister manually added stdlib packages.
   for (const auto& package : manually_registered_sql_packages_) {
-    RegisterSqlModule(package);
+    RegisterSqlPackage(package);
   }
 }
 

src/trace_processor/trace_processor_impl.h

@@ -67,7 +67,7 @@ class TraceProcessorImpl : public TraceProcessor,
   base::Status RegisterMetric(const std::string& path,
                               const std::string& sql) override;
 
-  base::Status RegisterSqlModule(SqlModule) override;
+  base::Status RegisterSqlPackage(SqlPackage) override;
 
   base::Status ExtendMetricsProto(const uint8_t* data, size_t size) override;
 
@@ -97,6 +97,15 @@ class TraceProcessorImpl : public TraceProcessor,
   base::Status DisableAndReadMetatrace(
       std::vector<uint8_t>* trace_proto) override;
 
+  base::Status RegisterSqlModule(SqlModule module) override {
+    SqlPackage package;
+    package.name = std::move(module.name);
+    package.modules = std::move(module.files);
+    package.allow_override = module.allow_module_override;
+
+    return RegisterSqlPackage(package);
+  }
+
  private:
   // Needed for iterators to be able to access the context.
   friend class IteratorImpl;
@@ -120,7 +129,7 @@ class TraceProcessorImpl : public TraceProcessor,
 
   // Manually registeres SQL packages are stored here, to be able to restore
   // them when running |RestoreInitialTables()|.
-  std::vector<SqlModule> manually_registered_sql_packages_;
+  std::vector<SqlPackage> manually_registered_sql_packages_;
 
   std::unordered_map<std::string, std::string> proto_field_to_sql_metric_path_;
   std::unordered_map<std::string, std::string> proto_fn_name_to_path_;

src/trace_processor/trace_processor_shell.cc

@@ -1256,9 +1256,9 @@ base::Status IncludeSqlModule(std::string root, bool allow_override) {
         .first->push_back({import_key, file_contents});
   }
   for (auto module_it = modules.GetIterator(); module_it; ++module_it) {
-    auto status = g_tp->RegisterSqlModule(
-        {/*name=*/module_it.key(), /*files=*/module_it.value(),
-         /*allow_module_override=*/allow_override});
+    auto status = g_tp->RegisterSqlPackage({/*name=*/module_it.key(),
+                                            /*files=*/module_it.value(),
+                                            /*allow_override=*/allow_override});
     if (!status.ok())
       return status;
   }
@@ -1294,9 +1294,9 @@ base::Status LoadOverridenStdlib(std::string root) {
         .first->push_back({module_name, module_file});
   }
   for (auto package = packages.GetIterator(); package; ++package) {
-    g_tp->RegisterSqlModule({/*name=*/package.key(),
-                             /*files=*/package.value(),
-                             /*allow_module_override=*/true});
+    g_tp->RegisterSqlPackage({/*name=*/package.key(),
+                              /*files=*/package.value(),
+                              /*allow_override=*/true});
   }
 
   return base::OkStatus();

ui/src/controller/trace_controller.ts

@@ -299,7 +299,7 @@ async function loadTraceIntoEngine(
     await engine.restoreInitialTables();
   }
   for (const p of globals.extraSqlPackages) {
-    await engine.registerSqlModules(p);
+    await engine.registerSqlPackages(p);
   }
 
   const traceDetails = await getTraceInfo(engine, traceSource);

ui/src/protos/index.ts

@@ -71,8 +71,8 @@ import QueryServiceStateRequest = protos.perfetto.protos.QueryServiceStateReques
 import QueryServiceStateResponse = protos.perfetto.protos.QueryServiceStateResponse;
 import ReadBuffersRequest = protos.perfetto.protos.ReadBuffersRequest;
 import ReadBuffersResponse = protos.perfetto.protos.ReadBuffersResponse;
-import RegisterSqlModuleArgs = protos.perfetto.protos.RegisterSqlModuleArgs;
-import RegisterSqlModuleResult = protos.perfetto.protos.RegisterSqlModuleResult;
+import RegisterSqlPackageArgs = protos.perfetto.protos.RegisterSqlPackageArgs;
+import RegisterSqlPackageResult = protos.perfetto.protos.RegisterSqlPackageResult;
 import ResetTraceProcessorArgs = protos.perfetto.protos.ResetTraceProcessorArgs;
 import StatCounters = protos.perfetto.protos.SysStatsConfig.StatCounters;
 import StatusResult = protos.perfetto.protos.StatusResult;
@@ -140,8 +140,8 @@ export {
   QueryServiceStateResponse,
   ReadBuffersRequest,
   ReadBuffersResponse,
-  RegisterSqlModuleArgs,
-  RegisterSqlModuleResult,
+  RegisterSqlPackageArgs,
+  RegisterSqlPackageResult,
   ResetTraceProcessorArgs,
   StatCounters,
   StatusResult,