Merge "tp: add option to force sorting based on flush period"

6 files changed, 104 insertions, 32 deletions

diff --git a/CHANGELOG b/CHANGELOG
index 53d428500..dc95ad2bd 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -2,7 +2,9 @@ Unreleased:
   Tracing service and probes:
     *
   Trace Processor:
-    *
+    * Added sorting mode to trace processor config to specify how trace
+      processor should sort events. The |force_full_sort| flag has been
+      deprecated (with replacement) and will be removed in the next version.
   UI:
     *
   SDK:
diff --git a/include/perfetto/trace_processor/basic_types.h b/include/perfetto/trace_processor/basic_types.h
index 53774e370..58e614786 100644
--- a/include/perfetto/trace_processor/basic_types.h
+++ b/include/perfetto/trace_processor/basic_types.h
@@ -34,13 +34,43 @@ namespace trace_processor {
 // simpler (e.g. use arrays instead of vectors).
 constexpr size_t kMaxCpus = 128;
 
+// Enum which encodes how trace processor should try to sort the ingested data.
+enum class SortingMode {
+  // This option allows trace processor to use built-in heuristics about how to
+  // sort the data. Generally, this option is correct for most embedders as
+  // trace processor reads information from the trace (e.g. TraceConfig) to make
+  // the best decision.
+  //
+  // The exact heuristics are implementation details but will ensure that all
+  // relevant tables are sorted by timestamp.
+  //
+  // This is the default mode.
+  kDefaultHeureustics = 0,
+
+  // This option forces trace processor to wait for all trace packets to be
+  // passed to it before doing a full sort of all the packets. This causes any
+  // heuristics trace processor would normally use to ingest partially sorted
+  // data to be skipped.
+  kForceFullSort = 1,
+
+  // This option forces trace processor to use the |flush_period_ms| specified
+  // in the TraceConfig to perform a windowed sort of the data. The window size
+  // is not guaranteed to be exactly |flush_period_ms| but will be of the same
+  // order of magnitude; the exact value is an implementation detail and should
+  // not be relied upon.
+  //
+  // If a |flush_period_ms| is not specified in the TraceConfig, this mode will
+  // act the same as |SortingMode::kDefaultHeureustics|.
+  kForceFlushPeriodWindowedSort = 2
+};
+
 // Enum which encodes which event (if any) should be used to drop ftrace data
 // from before this timestamp of that event.
 enum class DropFtraceDataBefore {
   // Drops ftrace data before timestmap specified by the
-  // TracingServiceEvent::tracing_started. If this packet is not in the trace,
-  // no data is dropped.
-  // Note: this event was introduced in S+ so will no data will be dropped on R-
+  // TracingServiceEvent::tracing_started packet. If this packet is not in the
+  // trace, no data is dropped.
+  // Note: this event was introduced in S+ so no data will be dropped on R-
   // traces.
   // This is the default approach.
   kTracingStarted = 0,
@@ -59,9 +89,16 @@ enum class DropFtraceDataBefore {
 // Struct for configuring a TraceProcessor instance (see trace_processor.h).
 struct PERFETTO_EXPORT Config {
   // When set to true, this option forces trace processor to perform a full
-  // sort ignoring any internal heureustics to skip sorting parts of the data.
+  // sort ignoring any internal heuristics to skip sorting parts of the data.
+  // This option was deprecated in v13 and replaced by
+  // setting |sorting_mode| to |SortingMode::kForceFullSort|
+  // This is option is scheduled to be removed in v14.
   bool force_full_sort = false;
 
+  // Indicates the sortinng mode that trace processor should use on the passed
+  // trace packets. See the enum documentation for more details.
+  SortingMode sorting_mode = SortingMode::kDefaultHeureustics;
+
   // When set to false, this option makes the trace processor not include ftrace
   // events in the raw table; this makes converting events back to the systrace
   // text format impossible. On the other hand, it also saves ~50% of memory
diff --git a/src/trace_processor/importers/proto/proto_trace_reader.cc b/src/trace_processor/importers/proto/proto_trace_reader.cc
index 3ccfc8b5e..09125a6dd 100644
--- a/src/trace_processor/importers/proto/proto_trace_reader.cc
+++ b/src/trace_processor/importers/proto/proto_trace_reader.cc
@@ -223,32 +223,8 @@ util::Status ProtoTraceReader::ParsePacket(TraceBlobView packet) {
     }
   }
 
-  // If we're not forcing a full sort and this is a write_into_file trace, then
-  // use flush_period_ms as an indiciator for how big the sliding window for the
-  // sorter should be.
-  if (!context_->config.force_full_sort && decoder.has_trace_config()) {
-    auto config = decoder.trace_config();
-    protos::pbzero::TraceConfig::Decoder trace_config(config.data, config.size);
-
-    if (trace_config.write_into_file()) {
-      int64_t window_size_ns;
-      if (trace_config.has_flush_period_ms() &&
-          trace_config.flush_period_ms() > 0) {
-        // We use 2x the flush period as a margin of error to allow for any
-        // late flush responses to still be sorted correctly.
-        window_size_ns = static_cast<int64_t>(trace_config.flush_period_ms()) *
-                         2 * 1000 * 1000;
-      } else {
-        constexpr uint64_t kDefaultWindowNs =
-            180 * 1000 * 1000 * 1000ULL;  // 3 minutes.
-        PERFETTO_ELOG(
-            "It is strongly recommended to have flush_period_ms set when "
-            "write_into_file is turned on. You will likely have many dropped "
-            "events because of inability to sort the events correctly.");
-        window_size_ns = static_cast<int64_t>(kDefaultWindowNs);
-      }
-      context_->sorter->SetWindowSizeNs(window_size_ns);
-    }
+  if (decoder.has_trace_config()) {
+    ParseTraceConfig(decoder.trace_config());
   }
 
   // Use parent data and length because we want to parse this again
@@ -258,6 +234,52 @@ util::Status ProtoTraceReader::ParsePacket(TraceBlobView packet) {
   return util::OkStatus();
 }
 
+void ProtoTraceReader::ParseTraceConfig(protozero::ConstBytes blob) {
+  protos::pbzero::TraceConfig::Decoder trace_config(blob);
+  // If we're forcing a full sort, we can keep using the INT_MAX duration set
+  // when we created the sorter.
+  const auto& cfg = context_->config;
+  if (cfg.sorting_mode == SortingMode::kForceFullSort) {
+    return;
+  }
+
+  base::Optional<int64_t> flush_period_window_size_ns;
+  if (trace_config.has_flush_period_ms() &&
+      trace_config.flush_period_ms() > 0) {
+    flush_period_window_size_ns =
+        static_cast<int64_t>(trace_config.flush_period_ms()) * 2 * 1000 * 1000;
+  }
+
+  // If we're trying to force flush period based sorting, use that as the
+  // window size if it exists.
+  if (cfg.sorting_mode == SortingMode::kForceFlushPeriodWindowedSort &&
+      flush_period_window_size_ns.has_value()) {
+    context_->sorter->SetWindowSizeNs(flush_period_window_size_ns.value());
+    return;
+  }
+
+  PERFETTO_DCHECK(cfg.sorting_mode == SortingMode::kDefaultHeureustics);
+
+  // If we're not forcing anything and this is a write_into_file trace, then
+  // use flush_period_ms as an indiciator for how big the sliding window for the
+  // sorter should be.
+  if (trace_config.write_into_file()) {
+    int64_t window_size_ns;
+    if (flush_period_window_size_ns.has_value()) {
+      window_size_ns = flush_period_window_size_ns.value();
+    } else {
+      constexpr uint64_t kDefaultWindowNs =
+          180 * 1000 * 1000 * 1000ULL;  // 3 minutes.
+      PERFETTO_ELOG(
+          "It is strongly recommended to have flush_period_ms set when "
+          "write_into_file is turned on. You will likely have many dropped "
+          "events because of inability to sort the events correctly.");
+      window_size_ns = static_cast<int64_t>(kDefaultWindowNs);
+    }
+    context_->sorter->SetWindowSizeNs(window_size_ns);
+  }
+}
+
 void ProtoTraceReader::HandleIncrementalStateCleared(
     const protos::pbzero::TracePacket::Decoder& packet_decoder) {
   if (PERFETTO_UNLIKELY(!packet_decoder.has_trusted_packet_sequence_id())) {
diff --git a/src/trace_processor/importers/proto/proto_trace_reader.h b/src/trace_processor/importers/proto/proto_trace_reader.h
index 4091f1abe..7252f62de 100644
--- a/src/trace_processor/importers/proto/proto_trace_reader.h
+++ b/src/trace_processor/importers/proto/proto_trace_reader.h
@@ -35,6 +35,7 @@ namespace perfetto {
 namespace protos {
 namespace pbzero {
 class TracePacket_Decoder;
+class TraceConfig_Decoder;
 }  // namespace pbzero
 }  // namespace protos
 
@@ -72,6 +73,8 @@ class ProtoTraceReader : public ChunkedTraceReader {
                                 TraceBlobView trace_packet_defaults);
   void ParseInternedData(const protos::pbzero::TracePacket_Decoder&,
                          TraceBlobView interned_data);
+  void ParseTraceConfig(ConstBytes);
+
   PacketSequenceState* GetIncrementalStateForPacketSequence(
       uint32_t sequence_id) {
     if (!incremental_state)
diff --git a/src/trace_processor/trace_processor_shell.cc b/src/trace_processor/trace_processor_shell.cc
index 144810e29..68131142c 100644
--- a/src/trace_processor/trace_processor_shell.cc
+++ b/src/trace_processor/trace_processor_shell.cc
@@ -1048,7 +1048,9 @@ util::Status TraceProcessorMain(int argc, char** argv) {
   CommandLineOptions options = ParseCommandLineOptions(argc, argv);
 
   Config config;
-  config.force_full_sort = options.force_full_sort;
+  config.sorting_mode = options.force_full_sort
+                            ? SortingMode::kForceFullSort
+                            : SortingMode::kDefaultHeureustics;
 
   std::unique_ptr<TraceProcessor> tp = TraceProcessor::CreateInstance(config);
   g_tp = tp.get();
diff --git a/src/trace_processor/trace_processor_storage_impl.cc b/src/trace_processor/trace_processor_storage_impl.cc
index ff9cee6e9..d367bab51 100644
--- a/src/trace_processor/trace_processor_storage_impl.cc
+++ b/src/trace_processor/trace_processor_storage_impl.cc
@@ -41,6 +41,12 @@ namespace trace_processor {
 
 TraceProcessorStorageImpl::TraceProcessorStorageImpl(const Config& cfg) {
   context_.config = cfg;
+
+  // TODO(lalitm): remove this code when Config::force_full_sort is removed.
+  if (cfg.force_full_sort) {
+    context_.config.sorting_mode = SortingMode::kForceFullSort;
+  }
+
   context_.storage.reset(new TraceStorage(context_.config));
   context_.track_tracker.reset(new TrackTracker(&context_));
   context_.async_track_set_tracker.reset(new AsyncTrackSetTracker(&context_));