Move MemoryLeakReportProto params into nested message field

Makes adding params to a memory leak report more straightforward.

BUG=chromium:382705
TEST=unit tests pass
R=isherman@chromium.org

Review URL: https://codereview.chromium.org/1892233002

Cr-Commit-Position: refs/heads/master@{#387825}

chrome/browser/metrics/leak_detector_controller.cc

@@ -17,7 +17,7 @@ namespace {
 // Reads parameters for the field trial variation. Any parameters not present in
 // the variation info or that cannot be parsed will be filled in with default
 // values. Returns a MemoryLeakReportProto with the parameter fields filled in.
-MemoryLeakReportProto GetVariationParameters() {
+MemoryLeakReportProto::Params GetVariationParameters() {
   // Variation parameter names.
   const char kFieldTrialName[] = "RuntimeMemoryLeakDetector";
   const char kSamplingRateParam[] = "sampling_rate";
@@ -76,28 +76,24 @@ MemoryLeakReportProto GetVariationParameters() {
     call_stack_suspicion_threshold = kDefaultCallStackSuspicionThreshold;
   }
 
-  MemoryLeakReportProto proto;
-  proto.set_sampling_rate(sampling_rate);
-  proto.set_max_stack_depth(max_stack_depth);
-  proto.set_analysis_interval_bytes(analysis_interval_kb * 1024);
-  proto.set_size_suspicion_threshold(size_suspicion_threshold);
-  proto.set_call_stack_suspicion_threshold(call_stack_suspicion_threshold);
-  return proto;
+  MemoryLeakReportProto_Params result;
+  result.set_sampling_rate(sampling_rate);
+  result.set_max_stack_depth(max_stack_depth);
+  result.set_analysis_interval_bytes(analysis_interval_kb * 1024);
+  result.set_size_suspicion_threshold(size_suspicion_threshold);
+  result.set_call_stack_suspicion_threshold(call_stack_suspicion_threshold);
+  return result;
 }
 
 }  // namespace
 
 LeakDetectorController::LeakDetectorController()
-    : leak_report_proto_template_(GetVariationParameters()) {
+    : params_(GetVariationParameters()) {
   LeakDetector* detector = LeakDetector::GetInstance();
   detector->AddObserver(this);
 
-  // Leak detector parameters are stored in |leak_report_proto_template_|.
-  const MemoryLeakReportProto& proto = leak_report_proto_template_;
-  detector->Init(proto.sampling_rate(), proto.max_stack_depth(),
-                 proto.analysis_interval_bytes(),
-                 proto.size_suspicion_threshold(),
-                 proto.call_stack_suspicion_threshold());
+  // Leak detector parameters are stored in |params_|.
+  detector->Init(params_);
 }
 
 LeakDetectorController::~LeakDetectorController() {
@@ -110,13 +106,9 @@ void LeakDetectorController::OnLeaksFound(
   DCHECK(thread_checker_.CalledOnValidThread());
 
   for (const auto& report : reports) {
-    // Initialize the new report with the protobuf template, which contains some
-    // pre-filled data (parameter values).
-    stored_reports_.push_back(leak_report_proto_template_);
-    MemoryLeakReportProto* proto = &stored_reports_.back();
-
-    // Merge in the other fields.
-    proto->MergeFrom(report);
+    // Store the report and insert stored parameters.
+    stored_reports_.push_back(report);
+    stored_reports_.back().mutable_params()->CopyFrom(params_);
   }
 }
 

chrome/browser/metrics/leak_detector_controller.h

@@ -34,12 +34,9 @@ class LeakDetectorController : public LeakDetector::Observer {
   // format.
   std::vector<MemoryLeakReportProto> stored_reports_;
 
-  // All incoming leak reports should be converted to a MemoryLeakReportProto
-  // starting with this template, which has some fields already filled in, e.g.
-  // leak detection parameters. Storing the LeakDetector parameters in the
-  // protobuf template avoids having to store these params as individual member
-  // variables.
-  const MemoryLeakReportProto leak_report_proto_template_;
+  // Contains all the parameters passed to LeakDetector. Store them in a single
+  // protobuf message instead of in separate member variables.
+  const MemoryLeakReportProto::Params params_;
 
   // For thread safety.
   base::ThreadChecker thread_checker_;

chrome/browser/metrics/leak_detector_controller_unittest.cc

@@ -69,11 +69,11 @@ TEST(LeakDetectorControllerTest, SingleReport) {
 
   // Check that default leak detector parameters are stored in the leak report.
   // Default values are listed in leak_detector_controller.cc.
-  EXPECT_DOUBLE_EQ(1.0f / 256, proto.sampling_rate());
-  EXPECT_EQ(4U, proto.max_stack_depth());
-  EXPECT_EQ(32U * 1024 * 1024, proto.analysis_interval_bytes());
-  EXPECT_EQ(4U, proto.size_suspicion_threshold());
-  EXPECT_EQ(4U, proto.call_stack_suspicion_threshold());
+  EXPECT_DOUBLE_EQ(1.0f / 256, proto.params().sampling_rate());
+  EXPECT_EQ(4U, proto.params().max_stack_depth());
+  EXPECT_EQ(32U * 1024 * 1024, proto.params().analysis_interval_bytes());
+  EXPECT_EQ(4U, proto.params().size_suspicion_threshold());
+  EXPECT_EQ(4U, proto.params().call_stack_suspicion_threshold());
 
   // No more reports.
   controller->GetLeakReports(&stored_reports);

components/metrics/leak_detector/leak_detector.cc

@@ -160,18 +160,15 @@ LeakDetector* LeakDetector::GetInstance() {
   return g_instance.Pointer();
 }
 
-void LeakDetector::Init(float sampling_rate,
-                        size_t max_call_stack_unwind_depth,
-                        uint64_t analysis_interval_bytes,
-                        uint32_t size_suspicion_threshold,
-                        uint32_t call_stack_suspicion_threshold) {
+void LeakDetector::Init(const MemoryLeakReportProto::Params& params) {
   DCHECK(thread_checker_.CalledOnValidThread());
-  DCHECK(sampling_rate > 0) << "Sampling rate cannot be zero or negative.";
+  DCHECK_GT(params.sampling_rate(), 0);
 
-  sampling_factor_ = base::saturated_cast<uint64_t>(sampling_rate * UINT64_MAX);
+  sampling_factor_ =
+      base::saturated_cast<uint64_t>(params.sampling_rate() * UINT64_MAX);
 
-  analysis_interval_bytes_ = analysis_interval_bytes;
-  max_call_stack_unwind_depth_ = max_call_stack_unwind_depth;
+  analysis_interval_bytes_ = params.analysis_interval_bytes();
+  max_call_stack_unwind_depth_ = params.max_stack_depth();
 
   MappingInfo mapping = {0};
 #if defined(OS_CHROMEOS)
@@ -187,8 +184,8 @@ void LeakDetector::Init(float sampling_rate,
   // whether |impl_| has already been initialized.
   CHECK(!impl_.get()) << "Cannot initialize LeakDetector more than once!";
   impl_.reset(new leak_detector::LeakDetectorImpl(
-      mapping.addr, mapping.size, size_suspicion_threshold,
-      call_stack_suspicion_threshold));
+      mapping.addr, mapping.size, params.size_suspicion_threshold(),
+      params.call_stack_suspicion_threshold()));
 
   // Register allocator hook functions. This must be done last since the
   // preceding code will need to call the allocator.

components/metrics/leak_detector/leak_detector.h

@@ -62,27 +62,10 @@ class LeakDetector {
   // Returns the sole instance, or creates it if it hasn't already been created.
   static LeakDetector* GetInstance();
 
-  // Initializer arguments:
-  // sampling_rate:
-  //     Pseudorandomly sample a fraction of the incoming allocations and frees,
-  //     based on hash values. Setting to 0 means no allocs/frees are sampled.
-  //     Setting to 1.0 or more means all allocs/frees are sampled. Anything in
-  //     between will result in an approximately that fraction of allocs/frees
-  //     being sampled.
-  // max_call_stack_unwind_depth:
-  //     The max number of call stack frames to unwind.
-  // analysis_interval_bytes:
-  //     Perform a leak analysis each time this many bytes have been allocated
-  //     since the previous analysis.
-  // size_suspicion_threshold, call_stack_suspicion_threshold:
-  //     A possible leak should be suspected this many times to take action on i
-  //     For size analysis, the action is to start profiling by call stack.
-  //     For call stack analysis, the action is to generate a leak report.
-  void Init(float sampling_rate,
-            size_t max_call_stack_unwind_depth,
-            uint64_t analysis_interval_bytes,
-            uint32_t size_suspicion_threshold,
-            uint32_t call_stack_suspicion_threshold);
+  // Initializes leak detector with a set of params given by |params|. See the
+  // definition of MemoryLeakReportProto::Params for info about individual
+  // parameters.
+  void Init(const MemoryLeakReportProto::Params& params);
 
   // Add |observer| to the list of stored Observers, i.e. |observers_|, to which
   // the leak detector will report leaks.

components/metrics/leak_detector/leak_detector_unittest.cc

@@ -67,10 +67,15 @@ class TestObserver : public LeakDetector::Observer {
 class LeakDetectorTest : public ::testing::Test {
  public:
   LeakDetectorTest() : detector_(LeakDetector::GetInstance()) {
-    detector_->Init(kDefaultSamplingRate, kDefaultMaxCallStackUnwindDepth,
-                    kDefaultAnalysisIntervalBytes,
-                    kDefaultSizeSuspicionThreshold,
-                    kDefaultCallStackSuspicionThreshold);
+    MemoryLeakReportProto::Params params;
+    params.set_sampling_rate(kDefaultSamplingRate);
+    params.set_max_stack_depth(kDefaultMaxCallStackUnwindDepth);
+    params.set_analysis_interval_bytes(kDefaultAnalysisIntervalBytes);
+    params.set_size_suspicion_threshold(kDefaultSizeSuspicionThreshold);
+    params.set_call_stack_suspicion_threshold(
+        kDefaultCallStackSuspicionThreshold);
+
+    detector_->Init(params);
   }
 
  protected:

components/metrics/proto/memory_leak_report.proto

@@ -8,7 +8,7 @@ option optimize_for = LITE_RUNTIME;
 
 package metrics;
 
-// Next tag: 8
+// Next tag: 5
 message MemoryLeakReportProto {
   // The call stack at which the leak was found. This is a list of offsets
   // within the program binary. The first entry is the deepest level of the call
@@ -29,27 +29,37 @@ message MemoryLeakReportProto {
 
   //////////////////////////////////////////////////////////////////////////////
 
-  // The rate at which allocations are pseudorandomly sampled. Ranges from 0 to
-  // 1. A rate of 1 means all incoming allocations are sampled by the leak
-  // detector, which is the maximum possible.
-  optional float sampling_rate = 3;
+  // Contains all parameters passed to the leak detector during initialization.
+  // Since these are known at the beginning, this message can be stored locally
+  // and then added to generated memory leak report protobufs.
+  //
+  // Next tag: 6
+  message Params {
+    // The rate at which allocations are pseudorandomly sampled. Ranges from 0
+    // to 1. A rate of 1 means all incoming allocations are sampled by the leak
+    // detector, which is the maximum possible.
+    optional float sampling_rate = 1;
+
+    // The max depth to which the call stacks were unwound by the leak detector.
+    // This may be greater than the size of |call_stack|.
+    optional uint32 max_stack_depth = 2;
 
-  // The max depth to which the call stacks were unwound by the leak detector.
-  // This may be greater than the size of |call_stack|.
-  optional uint32 max_stack_depth = 4;
+    // The leak analysis takes place every so often, with an interval based on
+    // the number of bytes allocated. This is independent of the sampling rate
+    // as it is computed from allocation sizes before sampling.
+    optional uint64 analysis_interval_bytes = 3;
 
-  // The leak analysis takes place every so often, with an interval based on the
-  // number of bytes allocated. This is independent of the sampling rate as it
-  // is computed from allocation sizes before sampling.
-  optional uint64 analysis_interval_bytes = 5;
+    // Suspicion thresholds used in leak analysis for size and call stacks,
+    // respectively. If an allocation size or call stack is suspected this many
+    // times in a row, the leak analysis escalates to the next level. For
+    // allocation sizes, the next level is to start analyzing by call stack. For
+    // call stacks, the next level is to generate a memory leak report.
+    optional uint32 size_suspicion_threshold = 4;
+    optional uint32 call_stack_suspicion_threshold = 5;
+  }
 
-  // Suspicion thresholds used in leak analysis for size and call stacks,
-  // respectively. If an allocation size or call stack is suspected this many
-  // times in a row, the leak analysis escalates to the next level. For
-  // allocation sizes, the next level is to start analyzing by call stack. For
-  // call stacks, the next level is to generate a memory leak report.
-  optional uint32 size_suspicion_threshold = 6;
-  optional uint32 call_stack_suspicion_threshold = 7;
+  // Parameters used to initialize the leak detector.
+  optional Params params = 3;
 
   //////////////////////////////////////////////////////////////////////////////
 
@@ -77,5 +87,5 @@ message MemoryLeakReportProto {
   // A new snapshot is taken every |analysis_interval_bytes| of memory
   // allocation. The oldest record is at the beginning. The most recent record,
   // taken at the time the report was generated, is at the end.
-  repeated AllocationBreakdown alloc_breakdown_history = 8;
+  repeated AllocationBreakdown alloc_breakdown_history = 4;
 }