[Sampling profiler] Encapsulate metadata processing in its own class

Moves the metadata processing for upload into a separate class as it's
somewhat complex but self-contained and has a narrow interface. This also
allows the behavior to be tested more directly, simplifying the tests.

Bug: 976864
Change-Id: Icd608d1665337fa7e8e5168d1e87e529acbd84cb
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1897359
Commit-Queue: Mike Wittman <wittman@chromium.org>
Reviewed-by: Charlie Andrews <charliea@chromium.org>
Cr-Commit-Position: refs/heads/master@{#712789}

components/metrics/BUILD.gn

@@ -322,10 +322,12 @@ source_set("call_stack_profile_params") {
 source_set("child_call_stack_profile_builder") {
   public = [
     "call_stack_profile_builder.h",
+    "call_stack_profile_metadata.h",
     "child_call_stack_profile_collector.h",
   ]
   sources = [
     "call_stack_profile_builder.cc",
+    "call_stack_profile_metadata.cc",
     "child_call_stack_profile_collector.cc",
   ]
   public_deps = [
@@ -406,6 +408,7 @@ source_set("unit_tests") {
   sources = [
     "android_metrics_provider_unittest.cc",
     "call_stack_profile_builder_unittest.cc",
+    "call_stack_profile_metadata_unittest.cc",
     "call_stack_profile_metrics_provider_unittest.cc",
     "child_call_stack_profile_collector_unittest.cc",
     "cloned_install_detector_unittest.cc",

components/metrics/call_stack_profile_builder.cc

@@ -83,7 +83,7 @@ void CallStackProfileBuilder::RecordMetadata(
     }
   }
 
-  metadata_item_count_ = metadata_provider->GetItems(&metadata_items_);
+  metadata_.RecordMetadata(metadata_provider);
 }
 
 void CallStackProfileBuilder::OnSampleCompleted(
@@ -148,8 +148,8 @@ void CallStackProfileBuilder::OnSampleCompleted(std::vector<base::Frame> frames,
   if (is_continued_work_)
     stack_sample_proto->set_continued_work(is_continued_work_);
 
-  *stack_sample_proto->mutable_metadata() =
-      CreateSampleMetadata(call_stack_profile->mutable_metadata_name_hash());
+  *stack_sample_proto->mutable_metadata() = metadata_.CreateSampleMetadata(
+      call_stack_profile->mutable_metadata_name_hash());
 }
 
 void CallStackProfileBuilder::OnProfileCompleted(
@@ -223,126 +223,4 @@ bool CallStackProfileBuilder::StackComparer::operator()(
       });
 }
 
-bool CallStackProfileBuilder::MetadataKeyCompare::operator()(
-    const MetadataKey& a,
-    const MetadataKey& b) const {
-  return std::tie(a.name_hash, a.key) < std::tie(b.name_hash, b.key);
-}
-
-CallStackProfileBuilder::MetadataKey::MetadataKey(uint64_t name_hash,
-                                                  base::Optional<int64_t> key)
-    : name_hash(name_hash), key(key) {}
-
-CallStackProfileBuilder::MetadataKey::MetadataKey(const MetadataKey& other) =
-    default;
-CallStackProfileBuilder::MetadataKey& CallStackProfileBuilder::MetadataKey::
-operator=(const MetadataKey& other) = default;
-
-CallStackProfileBuilder::MetadataMap CallStackProfileBuilder::CreateMetadataMap(
-    base::ProfileBuilder::MetadataItemArray items,
-    size_t item_count) {
-  MetadataMap item_map;
-  for (size_t i = 0; i < item_count; ++i)
-    item_map[MetadataKey{items[i].name_hash, items[i].key}] = items[i].value;
-  return item_map;
-}
-
-CallStackProfileBuilder::MetadataMap
-CallStackProfileBuilder::GetNewOrModifiedMetadataItems(
-    const MetadataMap& current_items,
-    const MetadataMap& previous_items) {
-  MetadataMap new_or_modified_items;
-  // Find the new or modified items by subtracting any previous items that are
-  // exactly the same as the current items (i.e. equal in key *and* value).
-  auto key_and_value_comparator = [](const std::pair<MetadataKey, int64_t>& a,
-                                     const std::pair<MetadataKey, int64_t>& b) {
-    return std::tie(a.first.name_hash, a.first.key, a.second) <
-           std::tie(b.first.name_hash, b.first.key, b.second);
-  };
-  std::set_difference(
-      current_items.begin(), current_items.end(), previous_items.begin(),
-      previous_items.end(),
-      std::inserter(new_or_modified_items, new_or_modified_items.begin()),
-      key_and_value_comparator);
-  return new_or_modified_items;
-}
-
-CallStackProfileBuilder::MetadataMap
-CallStackProfileBuilder::GetDeletedMetadataItems(
-    const MetadataMap& current_items,
-    const MetadataMap& previous_items) {
-  MetadataMap deleted_items;
-  // Find the deleted metadata items by subtracting the current items from the
-  // previous items, comparing items solely map key (as opposed to map key and
-  // value as in GetNewOrModifiedMetadataItems()). Comparing by key is necessary
-  // to distinguish modified items from deleted items: subtraction of modified
-  // items, which have the same key but different values, should produce the
-  // empty set. Deleted items have a key only in |previous_items| so should be
-  // retained in the result.
-  auto key_comparator = [](const std::pair<MetadataKey, int64_t>& lhs,
-                           const std::pair<MetadataKey, int64_t>& rhs) {
-    return MetadataKeyCompare()(lhs.first, rhs.first);
-  };
-  std::set_difference(previous_items.begin(), previous_items.end(),
-                      current_items.begin(), current_items.end(),
-                      std::inserter(deleted_items, deleted_items.begin()),
-                      key_comparator);
-  return deleted_items;
-}
-
-google::protobuf::RepeatedPtrField<CallStackProfile::MetadataItem>
-CallStackProfileBuilder::CreateSampleMetadata(
-    google::protobuf::RepeatedField<uint64_t>* metadata_name_hashes) {
-  DCHECK_EQ(metadata_hashes_cache_.size(),
-            static_cast<size_t>(metadata_name_hashes->size()));
-
-  google::protobuf::RepeatedPtrField<CallStackProfile::MetadataItem>
-      metadata_items;
-  MetadataMap current_items =
-      CreateMetadataMap(metadata_items_, metadata_item_count_);
-
-  for (auto item :
-       GetNewOrModifiedMetadataItems(current_items, previous_items_)) {
-    size_t name_hash_index =
-        MaybeAppendNameHash(item.first.name_hash, metadata_name_hashes);
-
-    CallStackProfile::MetadataItem* profile_item = metadata_items.Add();
-    profile_item->set_name_hash_index(name_hash_index);
-    if (item.first.key.has_value())
-      profile_item->set_key(*item.first.key);
-    profile_item->set_value(item.second);
-  }
-
-  for (auto item : GetDeletedMetadataItems(current_items, previous_items_)) {
-    size_t name_hash_index =
-        MaybeAppendNameHash(item.first.name_hash, metadata_name_hashes);
-
-    CallStackProfile::MetadataItem* profile_item = metadata_items.Add();
-    profile_item->set_name_hash_index(name_hash_index);
-    if (item.first.key.has_value())
-      profile_item->set_key(*item.first.key);
-    // Leave the value empty to indicate that the item was deleted.
-  }
-
-  previous_items_ = std::move(current_items);
-  metadata_item_count_ = 0;
-
-  return metadata_items;
-}
-
-size_t CallStackProfileBuilder::MaybeAppendNameHash(
-    uint64_t name_hash,
-    google::protobuf::RepeatedField<uint64_t>* metadata_name_hashes) {
-  std::unordered_map<uint64_t, int>::iterator it;
-  bool inserted;
-  int next_item_index = metadata_name_hashes->size();
-
-  std::tie(it, inserted) =
-      metadata_hashes_cache_.emplace(name_hash, next_item_index);
-  if (inserted)
-    metadata_name_hashes->Add(name_hash);
-
-  return it->second;
-}
-
 }  // namespace metrics

components/metrics/call_stack_profile_builder.h

@@ -18,6 +18,7 @@
 #include "base/profiler/profile_builder.h"
 #include "base/sampling_heap_profiler/module_cache.h"
 #include "base/time/time.h"
+#include "components/metrics/call_stack_profile_metadata.h"
 #include "components/metrics/call_stack_profile_params.h"
 #include "components/metrics/child_call_stack_profile_collector.h"
 #include "mojo/public/cpp/bindings/pending_remote.h"
@@ -100,52 +101,6 @@ class CallStackProfileBuilder : public base::ProfileBuilder {
                     const CallStackProfile::Stack* stack2) const;
   };
 
-  // Comparison function for the metadata map.
-  struct MetadataKey;
-  struct MetadataKeyCompare {
-    bool operator()(const MetadataKey& a, const MetadataKey& b) const;
-  };
-
-  // Definitions for a map-based representation of sample metadata.
-  struct MetadataKey {
-    MetadataKey(uint64_t name_hash, base::Optional<int64_t> key);
-
-    MetadataKey(const MetadataKey& other);
-    MetadataKey& operator=(const MetadataKey& other);
-
-    // The name_hash and optional user-specified key uniquely identifies a
-    // metadata value. See base::MetadataRecorder for details.
-    uint64_t name_hash;
-    base::Optional<int64_t> key;
-  };
-  using MetadataMap = std::map<MetadataKey, int64_t, MetadataKeyCompare>;
-
-  // Creates the metdata map from the array of items.
-  MetadataMap CreateMetadataMap(base::ProfileBuilder::MetadataItemArray items,
-                                size_t item_count);
-
-  // Returns all metadata items with new values in the current sample.
-  MetadataMap GetNewOrModifiedMetadataItems(const MetadataMap& current_items,
-                                            const MetadataMap& previous_items);
-
-  // Returns all metadata items deleted since the previous sample.
-  MetadataMap GetDeletedMetadataItems(const MetadataMap& current_items,
-                                      const MetadataMap& previous_items);
-
-  // Creates MetadataItems for the currently active metadata, adding new name
-  // hashes to |metadata_name_hashes| if necessary. The same
-  // |metadata_name_hashes| must be passed to each invocation, and must not be
-  // modified outside this function.
-  google::protobuf::RepeatedPtrField<CallStackProfile::MetadataItem>
-  CreateSampleMetadata(
-      google::protobuf::RepeatedField<uint64_t>* metadata_name_hashes);
-
-  // Appends the |name_hash| to |name_hashes| if it's not already
-  // present. Returns its index in |name_hashes|.
-  size_t MaybeAppendNameHash(
-      uint64_t name_hash,
-      google::protobuf::RepeatedField<uint64_t>* metadata_name_hashes);
-
   // The module cache to use for the duration the sampling associated with this
   // ProfileBuilder.
   base::ModuleCache module_cache_;
@@ -173,15 +128,8 @@ class CallStackProfileBuilder : public base::ProfileBuilder {
   // The start time of a profile collection.
   const base::TimeTicks profile_start_time_;
 
-  // The data fetched from the MetadataRecorder for the next sample.
-  base::ProfileBuilder::MetadataItemArray metadata_items_;
-  size_t metadata_item_count_ = 0;
-
-  // The data fetched from the MetadataRecorder for the previous sample.
-  MetadataMap previous_items_;
-
-  // Maps metadata hash to index in |metadata_name_hash| array.
-  std::unordered_map<uint64_t, int> metadata_hashes_cache_;
+  // Maintains the current metadata to apply to samples.
+  CallStackProfileMetadata metadata_;
 
   DISALLOW_COPY_AND_ASSIGN(CallStackProfileBuilder);
 };

components/metrics/call_stack_profile_metadata.cc

+// Copyright 2019 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "components/metrics/call_stack_profile_metadata.h"
+
+#include <algorithm>
+#include <iterator>
+#include <tuple>
+
+namespace metrics {
+
+CallStackProfileMetadata::CallStackProfileMetadata() = default;
+
+CallStackProfileMetadata::~CallStackProfileMetadata() = default;
+
+// This function is invoked on the profiler thread while the target thread is
+// suspended so must not take any locks, including indirectly through use of
+// heap allocation, LOG, CHECK, or DCHECK.
+void CallStackProfileMetadata::RecordMetadata(
+    base::ProfileBuilder::MetadataProvider* metadata_provider) {
+  metadata_item_count_ = metadata_provider->GetItems(&metadata_items_);
+}
+
+google::protobuf::RepeatedPtrField<CallStackProfile::MetadataItem>
+CallStackProfileMetadata::CreateSampleMetadata(
+    google::protobuf::RepeatedField<uint64_t>* metadata_name_hashes) {
+  DCHECK_EQ(metadata_hashes_cache_.size(),
+            static_cast<size_t>(metadata_name_hashes->size()));
+
+  google::protobuf::RepeatedPtrField<CallStackProfile::MetadataItem>
+      metadata_items;
+  MetadataMap current_items =
+      CreateMetadataMap(metadata_items_, metadata_item_count_);
+
+  for (auto item :
+       GetNewOrModifiedMetadataItems(current_items, previous_items_)) {
+    size_t name_hash_index =
+        MaybeAppendNameHash(item.first.name_hash, metadata_name_hashes);
+
+    CallStackProfile::MetadataItem* profile_item = metadata_items.Add();
+    profile_item->set_name_hash_index(name_hash_index);
+    if (item.first.key.has_value())
+      profile_item->set_key(*item.first.key);
+    profile_item->set_value(item.second);
+  }
+
+  for (auto item : GetDeletedMetadataItems(current_items, previous_items_)) {
+    size_t name_hash_index =
+        MaybeAppendNameHash(item.first.name_hash, metadata_name_hashes);
+
+    CallStackProfile::MetadataItem* profile_item = metadata_items.Add();
+    profile_item->set_name_hash_index(name_hash_index);
+    if (item.first.key.has_value())
+      profile_item->set_key(*item.first.key);
+    // Leave the value empty to indicate that the item was deleted.
+  }
+
+  previous_items_ = std::move(current_items);
+  metadata_item_count_ = 0;
+
+  return metadata_items;
+}
+
+bool CallStackProfileMetadata::MetadataKeyCompare::operator()(
+    const MetadataKey& a,
+    const MetadataKey& b) const {
+  return std::tie(a.name_hash, a.key) < std::tie(b.name_hash, b.key);
+}
+
+CallStackProfileMetadata::MetadataKey::MetadataKey(uint64_t name_hash,
+                                                   base::Optional<int64_t> key)
+    : name_hash(name_hash), key(key) {}
+
+CallStackProfileMetadata::MetadataKey::MetadataKey(const MetadataKey& other) =
+    default;
+CallStackProfileMetadata::MetadataKey& CallStackProfileMetadata::MetadataKey::
+operator=(const MetadataKey& other) = default;
+
+CallStackProfileMetadata::MetadataMap
+CallStackProfileMetadata::CreateMetadataMap(
+    base::ProfileBuilder::MetadataItemArray items,
+    size_t item_count) {
+  MetadataMap item_map;
+  for (size_t i = 0; i < item_count; ++i)
+    item_map[MetadataKey{items[i].name_hash, items[i].key}] = items[i].value;
+  return item_map;
+}
+
+CallStackProfileMetadata::MetadataMap
+CallStackProfileMetadata::GetNewOrModifiedMetadataItems(
+    const MetadataMap& current_items,
+    const MetadataMap& previous_items) {
+  MetadataMap new_or_modified_items;
+  // Find the new or modified items by subtracting any previous items that are
+  // exactly the same as the current items (i.e. equal in key *and* value).
+  auto key_and_value_comparator = [](const std::pair<MetadataKey, int64_t>& a,
+                                     const std::pair<MetadataKey, int64_t>& b) {
+    return std::tie(a.first.name_hash, a.first.key, a.second) <
+           std::tie(b.first.name_hash, b.first.key, b.second);
+  };
+  std::set_difference(
+      current_items.begin(), current_items.end(), previous_items.begin(),
+      previous_items.end(),
+      std::inserter(new_or_modified_items, new_or_modified_items.begin()),
+      key_and_value_comparator);
+  return new_or_modified_items;
+}
+
+CallStackProfileMetadata::MetadataMap
+CallStackProfileMetadata::GetDeletedMetadataItems(
+    const MetadataMap& current_items,
+    const MetadataMap& previous_items) {
+  MetadataMap deleted_items;
+  // Find the deleted metadata items by subtracting the current items from the
+  // previous items, comparing items solely map key (as opposed to map key and
+  // value as in GetNewOrModifiedMetadataItems()). Comparing by key is necessary
+  // to distinguish modified items from deleted items: subtraction of modified
+  // items, which have the same key but different values, should produce the
+  // empty set. Deleted items have a key only in |previous_items| so should be
+  // retained in the result.
+  auto key_comparator = [](const std::pair<MetadataKey, int64_t>& lhs,
+                           const std::pair<MetadataKey, int64_t>& rhs) {
+    return MetadataKeyCompare()(lhs.first, rhs.first);
+  };
+  std::set_difference(previous_items.begin(), previous_items.end(),
+                      current_items.begin(), current_items.end(),
+                      std::inserter(deleted_items, deleted_items.begin()),
+                      key_comparator);
+  return deleted_items;
+}
+
+size_t CallStackProfileMetadata::MaybeAppendNameHash(
+    uint64_t name_hash,
+    google::protobuf::RepeatedField<uint64_t>* metadata_name_hashes) {
+  std::unordered_map<uint64_t, int>::iterator it;
+  bool inserted;
+  int next_item_index = metadata_name_hashes->size();
+
+  std::tie(it, inserted) =
+      metadata_hashes_cache_.emplace(name_hash, next_item_index);
+  if (inserted)
+    metadata_name_hashes->Add(name_hash);
+
+  return it->second;
+}
+
+}  // namespace metrics

components/metrics/call_stack_profile_metadata.h

+// Copyright 2019 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef COMPONENTS_METRICS_CALL_STACK_PROFILE_METADATA_H_
+#define COMPONENTS_METRICS_CALL_STACK_PROFILE_METADATA_H_
+
+#include <map>
+#include <unordered_map>
+#include <utility>
+
+#include "base/optional.h"
+#include "base/profiler/metadata_recorder.h"
+#include "third_party/metrics_proto/sampled_profile.pb.h"
+
+namespace metrics {
+
+// Helper class for maintaining metadata state across samples and generating
+// metadata proto messages.
+class CallStackProfileMetadata {
+ public:
+  CallStackProfileMetadata();
+  ~CallStackProfileMetadata();
+
+  CallStackProfileMetadata(const CallStackProfileMetadata& other) = delete;
+  CallStackProfileMetadata& operator=(const CallStackProfileMetadata& other) =
+      delete;
+
+  // Records the metadata for the next sample.
+  void RecordMetadata(
+      base::ProfileBuilder::MetadataProvider* metadata_provider);
+
+  // Creates MetadataItems for the currently active metadata, adding new name
+  // hashes to |metadata_name_hashes| if necessary. The same
+  // |metadata_name_hashes| must be passed to each invocation, and must not be
+  // modified outside this function.
+  google::protobuf::RepeatedPtrField<CallStackProfile::MetadataItem>
+  CreateSampleMetadata(
+      google::protobuf::RepeatedField<uint64_t>* metadata_name_hashes);
+
+ private:
+  // Comparison function for the metadata map.
+  struct MetadataKey;
+  struct MetadataKeyCompare {
+    bool operator()(const MetadataKey& a, const MetadataKey& b) const;
+  };
+
+  // Definitions for a map-based representation of sample metadata.
+  struct MetadataKey {
+    MetadataKey(uint64_t name_hash, base::Optional<int64_t> key);
+
+    MetadataKey(const MetadataKey& other);
+    MetadataKey& operator=(const MetadataKey& other);
+
+    // The name_hash and optional user-specified key uniquely identifies a
+    // metadata value. See base::MetadataRecorder for details.
+    uint64_t name_hash;
+    base::Optional<int64_t> key;
+  };
+  using MetadataMap = std::map<MetadataKey, int64_t, MetadataKeyCompare>;
+
+  // Creates the metdata map from the array of items.
+  MetadataMap CreateMetadataMap(base::ProfileBuilder::MetadataItemArray items,
+                                size_t item_count);
+
+  // Returns all metadata items with new values in the current sample.
+  MetadataMap GetNewOrModifiedMetadataItems(const MetadataMap& current_items,
+                                            const MetadataMap& previous_items);
+
+  // Returns all metadata items deleted since the previous sample.
+  MetadataMap GetDeletedMetadataItems(const MetadataMap& current_items,
+                                      const MetadataMap& previous_items);
+
+  // Appends the |name_hash| to |name_hashes| if it's not already
+  // present. Returns its index in |name_hashes|.
+  size_t MaybeAppendNameHash(
+      uint64_t name_hash,
+      google::protobuf::RepeatedField<uint64_t>* metadata_name_hashes);
+
+  // The data provided for the next sample.
+  base::ProfileBuilder::MetadataItemArray metadata_items_;
+  size_t metadata_item_count_ = 0;
+
+  // The data provided for the previous sample.
+  MetadataMap previous_items_;
+
+  // Maps metadata hash to index in |metadata_name_hash| array.
+  std::unordered_map<uint64_t, int> metadata_hashes_cache_;
+};
+
+}  // namespace metrics
+
+#endif  // COMPONENTS_METRICS_CALL_STACK_PROFILE_METADATA_H_