memory-infra: Switch to MemoryPeakDetector and simplify MemoryDumpScheduler

base/trace_event/memory_dump_manager_unittest.cc

Index: base/trace_event/memory_dump_manager_unittest.cc
diff --git a/base/trace_event/memory_dump_manager_unittest.cc b/base/trace_event/memory_dump_manager_unittest.cc
index e126edd3972e052f5ad6515fcc39741bf5daeac3..2617841e9191029cfdadb571b2b3e7e031d3469d 100644
--- a/base/trace_event/memory_dump_manager_unittest.cc
+++ b/base/trace_event/memory_dump_manager_unittest.cc
@@ -244,10 +244,6 @@ class MemoryDumpManagerTest : public testing::Test {
     task_runner->PostTask(FROM_HERE, closure);
   }
 
-  void PollFastMemoryTotal(uint64_t* memory_total) {
-    mdm_->PollFastMemoryTotal(memory_total);
-  }
-
  protected:
   void InitializeMemoryDumpManager(bool is_coordinator) {
     mdm_->set_dumper_registrations_ignored_for_testing(true);
@@ -278,8 +274,7 @@ class MemoryDumpManagerTest : public testing::Test {
   void DisableTracing() { TraceLog::GetInstance()->SetDisabled(); }
 
   bool IsPeriodicDumpingEnabled() const {
-    return MemoryDumpScheduler::GetInstance()
-        ->IsPeriodicTimerRunningForTesting();
+    return MemoryDumpScheduler::GetInstance()->is_enabled_for_testing();
   }
 
   int GetMaxConsecutiveFailuresCount() const {
@@ -766,9 +761,6 @@ TEST_F(MemoryDumpManagerTest, TestPollingOnDumpThread) {
   std::unique_ptr<MockMemoryDumpProvider> mdp2(new MockMemoryDumpProvider());
   mdp1->enable_mock_destructor = true;
   mdp2->enable_mock_destructor = true;
-
-  EXPECT_CALL(*mdp1, SuspendFastMemoryPolling()).Times(1);
-  EXPECT_CALL(*mdp2, SuspendFastMemoryPolling()).Times(1);
   EXPECT_CALL(*mdp1, Destructor());
   EXPECT_CALL(*mdp2, Destructor());
 
@@ -777,53 +769,33 @@ TEST_F(MemoryDumpManagerTest, TestPollingOnDumpThread) {
   RegisterDumpProvider(mdp1.get(), nullptr, options);
 
   RunLoop run_loop;
-  scoped_refptr<SingleThreadTaskRunner> test_task_runner =
-      ThreadTaskRunnerHandle::Get();
+  auto test_task_runner = ThreadTaskRunnerHandle::Get();
   auto quit_closure = run_loop.QuitClosure();
-
-  const int kPollsToQuit = 10;
-  int call_count = 0;
   MemoryDumpManager* mdm = mdm_.get();
-  const auto poll_function1 = [&call_count, &test_task_runner, quit_closure,
-                               &mdp2, mdm, &options, kPollsToQuit,
-                               this](uint64_t* total) -> void {
-    ++call_count;
-    if (call_count == 1)
-      RegisterDumpProvider(mdp2.get(), nullptr, options, kMDPName);
-    else if (call_count == 4)
-      mdm->UnregisterAndDeleteDumpProviderSoon(std::move(mdp2));
-    else if (call_count == kPollsToQuit)
-      test_task_runner->PostTask(FROM_HERE, quit_closure);
-
-    // Record increase of 1 GiB of memory at each call.
-    *total = static_cast<uint64_t>(call_count) * 1024 * 1024 * 1024;
-  };
-  EXPECT_CALL(*mdp1, PollFastMemoryTotal(_))
-      .Times(testing::AtLeast(kPollsToQuit))
-      .WillRepeatedly(Invoke(poll_function1));
 
-  // Depending on the order of PostTask calls the mdp2 might be registered after
-  // all polls or in between polls.
-  EXPECT_CALL(*mdp2, PollFastMemoryTotal(_))
-      .Times(Between(0, kPollsToQuit - 1))
+  EXPECT_CALL(*mdp1, PollFastMemoryTotal(_))
+      .WillOnce(Invoke([&mdp2, options, this](uint64_t*) {
+        RegisterDumpProvider(mdp2.get(), nullptr, options);
+      }))
+      .WillOnce(Return())
+      .WillOnce(Invoke([mdm, &mdp2](uint64_t*) {
+        mdm->UnregisterAndDeleteDumpProviderSoon(std::move(mdp2));
+      }))
+      .WillOnce(Invoke([test_task_runner, quit_closure](uint64_t*) {
+        test_task_runner->PostTask(FROM_HERE, quit_closure);
+      }))
       .WillRepeatedly(Return());
 
-  MemoryDumpScheduler::SetPollingIntervalForTesting(1);
-  EnableTracingWithTraceConfig(
-      TraceConfigMemoryTestUtil::GetTraceConfig_PeakDetectionTrigger(3));
+  EXPECT_CALL(*mdp1, SuspendFastMemoryPolling()).Times(1);

[ssid, 2017/04/10 21:35:51]

Why not 

EXPECT_CALL(*mdp2, SuspendFastMemoryPolling()).Times(1);

Actually just realized ~ProcessMetricsMemoryDumpProvider() does not close the
fast_polling_statm_fd_

[Primiano Tucci (use gerrit), 2017/04/11 11:43:08]

Because I realized that there is no point calling SuspendFastMemoryPolling() at
unregistration, as the MDP will just be destroyed, and the FD will be closed in
its dtor. So I kept it only for the stop case.

I made the comment and the code more explicit though.
Unless i a missing something major, It does.
primiano-from-the-past was quite parnoid and asked you to use a base::ScopedFD
fast_polling_statm_fd_ :)
it should be auto closed.

[ssid, 2017/04/11 17:47:30]

Haha true!

 
-  int last_poll_to_request_dump = -2;
-  EXPECT_CALL(*delegate_, RequestGlobalMemoryDump(_, _))
-      .Times(testing::AtLeast(2))
-      .WillRepeatedly(Invoke([&last_poll_to_request_dump, &call_count](
-                                 const MemoryDumpRequestArgs& args,
-                                 const MemoryDumpCallback& callback) -> void {
-        // Minimum number of polls between dumps must be 3 (polling interval is
-        // 1ms).
-        EXPECT_GE(call_count - last_poll_to_request_dump, 3);
-        last_poll_to_request_dump = call_count;
-      }));
+  // |mdp2| should invoke exactly twice:
+  // - once after the registrarion, when |mdp1| hits the first Return()
+  // - the 2nd time when |mdp1| unregisters |mdp1|. The unregistration is
+  //   posted and will necessarily happen after the polling task.
+  EXPECT_CALL(*mdp2, PollFastMemoryTotal(_)).Times(2).WillRepeatedly(Return());
 
+  EnableTracingWithTraceConfig(
+      TraceConfigMemoryTestUtil::GetTraceConfig_PeakDetectionTrigger(1));
   run_loop.Run();
   DisableTracing();
   mdm_->UnregisterAndDeleteDumpProviderSoon(std::move(mdp1));
@@ -989,6 +961,7 @@ TEST_F(MemoryDumpManagerTest, TraceConfigExpectationsWhenIsCoordinator) {
   // process with a fully defined trigger config should cause periodic dumps to
   // be performed in the correct order.
   RunLoop run_loop;
+  auto test_task_runner = ThreadTaskRunnerHandle::Get();
   auto quit_closure = run_loop.QuitClosure();
 
   const int kHeavyDumpRate = 5;
@@ -1003,10 +976,11 @@ TEST_F(MemoryDumpManagerTest, TraceConfigExpectationsWhenIsCoordinator) {
   EXPECT_CALL(delegate, RequestGlobalMemoryDump(IsLightDump(), _))
       .Times(kHeavyDumpRate - 2);
   EXPECT_CALL(delegate, RequestGlobalMemoryDump(IsLightDump(), _))
-      .WillOnce(Invoke([quit_closure](const MemoryDumpRequestArgs& args,
-                                      const MemoryDumpCallback& callback) {
-        ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE, quit_closure);
-      }));
+      .WillOnce(Invoke(
+          [test_task_runner, quit_closure](const MemoryDumpRequestArgs& args,
+                                           const MemoryDumpCallback& callback) {
+            test_task_runner->PostTask(FROM_HERE, quit_closure);
+          }));
 
   // Swallow all the final spurious calls until tracing gets disabled.
   EXPECT_CALL(delegate, RequestGlobalMemoryDump(_, _)).Times(AnyNumber());
@@ -1259,16 +1233,18 @@ TEST_F(MemoryDumpManagerTest, TestBackgroundTracingSetup) {
   InitializeMemoryDumpManager(true /* is_coordinator */);
 
   RunLoop run_loop;
+  auto test_task_runner = ThreadTaskRunnerHandle::Get();
   auto quit_closure = run_loop.QuitClosure();
 
   testing::InSequence sequence;
   EXPECT_CALL(*delegate_, RequestGlobalMemoryDump(IsBackgroundDump(), _))
       .Times(5);
   EXPECT_CALL(*delegate_, RequestGlobalMemoryDump(IsBackgroundDump(), _))
-      .WillOnce(Invoke([quit_closure](const MemoryDumpRequestArgs& args,
-                                      const MemoryDumpCallback& callback) {
-        ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE, quit_closure);
-      }));
+      .WillOnce(Invoke(
+          [test_task_runner, quit_closure](const MemoryDumpRequestArgs& args,
+                                           const MemoryDumpCallback& callback) {
+            test_task_runner->PostTask(FROM_HERE, quit_closure);
+          }));
   EXPECT_CALL(*delegate_, RequestGlobalMemoryDump(_, _)).Times(AnyNumber());
 
   EnableTracingWithTraceConfig(