Merge "Re-land "Stack profiles: Reuse existing frame/module rows rather than adding one per sequence""

4 files changed, 71 insertions, 18 deletions

diff --git a/src/trace_processor/importers/proto/proto_trace_parser.cc b/src/trace_processor/importers/proto/proto_trace_parser.cc
index 0811da446..bd19c0e8f 100644
--- a/src/trace_processor/importers/proto/proto_trace_parser.cc
+++ b/src/trace_processor/importers/proto/proto_trace_parser.cc
@@ -630,20 +630,26 @@ void ProtoTraceParser::ParseModuleSymbols(ConstBytes blob) {
   for (auto addr_it = module_symbols.address_symbols(); addr_it; ++addr_it) {
     protos::pbzero::AddressSymbols::Decoder address_symbols(*addr_it);
 
-    ssize_t frame_row = -1;
+    uint32_t symbol_set_id = context_->storage->symbol_table().size();
+    bool frame_found = false;
     for (int64_t mapping_row : mapping_rows) {
-      frame_row = context_->storage->stack_profile_frames().FindFrameRow(
-          static_cast<size_t>(mapping_row), address_symbols.address());
-      if (frame_row != -1)
-        break;
+      std::vector<int64_t> frame_rows =
+          context_->storage->stack_profile_frames().FindFrameRow(
+              static_cast<size_t>(mapping_row), address_symbols.address());
+
+      for (const int64_t frame_row : frame_rows) {
+        PERFETTO_DCHECK(frame_row >= 0);
+        context_->storage->mutable_stack_profile_frames()->SetSymbolSetId(
+            static_cast<size_t>(frame_row), symbol_set_id);
+        frame_found = true;
+      }
     }
-    if (frame_row == -1) {
+
+    if (!frame_found) {
       context_->storage->IncrementStats(stats::stackprofile_invalid_frame_id);
       continue;
     }
-    uint32_t symbol_set_id = context_->storage->symbol_table().size();
-    context_->storage->mutable_stack_profile_frames()->SetSymbolSetId(
-        static_cast<size_t>(frame_row), symbol_set_id);
+
     for (auto line_it = address_symbols.lines(); line_it; ++line_it) {
       protos::pbzero::Line::Decoder line(*line_it);
       context_->storage->mutable_symbol_table()->Insert(
diff --git a/src/trace_processor/stack_profile_tracker.cc b/src/trace_processor/stack_profile_tracker.cc
index aa6b76470..25815ea10 100644
--- a/src/trace_processor/stack_profile_tracker.cc
+++ b/src/trace_processor/stack_profile_tracker.cc
@@ -82,12 +82,35 @@ base::Optional<int64_t> StackProfileTracker::AddMapping(
       static_cast<int64_t>(mapping.load_bias),
       context_->storage->InternString(base::StringView(path))};
 
-  int64_t cur_row;
+  TraceStorage::StackProfileMappings* mappings =
+      context_->storage->mutable_stack_profile_mappings();
+  int64_t cur_row = -1;
   auto it = mapping_idx_.find(row);
   if (it != mapping_idx_.end()) {
     cur_row = it->second;
   } else {
-    cur_row = context_->storage->mutable_stack_profile_mappings()->Insert(row);
+    std::vector<int64_t> db_mappings =
+        mappings->FindMappingRow(row.name_id, row.build_id);
+    for (const int64_t preexisting_mapping : db_mappings) {
+      PERFETTO_DCHECK(preexisting_mapping >= 0);
+      size_t preexisting_row_id = static_cast<size_t>(preexisting_mapping);
+      TraceStorage::StackProfileMappings::Row preexisting_row{
+          mappings->build_ids()[preexisting_row_id],
+          mappings->exact_offsets()[preexisting_row_id],
+          mappings->start_offsets()[preexisting_row_id],
+          mappings->starts()[preexisting_row_id],
+          mappings->ends()[preexisting_row_id],
+          mappings->load_biases()[preexisting_row_id],
+          mappings->names()[preexisting_row_id]};
+
+      if (row == preexisting_row) {
+        cur_row = preexisting_mapping;
+      }
+    }
+    if (cur_row == -1) {
+      cur_row =
+          context_->storage->mutable_stack_profile_mappings()->Insert(row);
+    }
     mapping_idx_.emplace(row, cur_row);
   }
   mappings_.emplace(id, cur_row);
@@ -118,12 +141,31 @@ base::Optional<int64_t> StackProfileTracker::AddFrame(
   TraceStorage::StackProfileFrames::Row row{str_id, mapping_row,
                                             static_cast<int64_t>(frame.rel_pc)};
 
-  int64_t cur_row;
+  TraceStorage::StackProfileFrames* frames =
+      context_->storage->mutable_stack_profile_frames();
+
+  int64_t cur_row = -1;
   auto it = frame_idx_.find(row);
   if (it != frame_idx_.end()) {
     cur_row = it->second;
   } else {
-    cur_row = context_->storage->mutable_stack_profile_frames()->Insert(row);
+    std::vector<int64_t> db_frames =
+        frames->FindFrameRow(static_cast<size_t>(mapping_row), frame.rel_pc);
+    for (const int64_t preexisting_frame : db_frames) {
+      PERFETTO_DCHECK(preexisting_frame >= 0);
+      size_t preexisting_row_id = static_cast<size_t>(preexisting_frame);
+      TraceStorage::StackProfileFrames::Row preexisting_row{
+          frames->names()[preexisting_row_id],
+          frames->mappings()[preexisting_row_id],
+          frames->rel_pcs()[preexisting_row_id]};
+
+      if (row == preexisting_row) {
+        cur_row = preexisting_frame;
+      }
+    }
+    if (cur_row == -1) {
+      cur_row = context_->storage->mutable_stack_profile_frames()->Insert(row);
+    }
     frame_idx_.emplace(row, cur_row);
   }
   frames_.emplace(id, cur_row);
diff --git a/src/trace_processor/stack_profile_tracker.h b/src/trace_processor/stack_profile_tracker.h
index 309c3767f..9496929cc 100644
--- a/src/trace_processor/stack_profile_tracker.h
+++ b/src/trace_processor/stack_profile_tracker.h
@@ -166,6 +166,8 @@ class StackProfileTracker {
   std::unordered_map<SourceCallstack, int64_t> callstacks_from_frames_;
   std::unordered_map<SourceCallstackId, int64_t> callstacks_;
 
+  // TODO(oysteine): Share these indices between the StackProfileTrackers,
+  // since they're not sequence-specific.
   std::unordered_map<TraceStorage::StackProfileMappings::Row, int64_t>
       mapping_idx_;
   std::unordered_map<TraceStorage::StackProfileFrames::Row, int64_t> frame_idx_;
diff --git a/src/trace_processor/trace_storage.h b/src/trace_processor/trace_storage.h
index a832e9f9c..7243b428b 100644
--- a/src/trace_processor/trace_storage.h
+++ b/src/trace_processor/trace_storage.h
@@ -787,15 +787,17 @@ class TraceStorage {
       rel_pcs_.emplace_back(row.rel_pc);
       symbol_set_ids_.emplace_back(0);
       size_t row_number = names_.size() - 1;
-      index_.emplace(std::make_pair(row.mapping_row, row.rel_pc), row_number);
+      index_[std::make_pair(row.mapping_row, row.rel_pc)].emplace_back(
+          row_number);
       return static_cast<uint32_t>(row_number);
     }
 
-    ssize_t FindFrameRow(size_t mapping_row, uint64_t rel_pc) const {
+    std::vector<int64_t> FindFrameRow(size_t mapping_row,
+                                      uint64_t rel_pc) const {
       auto it = index_.find(std::make_pair(mapping_row, rel_pc));
       if (it == index_.end())
-        return -1;
-      return static_cast<ssize_t>(it->second);
+        return {};
+      return it->second;
     }
 
     void SetSymbolSetId(size_t row_idx, uint32_t symbol_set_id) {
@@ -816,7 +818,8 @@ class TraceStorage {
     std::deque<int64_t> rel_pcs_;
     std::deque<uint32_t> symbol_set_ids_;
 
-    std::map<std::pair<size_t /* mapping row */, uint64_t /* rel_pc */>, size_t>
+    std::map<std::pair<size_t /* mapping row */, uint64_t /* rel_pc */>,
+             std::vector<int64_t>>
         index_;
   };