Task manager: Get physical memory efficiently for all processes from SharedSampler

chrome/browser/task_manager/sampling/shared_sampler_unittest_win.cc

Index: chrome/browser/task_manager/sampling/shared_sampler_unittest_win.cc
diff --git a/chrome/browser/task_manager/sampling/shared_sampler_unittest_win.cc b/chrome/browser/task_manager/sampling/shared_sampler_unittest_win.cc
new file mode 100644
index 0000000000000000000000000000000000000000..a1608830c9a53a17fc8605047ff6e7d916db648f
--- /dev/null
+++ b/chrome/browser/task_manager/sampling/shared_sampler_unittest_win.cc
@@ -0,0 +1,153 @@
+// 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.
+
+#include "chrome/browser/task_manager/sampling/shared_sampler.h"
+
+#include <windows.h>
+
+#include <memory>
+
+#include "base/callback.h"
+#include "base/macros.h"
+#include "base/memory/ref_counted.h"
+#include "base/run_loop.h"
+#include "base/sequenced_task_runner.h"
+#include "chrome/browser/task_manager/task_manager_observer.h"
+#include "content/public/browser/browser_thread.h"
+#include "content/public/test/test_browser_thread_bundle.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace task_manager {
+
+// This test class drives SharedSampler in a way similar to the real
+// implementation in TaskManagerImpl and TaskGroup.
+class SharedSamplerTest : public testing::Test {
+ public:
+  SharedSamplerTest()
+    : expected_refresh_type_(0), finished_refresh_type_(0),

[brucedawson, 2016/08/26 01:02:10]

We are now allowed to initialize member variables where they are declared
instead of in the constructor. I think it makes it clearer. Thoughts?

See "Non-Static Class Member Initializers" in https://chromium-cpp.appspot.com/

[stanisc, 2016/08/26 20:47:56]

Yes, I know we can do this now with C++ 11. Initially I initialized a couple of
integer fields inline but later thought that it would be better to initialize
all fields in one place. Anyway, I've changed this the way you suggested.

[brucedawson, 2016/08/26 21:47:27]

It is possible to initialize all fields in the class definition and then
"re-initialize" some (blocking_pool_runner_) in the constructor. If this is done
in the initializer list then there is no cost - each element is only initialized
once.

Anyway, don't feel that I'm laying down the law, on this or other style issues -
these are judgment calls.

+      physical_bytes_(0), idle_wakeups_per_second_(-1),
+      blocking_pool_runner_(GetBlockingPoolRunner()),
+      shared_sampler_(new SharedSampler(blocking_pool_runner_)) {
+    shared_sampler_->RegisterCallbacks(
+        base::GetCurrentProcId(),
+        base::Bind(&SharedSamplerTest::OnIdleWakeupsRefreshDone,
+                   base::Unretained(this)),
+        base::Bind(&SharedSamplerTest::OnPhysicalMemoryUsageRefreshDone,
+                   base::Unretained(this)));
+   }
+
+  ~SharedSamplerTest() override {}
+
+ protected:
+  int64_t physical_bytes() const { return physical_bytes_; }
+
+  int idle_wakeups_per_second() const { return idle_wakeups_per_second_; }
+
+  int64_t finished_refresh_type() const { return finished_refresh_type_; }
+
+  void StartRefresh(int64_t refresh_flags) {
+    finished_refresh_type_ = 0;
+    expected_refresh_type_ = refresh_flags;
+    shared_sampler_->Refresh(base::GetCurrentProcId(), refresh_flags);
+  }
+
+  void WaitUntilRefreshDone() {
+    base::RunLoop run_loop;
+    quit_closure_ = run_loop.QuitWhenIdleClosure();
+    run_loop.Run();
+  }
+
+ private:
+  static scoped_refptr<base::SequencedTaskRunner> GetBlockingPoolRunner() {
+    base::SequencedWorkerPool* blocking_pool =
+        content::BrowserThread::GetBlockingPool();
+    return blocking_pool->GetSequencedTaskRunner(
+        blocking_pool->GetSequenceToken());
+  }
+
+  void OnRefreshTypeFinished(int64_t finished_refresh_type) {
+    finished_refresh_type_ |= finished_refresh_type;
+
+    if (finished_refresh_type_ == expected_refresh_type_)
+      quit_closure_.Run();
+  }
+
+  void OnPhysicalMemoryUsageRefreshDone(int64_t physical_bytes) {
+    physical_bytes_ = physical_bytes;
+    OnRefreshTypeFinished(REFRESH_TYPE_PHYSICAL_MEMORY);
+  }
+
+  void OnIdleWakeupsRefreshDone(int idle_wakeups_per_second) {
+    idle_wakeups_per_second_ = idle_wakeups_per_second;
+    OnRefreshTypeFinished(REFRESH_TYPE_IDLE_WAKEUPS);
+  }
+
+  int64_t expected_refresh_type_;
+  int64_t finished_refresh_type_;
+  base::Closure quit_closure_;
+
+  int64_t physical_bytes_;
+  int idle_wakeups_per_second_;
+
+  content::TestBrowserThreadBundle thread_bundle_;
+  scoped_refptr<base::SequencedTaskRunner> blocking_pool_runner_;
+  scoped_refptr<SharedSampler> shared_sampler_;
+
+  DISALLOW_COPY_AND_ASSIGN(SharedSamplerTest);
+};
+
+// Tests that Idle Wakeups per second value can be obtained from SharedSampler.
+TEST_F(SharedSamplerTest, IdleWakeups) {
+  StartRefresh(REFRESH_TYPE_IDLE_WAKEUPS);
+  WaitUntilRefreshDone();
+  EXPECT_EQ(REFRESH_TYPE_IDLE_WAKEUPS, finished_refresh_type());
+
+  // Since idle_wakeups_per_second is an incremental value that reflects delta
+  // between two refreshes, the first refresh always returns zero value. This
+  // is consistent with implementation on other platforms.
+  EXPECT_EQ(0, idle_wakeups_per_second());
+
+  // Do a short sleep - that should trigger a context switch.
+  ::Sleep(1);
+
+  // Do another refresh.
+  StartRefresh(REFRESH_TYPE_IDLE_WAKEUPS);
+  WaitUntilRefreshDone();
+
+  // Should get a greater than zero rate now.
+  EXPECT_LT(0, idle_wakeups_per_second());

[brucedawson, 2016/08/26 01:02:10]

If we except a greater than zero rate now then why aren't we using EXPECT_GT
(with the arguments swapped). It's good to minimize the semantic distance
between the code and comments.

See later comment also.

[stanisc, 2016/08/26 20:47:56]

The only reason I've done it this way is to keep it similar to EXPECT_EQ
statements where constants are almost always in the first place. I have no
problem changing this the way you suggested.

+}
+
+// Verifies that Memory (Private WS) value can be obtained from Shared Sampler.
+TEST_F(SharedSamplerTest, PhysicalMemory) {
+  StartRefresh(REFRESH_TYPE_PHYSICAL_MEMORY);
+  WaitUntilRefreshDone();
+  EXPECT_EQ(REFRESH_TYPE_PHYSICAL_MEMORY, finished_refresh_type());
+
+  int64_t initial_value = physical_bytes();
+
+  // Allocate a large continuos block of memory.
+  const int allocated_size = 4 * 1024 * 1024;
+  std::unique_ptr<uint8_t[]> memory_block(new uint8_t[allocated_size]);

[brucedawson, 2016/08/26 01:02:10]

On further reflection...

This could be simplified to:

    std::vector<uint8_t> memory_block(allocated_size);

In one of your previous changes it was a performance nuisance that vector
initialized its data to zero. In this case it is exactly what we want, and the
memset could be avoided. Or, just ignore this comment and leave it as is. In the
unit test which this CL deletes the working set was tested with vector, FWIW.

[stanisc, 2016/08/26 20:47:56]

Good suggestion. Done.

+  DCHECK(memory_block.get());
+  // Touch the memory block to initialize it to make sure it is reflected in the
+  // working set.
+  ::memset(memory_block.get(), 0, allocated_size);
+
+  StartRefresh(REFRESH_TYPE_PHYSICAL_MEMORY);
+  WaitUntilRefreshDone();
+
+  // Verify that physical bytes has increased accordingly.
+  EXPECT_LE(initial_value + allocated_size, physical_bytes());

[brucedawson, 2016/08/26 01:02:10]

I find this hard to read with EXPECT_LE. Have you considered swapping the
arguments and using EXPECT_GE?

Or maybe it's just me.

[stanisc, 2016/08/26 20:47:56]

Done.

+}
+
+// Verifies that multiple refresh types can be refreshed at the same time.
+TEST_F(SharedSamplerTest, MultipleRefreshTypes) {
+  StartRefresh(REFRESH_TYPE_IDLE_WAKEUPS | REFRESH_TYPE_PHYSICAL_MEMORY);
+  WaitUntilRefreshDone();
+  EXPECT_EQ(REFRESH_TYPE_IDLE_WAKEUPS | REFRESH_TYPE_PHYSICAL_MEMORY,
+            finished_refresh_type());
+}
+
+}  // namespace task_manager