[Sampling profiler] Provide public API for setting metadata on past samples

Provides a global public metadata interface for setting metadata on
already-recorded samples in active profiles.

Routes the metadata setting via global functions in sample_metadata.h to
provide consistency with the existing interface for setting metadata.

Bug: 1034758
Change-Id: Ie7677efb976a8a035e9bbcf689abe98ad64cf225
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2036561
Commit-Queue: Mike Wittman <wittman@chromium.org>
Reviewed-by: Etienne Pierre-Doray <etiennep@chromium.org>
Cr-Commit-Position: refs/heads/master@{#742181}

base/profiler/sample_metadata.cc

@@ -6,6 +6,7 @@
 
 #include "base/metrics/metrics_hashes.h"
 #include "base/no_destructor.h"
+#include "base/profiler/stack_sampling_profiler.h"
 
 namespace base {
 
@@ -41,6 +42,34 @@ void RemoveSampleMetadata(StringPiece name, int64_t key) {
   GetSampleMetadataRecorder()->Remove(HashMetricName(name), key);
 }
 
+// This function is friended by StackSamplingProfiler so must live directly in
+// the base namespace.
+void ApplyMetadataToPastSamplesImpl(TimeTicks period_start,
+                                    TimeTicks period_end,
+                                    int64_t name_hash,
+                                    Optional<int64_t> key,
+                                    int64_t value) {
+  StackSamplingProfiler::ApplyMetadataToPastSamples(period_start, period_end,
+                                                    name_hash, key, value);
+}
+
+void ApplyMetadataToPastSamples(TimeTicks period_start,
+                                TimeTicks period_end,
+                                StringPiece name,
+                                int64_t value) {
+  return ApplyMetadataToPastSamplesImpl(period_start, period_end,
+                                        HashMetricName(name), nullopt, value);
+}
+
+void ApplyMetadataToPastSamples(TimeTicks period_start,
+                                TimeTicks period_end,
+                                StringPiece name,
+                                int64_t key,
+                                int64_t value) {
+  return ApplyMetadataToPastSamplesImpl(period_start, period_end,
+                                        HashMetricName(name), key, value);
+}
+
 MetadataRecorder* GetSampleMetadataRecorder() {
   static NoDestructor<MetadataRecorder> instance;
   return instance.get();

base/profiler/sample_metadata.h

@@ -105,6 +105,25 @@ BASE_EXPORT void RemoveSampleMetadata(StringPiece name);
 // If such an item doesn't exist, this has no effect.
 BASE_EXPORT void RemoveSampleMetadata(StringPiece name, int64_t key);
 
+// Applies the specified metadata to samples already recorded between
+// |period_start| and |period_end| in all thread's active profiles, subject to
+// the condition that the profile fully encompasses the period and the profile
+// has not already completed. The condition ensures that the metadata is applied
+// only if all execution during its scope was seen in the profile. This avoids
+// biasng the samples towards the 'middle' of the execution seen during the
+// metadata scope (i.e. because the start or end of execution was missed), at
+// the cost of missing execution that are longer than the profiling period, or
+// extend before or after it. |period_end| must be <= TimeTicks::Now().
+BASE_EXPORT void ApplyMetadataToPastSamples(TimeTicks period_start,
+                                            TimeTicks period_end,
+                                            StringPiece name,
+                                            int64_t value);
+BASE_EXPORT void ApplyMetadataToPastSamples(TimeTicks period_start,
+                                            TimeTicks period_end,
+                                            StringPiece name,
+                                            int64_t key,
+                                            int64_t value);
+
 // Returns the process-global metadata recorder instance used for tracking
 // sampling profiler metadata.
 //

base/profiler/stack_sampling_profiler.cc

@@ -128,6 +128,13 @@ class StackSamplingProfiler::SamplingThread : public Thread {
   // additional, non-native-code unwind scenarios.
   void AddAuxUnwinder(int collection_id, std::unique_ptr<Unwinder> unwinder);
 
+  // Applies the metadata to already recorded samples in all collections.
+  void ApplyMetadataToPastSamples(base::TimeTicks period_start,
+                                  base::TimeTicks period_end,
+                                  int64_t name_hash,
+                                  Optional<int64_t> key,
+                                  int64_t value);
+
   // Removes an active collection based on its collection id, forcing it to run
   // its callback if any data has been collected. This can be called externally
   // from any thread.
@@ -179,6 +186,11 @@ class StackSamplingProfiler::SamplingThread : public Thread {
   void AddCollectionTask(std::unique_ptr<CollectionContext> collection);
   void AddAuxUnwinderTask(int collection_id,
                           std::unique_ptr<Unwinder> unwinder);
+  void ApplyMetadataToPastSamplesTask(base::TimeTicks period_start,
+                                      base::TimeTicks period_end,
+                                      int64_t name_hash,
+                                      Optional<int64_t> key,
+                                      int64_t value);
   void RemoveCollectionTask(int collection_id);
   void RecordSampleTask(int collection_id);
   void ShutdownTask(int add_events);
@@ -342,6 +354,23 @@ void StackSamplingProfiler::SamplingThread::AddAuxUnwinder(
                           collection_id, std::move(unwinder)));
 }
 
+void StackSamplingProfiler::SamplingThread::ApplyMetadataToPastSamples(
+    base::TimeTicks period_start,
+    base::TimeTicks period_end,
+    int64_t name_hash,
+    Optional<int64_t> key,
+    int64_t value) {
+  ThreadExecutionState state;
+  scoped_refptr<SingleThreadTaskRunner> task_runner = GetTaskRunner(&state);
+  if (state != RUNNING)
+    return;
+  DCHECK(task_runner);
+  task_runner->PostTask(
+      FROM_HERE, BindOnce(&SamplingThread::ApplyMetadataToPastSamplesTask,
+                          Unretained(this), period_start, period_end, name_hash,
+                          key, value));
+}
+
 void StackSamplingProfiler::SamplingThread::Remove(int collection_id) {
   // This is not to be run on the sampling thread.
 
@@ -489,6 +518,20 @@ void StackSamplingProfiler::SamplingThread::AddAuxUnwinderTask(
   loc->second->sampler->AddAuxUnwinder(std::move(unwinder));
 }
 
+void StackSamplingProfiler::SamplingThread::ApplyMetadataToPastSamplesTask(
+    base::TimeTicks period_start,
+    base::TimeTicks period_end,
+    int64_t name_hash,
+    Optional<int64_t> key,
+    int64_t value) {
+  DCHECK_EQ(GetThreadId(), PlatformThread::CurrentId());
+  ProfileBuilder::MetadataItem item(name_hash, key, value);
+  for (auto& id_collection_pair : active_collections_) {
+    id_collection_pair.second->profile_builder->ApplyMetadataRetrospectively(
+        period_start, period_end, item);
+  }
+}
+
 void StackSamplingProfiler::SamplingThread::AddCollectionTask(
     std::unique_ptr<CollectionContext> collection) {
   DCHECK_EQ(GetThreadId(), PlatformThread::CurrentId());
@@ -761,4 +804,15 @@ void StackSamplingProfiler::AddAuxUnwinder(std::unique_ptr<Unwinder> unwinder) {
                                                 std::move(unwinder));
 }
 
+// static
+void StackSamplingProfiler::ApplyMetadataToPastSamples(
+    base::TimeTicks period_start,
+    base::TimeTicks period_end,
+    int64_t name_hash,
+    Optional<int64_t> key,
+    int64_t value) {
+  SamplingThread::GetInstance()->ApplyMetadataToPastSamples(
+      period_start, period_end, name_hash, key, value);
+}
+
 }  // namespace base

base/profiler/stack_sampling_profiler.h

@@ -10,6 +10,7 @@
 
 #include "base/base_export.h"
 #include "base/macros.h"
+#include "base/optional.h"
 #include "base/profiler/profile_builder.h"
 #include "base/profiler/sampling_profiler_thread_token.h"
 #include "base/synchronization/waitable_event.h"
@@ -157,6 +158,22 @@ class BASE_EXPORT StackSamplingProfiler {
   // the target thread.
   class SamplingThread;
 
+  // Friend the global function from sample_metadata.cc so that it can call into
+  // the function below.
+  friend void ApplyMetadataToPastSamplesImpl(TimeTicks period_start,
+                                             TimeTicks period_end,
+                                             int64_t name_hash,
+                                             Optional<int64_t> key,
+                                             int64_t value);
+
+  // Apply metadata to already recorded samples. See the
+  // ApplyMetadataToPastSamples() docs in sample_metadata.h.
+  static void ApplyMetadataToPastSamples(TimeTicks period_start,
+                                         TimeTicks period_end,
+                                         int64_t name_hash,
+                                         Optional<int64_t> key,
+                                         int64_t value);
+
   // The thread whose stack will be sampled.
   SamplingProfilerThreadToken thread_token_;
 

base/profiler/stack_sampling_profiler_unittest.cc

@@ -19,8 +19,10 @@
 #include "base/location.h"
 #include "base/macros.h"
 #include "base/memory/ptr_util.h"
+#include "base/metrics/metrics_hashes.h"
 #include "base/native_library.h"
 #include "base/path_service.h"
+#include "base/profiler/sample_metadata.h"
 #include "base/profiler/stack_sampler.h"
 #include "base/profiler/stack_sampling_profiler.h"
 #include "base/profiler/stack_sampling_profiler_test_util.h"
@@ -122,24 +124,23 @@ CallThroughOtherLibrary(NativeLibrary library, OnceClosure wait_for_sample) {
   return {start_program_counter, end_program_counter};
 }
 
+// State provided to the ProfileBuilder's ApplyMetadataRetrospectively function.
+struct RetrospectiveMetadata {
+  TimeTicks period_start;
+  TimeTicks period_end;
+  ProfileBuilder::MetadataItem item;
+};
+
 // Profile consists of a set of samples and other sampling information.
 struct Profile {
-  Profile() = default;
-  Profile(Profile&& other) = default;
-  Profile(const std::vector<std::vector<Frame>>& samples,
-          int metadata_count,
-          TimeDelta profile_duration,
-          TimeDelta sampling_period);
-
-  ~Profile() = default;
-
-  Profile& operator=(Profile&& other) = default;
-
   // The collected samples.
   std::vector<std::vector<Frame>> samples;
 
   // The number of invocations of RecordMetadata().
-  int metadata_count;
+  int record_metadata_count;
+
+  // The retrospective metadata requests.
+  std::vector<RetrospectiveMetadata> retrospective_metadata;
 
   // Duration of this profile.
   TimeDelta profile_duration;
@@ -148,15 +149,6 @@ struct Profile {
   TimeDelta sampling_period;
 };
 
-Profile::Profile(const std::vector<std::vector<Frame>>& samples,
-                 int metadata_count,
-                 TimeDelta profile_duration,
-                 TimeDelta sampling_period)
-    : samples(samples),
-      metadata_count(metadata_count),
-      profile_duration(profile_duration),
-      sampling_period(sampling_period) {}
-
 // The callback type used to collect a profile. The passed Profile is move-only.
 // Other threads, including the UI thread, may block on callback completion so
 // this should run as quickly as possible.
@@ -174,6 +166,9 @@ class TestProfileBuilder : public ProfileBuilder {
   ModuleCache* GetModuleCache() override;
   void RecordMetadata(
       ProfileBuilder::MetadataProvider* metadata_provider) override;
+  void ApplyMetadataRetrospectively(TimeTicks period_start,
+                                    TimeTicks period_end,
+                                    const MetadataItem& item) override;
   void OnSampleCompleted(std::vector<Frame> sample,
                          TimeTicks sample_timestamp) override;
   void OnProfileCompleted(TimeDelta profile_duration,
@@ -186,7 +181,10 @@ class TestProfileBuilder : public ProfileBuilder {
   std::vector<std::vector<Frame>> samples_;
 
   // The number of invocations of RecordMetadata().
-  int metadata_count_ = 0;
+  int record_metadata_count_ = 0;
+
+  // The retrospective metadata requests.
+  std::vector<RetrospectiveMetadata> retrospective_metadata_;
 
   // Callback made when sampling a profile completes.
   ProfileCompletedCallback callback_;
@@ -206,7 +204,15 @@ ModuleCache* TestProfileBuilder::GetModuleCache() {
 
 void TestProfileBuilder::RecordMetadata(
     ProfileBuilder::MetadataProvider* metadata_provider) {
-  ++metadata_count_;
+  ++record_metadata_count_;
+}
+
+void TestProfileBuilder::ApplyMetadataRetrospectively(
+    TimeTicks period_start,
+    TimeTicks period_end,
+    const MetadataItem& item) {
+  retrospective_metadata_.push_back(
+      RetrospectiveMetadata{period_start, period_end, item});
 }
 
 void TestProfileBuilder::OnSampleCompleted(std::vector<Frame> sample,
@@ -216,8 +222,9 @@ void TestProfileBuilder::OnSampleCompleted(std::vector<Frame> sample,
 
 void TestProfileBuilder::OnProfileCompleted(TimeDelta profile_duration,
                                             TimeDelta sampling_period) {
-  std::move(callback_).Run(
-      Profile(samples_, metadata_count_, profile_duration, sampling_period));
+  std::move(callback_).Run(Profile{samples_, record_metadata_count_,
+                                   retrospective_metadata_, profile_duration,
+                                   sampling_period});
 }
 
 // Loads the other library, which defines a function to be called in the
@@ -866,7 +873,7 @@ PROFILER_TEST_F(StackSamplingProfilerTest, ProfileGeneralInfo) {
 
         // The number of invocations of RecordMetadata() should be equal to the
         // number of samples recorded.
-        EXPECT_EQ(3, profiler_info.profile.metadata_count);
+        EXPECT_EQ(3, profiler_info.profile.record_metadata_count);
       }));
 }
 
@@ -1370,4 +1377,101 @@ PROFILER_TEST_F(StackSamplingProfilerTest, AddAuxUnwinder_AfterStop) {
   // since the collection has stopped.
 }
 
+// Checks that requests to apply metadata to past samples are passed on to the
+// profile builder.
+PROFILER_TEST_F(StackSamplingProfilerTest,
+                ApplyMetadataToPastSamples_PassedToProfileBuilder) {
+  // Runs the passed closure on the profiler thread after a sample is taken.
+  class PostSampleInvoker : public StackSamplerTestDelegate {
+   public:
+    explicit PostSampleInvoker(RepeatingClosure post_sample_closure)
+        : post_sample_closure_(std::move(post_sample_closure)) {}
+
+    void OnPreStackWalk() override { post_sample_closure_.Run(); }
+
+   private:
+    RepeatingClosure post_sample_closure_;
+  };
+
+  // Thread-safe representation of the times that samples were taken.
+  class SynchronizedSampleTimes {
+   public:
+    void AddNow() {
+      AutoLock lock(lock_);
+      times_.push_back(TimeTicks::Now());
+    }
+
+    std::vector<TimeTicks> GetTimes() {
+      AutoLock lock(lock_);
+      return times_;
+    }
+
+   private:
+    Lock lock_;
+    std::vector<TimeTicks> times_;
+  };
+
+  SamplingParams params;
+  params.sampling_interval = TimeDelta::FromMilliseconds(10);
+  // 10,000 samples ensures the profiler continues running until manually
+  // stopped, after applying metadata.
+  params.samples_per_profile = 10000;
+
+  UnwindScenario scenario(BindRepeating(&CallWithPlainFunction));
+
+  std::vector<TimeTicks> sample_times;
+  Profile profile;
+  WithTargetThread(
+      &scenario,
+      BindLambdaForTesting(
+          [&](SamplingProfilerThreadToken target_thread_token) {
+            SynchronizedSampleTimes synchronized_sample_times;
+            WaitableEvent sample_seen(WaitableEvent::ResetPolicy::AUTOMATIC);
+            PostSampleInvoker post_sample_invoker(BindLambdaForTesting([&]() {
+              synchronized_sample_times.AddNow();
+              sample_seen.Signal();
+            }));
+
+            StackSamplingProfiler profiler(
+                target_thread_token, params,
+                std::make_unique<TestProfileBuilder>(
+                    module_cache(),
+                    BindLambdaForTesting([&profile](Profile result_profile) {
+                      profile = std::move(result_profile);
+                    })),
+                &post_sample_invoker);
+            profiler.Start();
+            // Wait for 5 samples to be collected.
+            for (int i = 0; i < 5; ++i)
+              sample_seen.Wait();
+            sample_times = synchronized_sample_times.GetTimes();
+            // Record metadata on past samples, with and without a key value.
+            // The range [times[1], times[3]] is guaranteed to include only
+            // samples 2 and 3, and likewise [times[2], times[4]] is guaranteed
+            // to include only samples 3 and 4.
+            ApplyMetadataToPastSamples(sample_times[1], sample_times[3],
+                                       "TestMetadata1", 10);
+            ApplyMetadataToPastSamples(sample_times[2], sample_times[4],
+                                       "TestMetadata2", 100, 11);
+            profiler.Stop();
+          }));
+
+  ASSERT_EQ(2u, profile.retrospective_metadata.size());
+
+  const RetrospectiveMetadata& metadata1 = profile.retrospective_metadata[0];
+  EXPECT_EQ(sample_times[1], metadata1.period_start);
+  EXPECT_EQ(sample_times[3], metadata1.period_end);
+  EXPECT_EQ(HashMetricName("TestMetadata1"), metadata1.item.name_hash);
+  EXPECT_FALSE(metadata1.item.key.has_value());
+  EXPECT_EQ(10, metadata1.item.value);
+
+  const RetrospectiveMetadata& metadata2 = profile.retrospective_metadata[1];
+  EXPECT_EQ(sample_times[2], metadata2.period_start);
+  EXPECT_EQ(sample_times[4], metadata2.period_end);
+  EXPECT_EQ(HashMetricName("TestMetadata2"), metadata2.item.name_hash);
+  ASSERT_TRUE(metadata2.item.key.has_value());
+  EXPECT_EQ(100, *metadata2.item.key);
+  EXPECT_EQ(11, metadata2.item.value);
+}
+
 }  // namespace base