[tracing] Introduce config to setup memory-infra peak detection

base/trace_event/trace_config.cc

 // Copyright (c) 2015 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 "base/trace_event/trace_config.h"
 
 #include <stddef.h>
 
 #include <utility>
 
@@ skipping 30 matching lines @@
 const char kExcludedCategoriesParam[] = "excluded_categories";
 const char kSyntheticDelaysParam[] = "synthetic_delays";
 
 const char kSyntheticDelayCategoryFilterPrefix[] = "DELAY(";
 
 // String parameters that is used to parse memory dump config in trace config
 // string.
 const char kMemoryDumpConfigParam[] = "memory_dump_config";
 const char kAllowedDumpModesParam[] = "allowed_dump_modes";
 const char kTriggersParam[] = "triggers";
-const char kPeriodicIntervalParam[] = "periodic_interval_ms";
-const char kModeParam[] = "mode";
+const char kTriggerModeParam[] = "mode";
+const char kMinTimeBetweenDumps[] = "min_time_between_dumps_ms";
+const char kTriggerTypeParam[] = "type";
+const char kPeriodicIntervalParam[] = "periodic_interval_ms";  // legacy config

[Primiano Tucci (use gerrit), 2016/12/15 15:40:43]

maybe add Legacy to the name (kPeriodicIntervalLegacyParam)?

[ssid, 2016/12/15 19:48:07]

