[tracing] Implement polling in MemoryDumpManager

base/trace_event/memory_dump_trigger.h

Index: base/trace_event/memory_dump_trigger.h
diff --git a/base/trace_event/memory_dump_trigger.h b/base/trace_event/memory_dump_trigger.h
new file mode 100644
index 0000000000000000000000000000000000000000..dd4738b6134fc9d52571a96623e533f82ae8fd05
--- /dev/null
+++ b/base/trace_event/memory_dump_trigger.h
@@ -0,0 +1,77 @@
+// Copyright 2016 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 BASE_TRACE_EVENT_MEMORY_DUMP_TRIGGER_H
+#define BASE_TRACE_EVENT_MEMORY_DUMP_TRIGGER_H
+
+#include "base/atomicops.h"
+#include "base/base_export.h"
+#include "base/gtest_prod_util.h"
+#include "base/memory/ref_counted.h"
+#include "base/timer/timer.h"
+#include "base/trace_event/trace_config.h"
+
+namespace base {
+class SingleThreadTaskRunner;
+
+namespace trace_event {
+
+class MemoryDumpManager;
+
+// Sets up memory dump triggers to start global dump requests based on the trace
+// config.
+class BASE_EXPORT MemoryDumpTrigger {

[Primiano Tucci (use gerrit), 2017/01/18 16:16:06]

I think a better name for this class would be MemoryDumpScheduler. If you think
about it, this is what this class really does: decide when to trigger dumps
depending on the various configuration.

[ssid, 2017/01/20 23:07:27]

Makes sense. I didn't find a better name.

+ public:
+  using TriggerList = std::vector<TraceConfig::MemoryDumpConfig::Trigger>;
+
+  MemoryDumpTrigger(MemoryDumpManager* mdm_, bool is_coordinator);
+  ~MemoryDumpTrigger();
+
+  // Sets up peak dump triggers for global memory dumps according to
+  // |trigger_list|.
+  void SetupPeakTriggers(

[Primiano Tucci (use gerrit), 2017/01/18 16:16:07]

This one I find a bit confusing. You require to pass a trigger_list but then
expect that the trigger_list really contains exactly one trigger of
type==PEAK_MEMORY_USAGE. It smells a bit about bad layering.
What I would expect here instead is:
void SetPeakTrigger(uint32_t min_time_between_dumps_ms, MDLevelOfDetail)

[ssid, 2017/01/20 23:07:27]

2 reasons:
1. Maybe in future we will need more than one peak trigger.
2. It is consistent with the other function that sets up periodic dump triggers.

But, yes it's a bit weird to pass a list of config here. I changed it to just
one.

+      const TriggerList& trigger_list,
+      scoped_refptr<SingleThreadTaskRunner> polling_task_runner);
+
+  // Sets up periodic dump triggers for global memory dumps according to
+  // |trigger_list|.
+  void SetupPeriodicTriggers(const TriggerList& trigger_list);
+
+  // Disables all triggers. It must be called after the polling task runner
+  // stops running.
+  void Disable();

[Primiano Tucci (use gerrit), 2017/01/18 16:16:07]

I'd be a bit more explicit and call this either ClearAllTriggers() or
DisableAllTriggers()
Disable() makes me doubt whether one can keep using this class afterwards.

[ssid, 2017/01/20 23:07:27]

Done.

+
+  // Returns true if periodic timer is running.
+  bool IsPeriodicDumpTimerRunning() const;

[Primiano Tucci (use gerrit), 2017/01/18 16:16:07]

"Timer running" here is an implementation detail. I think 
ArePeriodicDumpsEnabled or something similar is cleaner

[ssid, 2017/01/20 23:07:27]

Done.

+
+ private:
+  FRIEND_TEST_ALL_PREFIXES(MemoryDumpManagerTest, TestTriggerDumpAtPeak);
+  FRIEND_TEST_ALL_PREFIXES(MemoryDumpManagerTest, TestPollingOnDumpThread);
+
+  static void SetPollingIntervalForTesting(uint32_t interval);
+
+  // Periodically called by the timer.
+  void RequestPeriodicGlobalDump();
+
+  // Called for polling memory usage and trigger dumps if peak is detected.
+  void PollMemoryOnPollingThread();
+
+  MemoryDumpManager* mdm_;
+
+  // When true, this trigger is in charge of coordinating periodic dumps.
+  bool is_coordinator_;

[Primiano Tucci (use gerrit), 2017/01/18 16:16:07]

Hmm I don't think this should be here, at most in the MDM (in fact you make very
little use of this here)
If you think about that, coordinator is a impl detail of MDM. 
This class has a specific responsibility: when instructed it does all the timer
and peak scheduling.
It should be responsibility of the caller (MDM) to figure out if we need such
responsibilities in the current process or not.

[ssid, 2017/01/20 23:07:27]

Yes I think it's fair to keep this in MDM. I moved it here because there was no
other use of this bool in MDM other than to not set periodic timer.

+
+  // Timer for periodic dumps.
+  RepeatingTimer timer_;
+
+  subtle::AtomicWord polling_enabled_;
+  scoped_refptr<SingleThreadTaskRunner> polling_task_runner_;
+
+  DISALLOW_COPY_AND_ASSIGN(MemoryDumpTrigger);
+};
+
+}  // namespace trace_event
+}  // namespace base
+
+#endif  // BASE_TRACE_EVENT_MEMORY_DUMP_TRIGGER_H