Simpleperf: improve records parsing.

Sort records by time before handling.
Separate kernel space and user space sample records handling.

Bug: 19483574

Change-Id: I7722bb15678af18cbe883a2cf3fdac948bdd0c9f

diff --git a/simpleperf/cmd_report.cpp b/simpleperf/cmd_report.cpp
index d6aca57..2176220 100644 (file)
--- a/simpleperf/cmd_report.cpp
+++ b/simpleperf/cmd_report.cpp
@@ -248,8 +248,9 @@ void ReportCommand::ReadSampleTreeFromRecordFile() {
       }
     } else if (record->header.type == PERF_RECORD_SAMPLE) {
       const SampleRecord& r = *static_cast<const SampleRecord*>(record.get());
+      bool in_kernel = (r.header.misc & PERF_RECORD_MISC_CPUMODE_MASK) == PERF_RECORD_MISC_KERNEL;
       sample_tree_->AddSample(r.tid_data.pid, r.tid_data.tid, r.ip_data.ip, r.time_data.time,
-                              r.period_data.period);
+                              r.period_data.period, in_kernel);
     } else if (record->header.type == PERF_RECORD_COMM) {
       const CommRecord& r = *static_cast<const CommRecord*>(record.get());
       sample_tree_->AddProcess(r.data.pid, r.comm);

diff --git a/simpleperf/read_elf.cpp b/simpleperf/read_elf.cpp
index 6585252..0cd0b40 100644 (file)
--- a/simpleperf/read_elf.cpp
+++ b/simpleperf/read_elf.cpp
@@ -165,7 +165,7 @@ bool ParseSymbolsFromElfFile(const std::string& filename,
                              std::function<void(const ElfFileSymbol&)> callback) {
   auto owning_binary = llvm::object::createBinary(llvm::StringRef(filename));
   if (owning_binary.getError()) {
-    PLOG(DEBUG) << "can't open file " << filename;
+    PLOG(DEBUG) << "can't open file '" << filename << "'";
     return false;
   }
   bool result = false;

diff --git a/simpleperf/record_file.cpp b/simpleperf/record_file.cpp
index 54a4dda..360d842 100644 (file)
--- a/simpleperf/record_file.cpp
+++ b/simpleperf/record_file.cpp
@@ -397,6 +397,27 @@ std::vector<uint64_t> RecordFileReader::IdsForAttr(const FileAttr* attr) {
   return result;
 }
 
+static bool IsRecordHappensBefore(const std::unique_ptr<const Record>& r1,
+                                  const std::unique_ptr<const Record>& r2) {
+  bool is_r1_sample = (r1->header.type == PERF_RECORD_SAMPLE);
+  bool is_r2_sample = (r2->header.type == PERF_RECORD_SAMPLE);
+  uint64_t time1 = (is_r1_sample ? static_cast<const SampleRecord*>(r1.get())->time_data.time
+                                 : r1->sample_id.time_data.time);
+  uint64_t time2 = (is_r2_sample ? static_cast<const SampleRecord*>(r2.get())->time_data.time
+                                 : r2->sample_id.time_data.time);
+  // The record with smaller time happens first.
+  if (time1 != time2) {
+    return time1 < time2;
+  }
+  // If happening at the same time, make non-sample records before sample records,
+  // because non-sample records may contain useful information to parse sample records.
+  if (is_r1_sample != is_r2_sample) {
+    return is_r1_sample ? false : true;
+  }
+  // Otherwise, don't care of the order.
+  return false;
+}
+
 std::vector<std::unique_ptr<const Record>> RecordFileReader::DataSection() {
   std::vector<std::unique_ptr<const Record>> result;
   const struct FileHeader* header = FileHeader();
@@ -413,6 +434,9 @@ std::vector<std::unique_ptr<const Record>> RecordFileReader::DataSection() {
     }
     p += header->size;
   }
+  if ((attr.sample_type & PERF_SAMPLE_TIME) && attr.sample_id_all) {
+    std::sort(result.begin(), result.end(), IsRecordHappensBefore);
+  }
   return result;
 }
 

diff --git a/simpleperf/record_file_test.cpp b/simpleperf/record_file_test.cpp
index fffaa2a..8d14133 100644 (file)
--- a/simpleperf/record_file_test.cpp
+++ b/simpleperf/record_file_test.cpp
@@ -35,6 +35,8 @@ class RecordFileTest : public ::testing::Test {
     const EventType* event_type = EventTypeFactory::FindEventTypeByName("cpu-cycles");
     ASSERT_TRUE(event_type != nullptr);
     event_attr = CreateDefaultPerfEventAttr(*event_type);
+    event_attr.sample_id_all = 1;
+    event_attr.sample_type |= PERF_SAMPLE_TIME;
     std::unique_ptr<EventFd> event_fd = EventFd::OpenEventFileForProcess(event_attr, getpid());
     ASSERT_TRUE(event_fd != nullptr);
     event_fds.push_back(std::move(event_fd));
@@ -80,7 +82,6 @@ TEST_F(RecordFileTest, smoke) {
   // Read and check data section.
   std::vector<std::unique_ptr<const Record>> records = reader->DataSection();
   ASSERT_EQ(1u, records.size());
-  ASSERT_EQ(mmap_record.header.type, records[0]->header.type);
   CheckRecordEqual(mmap_record, *records[0]);
 
   // Read and check feature section.
@@ -95,3 +96,33 @@ TEST_F(RecordFileTest, smoke) {
 
   ASSERT_TRUE(reader->Close());
 }
+
+TEST_F(RecordFileTest, records_sorted_by_time) {
+  // Write to a record file;
+  std::unique_ptr<RecordFileWriter> writer =
+      RecordFileWriter::CreateInstance(filename, event_attr, event_fds);
+  ASSERT_TRUE(writer != nullptr);
+
+  // Write data section.
+  MmapRecord r1 = CreateMmapRecord(event_attr, true, 1, 1, 0x100, 0x2000, 0x3000, "mmap_record1");
+  MmapRecord r2 = r1;
+  MmapRecord r3 = r1;
+  r1.sample_id.time_data.time = 2;
+  r2.sample_id.time_data.time = 1;
+  r3.sample_id.time_data.time = 3;
+  ASSERT_TRUE(writer->WriteData(r1.BinaryFormat()));
+  ASSERT_TRUE(writer->WriteData(r2.BinaryFormat()));
+  ASSERT_TRUE(writer->WriteData(r3.BinaryFormat()));
+  ASSERT_TRUE(writer->Close());
+
+  // Read from a record file.
+  std::unique_ptr<RecordFileReader> reader = RecordFileReader::CreateInstance(filename);
+  ASSERT_TRUE(reader != nullptr);
+  std::vector<std::unique_ptr<const Record>> records = reader->DataSection();
+  ASSERT_EQ(3u, records.size());
+  CheckRecordEqual(r2, *records[0]);
+  CheckRecordEqual(r1, *records[1]);
+  CheckRecordEqual(r3, *records[2]);
+
+  ASSERT_TRUE(reader->Close());
+}

diff --git a/simpleperf/sample_tree.cpp b/simpleperf/sample_tree.cpp
index 5c359e8..209d786 100644 (file)
--- a/simpleperf/sample_tree.cpp
+++ b/simpleperf/sample_tree.cpp
@@ -125,22 +125,24 @@ static bool IsIpInMap(int pid, uint64_t ip, const MapEntry* map) {
   return (pid == map->pid && map->start_addr <= ip && map->start_addr + map->len > ip);
 }
 
-const MapEntry* SampleTree::FindMapEntryOrNew(int pid, uint64_t ip) {
+const MapEntry* SampleTree::FindMapEntryOrNew(int pid, uint64_t ip, bool in_kernel) {
   // Construct a map_entry which is strictly after the searched map_entry, based on MapComparator.
   MapEntry find_map = {
-      .pid = pid,
+      .pid = (in_kernel) ? -1 : pid,
       .start_addr = ip,
       .len = static_cast<uint64_t>(-1),
       .time = static_cast<uint64_t>(-1),
   };
-  auto it = user_map_tree_.upper_bound(&find_map);
-  if (it != user_map_tree_.begin() && IsIpInMap(pid, ip, *--it)) {
-    return *it;
-  }
-  find_map.pid = -1;
-  it = kernel_map_tree_.upper_bound(&find_map);
-  if (it != kernel_map_tree_.begin() && IsIpInMap(-1, ip, *--it)) {
-    return *it;
+  if (!in_kernel) {
+    auto it = user_map_tree_.upper_bound(&find_map);
+    if (it != user_map_tree_.begin() && IsIpInMap(pid, ip, *--it)) {
+      return *it;
+    }
+  } else {
+    auto it = kernel_map_tree_.upper_bound(&find_map);
+    if (it != kernel_map_tree_.begin() && IsIpInMap(-1, ip, *--it)) {
+      return *it;
+    }
   }
   return FindUnknownMapEntryOrNew(pid);
 }
@@ -164,9 +166,10 @@ const MapEntry* SampleTree::FindUnknownMapEntryOrNew(int pid) {
   return it->second;
 }
 
-void SampleTree::AddSample(int pid, int tid, uint64_t ip, uint64_t time, uint64_t period) {
+void SampleTree::AddSample(int pid, int tid, uint64_t ip, uint64_t time, uint64_t period,
+                           bool in_kernel) {
   const ProcessEntry* process_entry = FindProcessEntryOrNew(pid);
-  const MapEntry* map_entry = FindMapEntryOrNew(pid, ip);
+  const MapEntry* map_entry = FindMapEntryOrNew(pid, ip, in_kernel);
   const SymbolEntry* symbol_entry = FindSymbolEntry(ip, map_entry);
 
   SampleEntry find_sample = {

diff --git a/simpleperf/sample_tree.h b/simpleperf/sample_tree.h
index 6d18393..f97d0d3 100644 (file)
--- a/simpleperf/sample_tree.h
+++ b/simpleperf/sample_tree.h
@@ -73,7 +73,7 @@ class SampleTree {
                     const std::string& filename);
   void AddUserMap(int pid, uint64_t start_addr, uint64_t len, uint64_t pgoff, uint64_t time,
                   const std::string& filename);
-  void AddSample(int pid, int tid, uint64_t ip, uint64_t time, uint64_t period);
+  void AddSample(int pid, int tid, uint64_t ip, uint64_t time, uint64_t period, bool in_kernel);
   void VisitAllSamples(std::function<void(const SampleEntry&)> callback);
 
   uint64_t TotalSamples() const {
@@ -87,7 +87,7 @@ class SampleTree {
  private:
   void RemoveOverlappedUserMap(const MapEntry* map);
   const ProcessEntry* FindProcessEntryOrNew(int pid);
-  const MapEntry* FindMapEntryOrNew(int pid, uint64_t ip);
+  const MapEntry* FindMapEntryOrNew(int pid, uint64_t ip, bool in_kernel);
   const MapEntry* FindUnknownMapEntryOrNew(int pid);
   DsoEntry* FindKernelDsoOrNew(const std::string& filename);
   DsoEntry* FindUserDsoOrNew(const std::string& filename);

diff --git a/simpleperf/sample_tree_test.cpp b/simpleperf/sample_tree_test.cpp
index 166297d..9a2ca0b 100644 (file)
--- a/simpleperf/sample_tree_test.cpp
+++ b/simpleperf/sample_tree_test.cpp
@@ -90,9 +90,9 @@ class SampleTreeTest : public testing::Test {
 };
 
 TEST_F(SampleTreeTest, ip_in_map) {
-  sample_tree->AddSample(1, 1, 1, 0, 0);
-  sample_tree->AddSample(1, 1, 5, 0, 0);
-  sample_tree->AddSample(1, 1, 10, 0, 0);
+  sample_tree->AddSample(1, 1, 1, 0, 0, false);
+  sample_tree->AddSample(1, 1, 5, 0, 0, false);
+  sample_tree->AddSample(1, 1, 10, 0, 0, false);
   std::vector<ExpectedSampleInMap> expected_samples = {
       {1, 1, 1, 1, 3},
   };
@@ -100,8 +100,8 @@ TEST_F(SampleTreeTest, ip_in_map) {
 }
 
 TEST_F(SampleTreeTest, different_pid) {
-  sample_tree->AddSample(1, 1, 1, 0, 0);
-  sample_tree->AddSample(2, 2, 1, 0, 0);
+  sample_tree->AddSample(1, 1, 1, 0, 0, false);
+  sample_tree->AddSample(2, 2, 1, 0, 0, false);
   std::vector<ExpectedSampleInMap> expected_samples = {
       {1, 1, 1, 1, 1}, {2, 2, 2, 1, 1},
   };
@@ -109,8 +109,8 @@ TEST_F(SampleTreeTest, different_pid) {
 }
 
 TEST_F(SampleTreeTest, different_tid) {
-  sample_tree->AddSample(1, 1, 1, 0, 0);
-  sample_tree->AddSample(1, 11, 1, 0, 0);
+  sample_tree->AddSample(1, 1, 1, 0, 0, false);
+  sample_tree->AddSample(1, 11, 1, 0, 0, false);
   std::vector<ExpectedSampleInMap> expected_samples = {
       {1, 1, 1, 1, 1}, {1, 11, 1, 1, 1},
   };
@@ -118,8 +118,8 @@ TEST_F(SampleTreeTest, different_tid) {
 }
 
 TEST_F(SampleTreeTest, different_map) {
-  sample_tree->AddSample(1, 1, 1, 0, 0);
-  sample_tree->AddSample(1, 1, 11, 0, 0);
+  sample_tree->AddSample(1, 1, 1, 0, 0, false);
+  sample_tree->AddSample(1, 1, 11, 0, 0, false);
   std::vector<ExpectedSampleInMap> expected_samples = {
       {1, 1, 1, 1, 1}, {1, 1, 1, 11, 1},
   };
@@ -127,9 +127,9 @@ TEST_F(SampleTreeTest, different_map) {
 }
 
 TEST_F(SampleTreeTest, unmapped_sample) {
-  sample_tree->AddSample(1, 1, 0, 0, 0);
-  sample_tree->AddSample(1, 1, 31, 0, 0);
-  sample_tree->AddSample(1, 1, 70, 0, 0);
+  sample_tree->AddSample(1, 1, 0, 0, 0, false);
+  sample_tree->AddSample(1, 1, 31, 0, 0, false);
+  sample_tree->AddSample(1, 1, 70, 0, 0, false);
   // Match the unknown map.
   std::vector<ExpectedSampleInMap> expected_samples = {
       {1, 1, 1, 0, 3},
@@ -138,8 +138,8 @@ TEST_F(SampleTreeTest, unmapped_sample) {
 }
 
 TEST_F(SampleTreeTest, map_kernel) {
-  sample_tree->AddSample(1, 1, 11, 0, 0);
-  sample_tree->AddSample(1, 1, 21, 0, 0);
+  sample_tree->AddSample(1, 1, 11, 0, 0, true);
+  sample_tree->AddSample(1, 1, 11, 0, 0, false);
   std::vector<ExpectedSampleInMap> expected_samples = {
       {1, 1, -1, 11, 1}, {1, 1, 1, 11, 1},
   };
@@ -148,14 +148,14 @@ TEST_F(SampleTreeTest, map_kernel) {
 
 TEST(sample_tree, overlapped_map) {
   auto sample_tree = std::unique_ptr<SampleTree>(new SampleTree(CompareSampleFunction));
-  sample_tree->AddUserMap(1, 1, 10, 0, 0, "");  // Add map 1.
-  sample_tree->AddSample(1, 1, 5, 0, 0);        // Hit map 1.
-  sample_tree->AddUserMap(1, 5, 20, 0, 0, "");  // Add map 2.
-  sample_tree->AddSample(1, 1, 6, 0, 0);        // Hit map 2.
-  sample_tree->AddSample(1, 1, 4, 0, 0);        // Hit unknown map.
-  sample_tree->AddUserMap(1, 2, 7, 0, 0, "");   // Add map 3.
-  sample_tree->AddSample(1, 1, 7, 0, 0);        // Hit map 3.
-  sample_tree->AddSample(1, 1, 10, 0, 0);       // Hit unknown map.
+  sample_tree->AddUserMap(1, 1, 10, 0, 0, "");    // Add map 1.
+  sample_tree->AddSample(1, 1, 5, 0, 0, false);   // Hit map 1.
+  sample_tree->AddUserMap(1, 5, 20, 0, 0, "");    // Add map 2.
+  sample_tree->AddSample(1, 1, 6, 0, 0, false);   // Hit map 2.
+  sample_tree->AddSample(1, 1, 4, 0, 0, false);   // Hit unknown map.
+  sample_tree->AddUserMap(1, 2, 7, 0, 0, "");     // Add map 3.
+  sample_tree->AddSample(1, 1, 7, 0, 0, false);   // Hit map 3.
+  sample_tree->AddSample(1, 1, 10, 0, 0, false);  // Hit unknown map.
 
   std::vector<ExpectedSampleInMap> expected_samples = {
       {1, 1, 1, 0, 2}, {1, 1, 1, 1, 1}, {1, 1, 1, 2, 1}, {1, 1, 1, 5, 1},