Done.

 const char kHeapProfilerOptions[] = "heap_profiler_options";
 const char kBreakdownThresholdBytes[] = "breakdown_threshold_bytes";
 
 // String parameters used to parse category event filters.
 const char kEventFiltersParam[] = "event_filters";
 const char kFilterPredicateParam[] = "filter_predicate";
 const char kFilterArgsParam[] = "filter_args";
 
 // Default configuration of memory dumps.
 const TraceConfig::MemoryDumpConfig::Trigger kDefaultHeavyMemoryDumpTrigger = {
-    2000,  // periodic_interval_ms
-    MemoryDumpLevelOfDetail::DETAILED};
+    2000,  // min_time_between_dumps_ms
+    MemoryDumpLevelOfDetail::DETAILED, MemoryDumpType::PERIODIC_INTERVAL};
 const TraceConfig::MemoryDumpConfig::Trigger kDefaultLightMemoryDumpTrigger = {
-    250,  // periodic_interval_ms
-    MemoryDumpLevelOfDetail::LIGHT};
+    250,  // min_time_between_dumps_ms
+    MemoryDumpLevelOfDetail::LIGHT, MemoryDumpType::PERIODIC_INTERVAL};
 
 class ConvertableTraceConfigToTraceFormat
     : public base::trace_event::ConvertableToTraceFormat {
  public:
   explicit ConvertableTraceConfigToTraceFormat(const TraceConfig& trace_config)
       : trace_config_(trace_config) {}
 
   ~ConvertableTraceConfigToTraceFormat() override {}
 
   void AppendAsTraceFormat(std::string* out) const override {
@@ skipping 507 matching lines @@
   // Set triggers
   memory_dump_config_.triggers.clear();
   const ListValue* trigger_list = nullptr;
   if (memory_dump_config.GetList(kTriggersParam, &trigger_list) &&
       trigger_list->GetSize() > 0) {
     for (size_t i = 0; i < trigger_list->GetSize(); ++i) {
       const DictionaryValue* trigger = nullptr;
       if (!trigger_list->GetDictionary(i, &trigger))
         continue;
 
+      MemoryDumpConfig::Trigger dump_config;
       int interval = 0;
-      if (!trigger->GetInteger(kPeriodicIntervalParam, &interval))
-        continue;
+      if (!trigger->GetInteger(kMinTimeBetweenDumps, &interval)) {
+        // If "min_time_between_dumps_ms" param was not given, then the trace
+        // config uses old format where only periodic dumps are supported.
+        trigger->GetInteger(kPeriodicIntervalParam, &interval);

[Primiano Tucci (use gerrit), 2016/12/15 15:40:43]

don't you have to keep the path where you skip if you don't pass neither the new
(min_time) nor the legacy param?
I.e. shouldn't this be

if (!trigger->getInteger(kPeriodicIntervalParam)
  continue
?

[ssid, 2016/12/15 19:48:07]

um, this will happen if someone writes:
memory_dump_config : { triggers [
  { "something_else": 12 }
]}
There is a dcheck to verify if the interval is set. So, lets just crash instead.

[Primiano Tucci (use gerrit), 2016/12/16 12:13:55]

Yeah I just didn't see the "!", sorry ignore my previous comment.
Just find this backward compat logic a bit hard to reason about.
Wouldn't be a bit more logical saying:

"if you are not providing the |type| you must be using the old-style config (in
which case you check for the presence of the legacy param). Otherwise you must
be passing a type and the new |min_interval| key"

+        dump_config.trigger_type = MemoryDumpType::PERIODIC_INTERVAL;
+      } else {
+        std::string trigger_type_str;
+        trigger->GetString(kTriggerTypeParam, &trigger_type_str);
+        dump_config.trigger_type = StringToMemoryDumpType(trigger_type_str);
+      }
+      DCHECK_GT(interval, 0);
+      dump_config.min_time_between_dumps_ms = static_cast<uint32_t>(interval);
 
-      DCHECK_GT(interval, 0);
-      MemoryDumpConfig::Trigger dump_config;
-      dump_config.periodic_interval_ms = static_cast<uint32_t>(interval);
       std::string level_of_detail_str;
-      trigger->GetString(kModeParam, &level_of_detail_str);
+      trigger->GetString(kTriggerModeParam, &level_of_detail_str);
       dump_config.level_of_detail =
           StringToMemoryDumpLevelOfDetail(level_of_detail_str);
+
       memory_dump_config_.triggers.push_back(dump_config);
     }
   }
 
   // Set heap profiler options
   const DictionaryValue* heap_profiler_options = nullptr;
   if (memory_dump_config.GetDictionary(kHeapProfilerOptions,
                                        &heap_profiler_options)) {
     int min_size_bytes = 0;
     if (heap_profiler_options->GetInteger(kBreakdownThresholdBytes,
@@ skipping 128 matching lines @@
     auto allowed_modes = MakeUnique<ListValue>();
     for (auto dump_mode : memory_dump_config_.allowed_dump_modes)
       allowed_modes->AppendString(MemoryDumpLevelOfDetailToString(dump_mode));
 
     auto memory_dump_config = MakeUnique<DictionaryValue>();
     memory_dump_config->Set(kAllowedDumpModesParam, std::move(allowed_modes));
 
     auto triggers_list = MakeUnique<ListValue>();
     for (const auto& config : memory_dump_config_.triggers) {
       auto trigger_dict = MakeUnique<DictionaryValue>();
-      trigger_dict->SetInteger(kPeriodicIntervalParam,
-                               static_cast<int>(config.periodic_interval_ms));
+      trigger_dict->SetString(kTriggerTypeParam,
+                              MemoryDumpTypeToString(config.trigger_type));
+      trigger_dict->SetInteger(
+          kMinTimeBetweenDumps,
+          static_cast<int>(config.min_time_between_dumps_ms));
       trigger_dict->SetString(
-          kModeParam, MemoryDumpLevelOfDetailToString(config.level_of_detail));
+          kTriggerModeParam,
+          MemoryDumpLevelOfDetailToString(config.level_of_detail));
       triggers_list->Append(std::move(trigger_dict));
     }
 
     // Empty triggers will still be specified explicitly since it means that
     // the periodic dumps are not enabled.
     memory_dump_config->Set(kTriggersParam, std::move(triggers_list));
 
     if (memory_dump_config_.heap_profiler_options.breakdown_threshold_bytes !=
         MemoryDumpConfig::HeapProfiler::kDefaultBreakdownThresholdBytes) {
       auto options = MakeUnique<DictionaryValue>();
@@ skipping 81 matching lines @@
     StringPiece str) {
   return str.empty() || str.front() == ' ' || str.back() == ' ';
 }
 
 bool TraceConfig::HasIncludedPatterns() const {
   return !included_categories_.empty();
 }
 
 }  // namespace trace_event
 }  // namespace base