Make task manager memory data more efficient and meaningful.

base/process/private_working_set_snapshot_win.cc

Index: base/process/private_working_set_snapshot_win.cc
diff --git a/base/process/private_working_set_snapshot_win.cc b/base/process/private_working_set_snapshot_win.cc
new file mode 100644
index 0000000000000000000000000000000000000000..de2e5517911829c425952067d73891c98f26e58d
--- /dev/null
+++ b/base/process/private_working_set_snapshot_win.cc
@@ -0,0 +1,169 @@
+// Copyright (c) 2013 The Chromium Authors. All rights reserved.

[ncarter (slow), 2015/06/26 21:46:16]

2015, no (c)

[brucedawson, 2015/06/27 00:13:05]

Done.

+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "base/process/private_working_set_snapshot.h"
+
+#include <algorithm>
+
+#if defined(OS_WIN)

[ncarter (slow), 2015/06/26 21:46:16]

This isn't needed. Since the file ends in _win, it will be excluded from the
build on other platforms.

[brucedawson, 2015/06/27 00:13:05]

The reason for the #if defined was to make the presubmit happy. Without it I get
this:

Your #include order seems to be broken. Remember to use the right collation
(LC_COLLATE=C) and check
https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Names_and_Or...
      base\process\private_working_set_snapshot_win.cc:9 \
      base\process\private_working_set_snapshot_win.cc:10

My assumption was that without the #if defined(OS_WIN) the presubmit check
couldn't tell that those were Windows header files and therefore wanted them
sorted in with <algorithm> and windows_version.h. Without the #if defined I can
only get past that warning by having all five #includes adjacent and sorted,
which is (I believe) incorrect.

Thoughts?

+#include <pdh.h>
+#include <pdhmsg.h>
+#include <windows.h>
+#endif
+
+#include "base/win/windows_version.h"
+
+namespace base {
+
+PrivateWorkingSetSnapshot::PrivateWorkingSetSnapshot()
+  : query_handle_(nullptr)
+{

[ncarter (slow), 2015/06/26 21:46:16]

"{" always goes on the previous line (here and elsewhere), and ":" gets a four
space indent.

Or do what I do, and let "git cl format" take care of this stuff.

[brucedawson, 2015/06/27 00:13:05]

Huh. Clearly I *know* that rule, and yet...

Okay, time to try git cl format. Huh. It works nicely. I thought it failed last
time but it certainly worked this time. Done.

+  // The Pdh APIs are supported on Windows XP, but the "Working Set - Private"
+  // counter that PrivateWorkingSetSnapshot depends on is not defined until
+  // Windows Vista. Early-out to avoid wasted effort. All queries will return
+  // zero and will have to use the fallback calculations.
+  if (base::win::GetVersion() < base::win::VERSION_VISTA)
+    return;
+
+  // Create a Pdh query
+  if (PdhOpenQuery(NULL, NULL, &query_handle_) != ERROR_SUCCESS)
+  {
+    query_handle_ = nullptr;
+    return;
+  }
+}
+
+PrivateWorkingSetSnapshot::~PrivateWorkingSetSnapshot()
+{
+  if (query_handle_)
+    PdhCloseQuery(query_handle_);
+}
+
+void PrivateWorkingSetSnapshot::AddToMonitorList(
+    const std::string& process_name)
+{
+  // Create the magic strings that will return a list of process IDs and a list
+  // of private working sets. The 'process_name' variable should be something
+  // like "chrome". The '*' character indicates that we want records for all
+  // processes whose names start with process_name - all chrome processes, but
+  // also all 'chrome_editor.exe' processes or other matching names. The excess
+  // information is unavoidable but harmless.
+  std::string process_id_query = "\\Process(" + process_name + "*)\\ID Process";
+  std::string private_ws_query = "\\Process(" + process_name +
+      "*)\\Working Set - Private";
+
+  // Add the two counters to the query.
+  PdhCounterPair new_counters;
+  if (PdhAddCounterA(query_handle_, process_id_query.c_str(),
+      NULL, &new_counters.process_id_handle) != ERROR_SUCCESS) {

[ncarter (slow), 2015/06/26 21:46:16]

Cases like this should get an additional 4 lines of indent. The common
interpretation of "wrapped function call arguments get a 4 space indent" rule
for it to be relative to the callable function -- so in this case, relative to
the P in PdhAddCounterA.

[brucedawson, 2015/06/27 00:13:05]

"git cl format" did it somewhat differently from that and it looks tidy so I'm
going to defer to it. That is, the new version of this is indented according to
git cl format.

[ncarter (slow), 2015/06/29 20:03:19]

As a rule you won't see me disagreeing with git cl format's choices. It produces
really tidy code.

+    return;
+  }
+
+  // If adding the second counter fails then we should remove the first one.
+  if (PdhAddCounterA(query_handle_, private_ws_query.c_str(),
+      NULL, &new_counters.private_ws_handle) != ERROR_SUCCESS) {
+    PdhRemoveCounter(new_counters.process_id_handle);
+  }
+
+  // Record the pair of counter query handles so that we can query them later.
+  counter_pairs_.push_back(new_counters);
+}
+
+// Query the system for working-set information for all monitored processes
+// and update the results cache. This function may take a few ms to run.
+// The time it takes seems to be independent of the number of processes it
+// retrieves data for. This makes it faster than using QueryWorkingSet as soon
+// as the process count exceeds two or three.

[ncarter (slow), 2015/06/26 21:46:16]

Don't duplicate comments between the .h and the .cc file.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Comm...

I think you'd be fine to have no comment here -- the comments in the body of the
function seem to suffice to describe how things work.

[brucedawson, 2015/06/27 00:13:05]

Done.

+void PrivateWorkingSetSnapshot::Sample()
+{
+  if (!query_handle_ || counter_pairs_.empty())
+    return;
+
+  // Destroy all previous data.
+  records_.resize(0);
+  // Record the requested data into PDH's internal buffers.
+  if (PdhCollectQueryData(query_handle_) != ERROR_SUCCESS)
+    return;
+
+  for (auto& counter_pair : counter_pairs_) {
+    // Find out how much space is required for the two counter arrays.
+    // A return code of PDH_MORE_DATA indicates that we should call again with
+    // the buffer size returned.
+    DWORD buffer_size1 = 0;
+    DWORD item_count1 = 0;
+    // Process IDs should be retrieved as PDH_FMT_LONG
+    if (PdhGetFormattedCounterArray(counter_pair.process_id_handle,
+        PDH_FMT_LONG, &buffer_size1, &item_count1, nullptr) != PDH_MORE_DATA)
+      continue;
+    DWORD buffer_size2 = 0;
+    DWORD item_count2 = 0;
+    // Working sets should be retrieved as PDH_FMT_LARGE (LONGLONG)
+    if (PdhGetFormattedCounterArray(counter_pair.private_ws_handle,

[ncarter (slow), 2015/06/26 21:46:15]

Optimization idea: since we bail if the sizes/counts aren't equal, it seems like
we could skip this second call, assume that the buffer sizes are equal, and use
the result of the "Retrieve the private working set data" operation to validate
that they were?

[brucedawson, 2015/06/27 00:13:05]

The calls to PdhGetFormattedCounterArray are very cheap (they never show up on
the profile, only PdhCollectQueryData does) so there is not much gain. However
it should allow slightly less code, which is a good thing. I'll try it.

Aside: the check for matching buffer sizes/counts was broken. It was intended to
continue if *either* value differed, not if both. Probably irrelevant, but
*oops*.

[ncarter (slow), 2015/06/29 20:03:19]

I'm glad you caught this. Thanks for pointing it out.

+        PDH_FMT_LARGE, &buffer_size2, &item_count2, nullptr) != PDH_MORE_DATA)
+      continue;
+
+    // It is not clear whether Pdh guarantees that the two counters in the same
+    // query will execute atomically - if they will see the same set of
+    // processes. If they do not then the correspondence between "ID Process"
+    // and "Working Set - Private" is lost and we have to discard these results.
+    // In testing these values have always matched. If this check fails then
+    // the old per-process memory calculations will be used instead.
+    if (buffer_size1 != buffer_size2 && item_count1 != item_count2)
+      continue;
+
+    // Allocate enough space for the results of both queries.
+    std::vector<char> buffer(buffer_size1 * 2);
+    // Retrieve the process ID data.
+    auto process_id_data = reinterpret_cast<PDH_FMT_COUNTERVALUE_ITEM*>
+        (&buffer[0]);

[ncarter (slow), 2015/06/26 21:46:16]

Apparently (I just learned this) C++11 has vector::data() as an alternative to
&vector[0], with the advantage that it's safe to call on an empty vector.

You could also use scoped_ptr<char[]> buffer or
scoped_ptr<PDH_FMT_COUNTERVALUE_ITEM[]> instead of a vector.

[brucedawson, 2015/06/27 00:13:05]

I started changing to .data() but then decided to leave it as [0] for symmetry
with [buffer_size1].

I also like the portability between C arrays and vector that &[] gives, although
that's not a great rationale.

I prefer vector over scoped_ptr<char[]> - more standard. And
scoped_ptr<PDH_FMT_COUNTERVALUE_ITEM[]> would not actually work (I believe)
because the size is actually greater than
item_count*sizeof(PDH_FMT_COUNTERVALUE_ITEM).

+    if (PdhGetFormattedCounterArray(counter_pair.process_id_handle,
+        PDH_FMT_LONG, &buffer_size1, &item_count1, process_id_data)
+        != ERROR_SUCCESS)
+      continue;
+    // Retrieve the private working set data.
+    auto private_ws_data = reinterpret_cast<PDH_FMT_COUNTERVALUE_ITEM*>
+        (&buffer[buffer_size1]);
+    if (PdhGetFormattedCounterArray(counter_pair.private_ws_handle,
+        PDH_FMT_LARGE, &buffer_size1, &item_count1, private_ws_data)
+        != ERROR_SUCCESS)
+      continue;
+
+    // Make room for the new set of records.
+    size_t start_offset = records_.size();
+    records_.resize(start_offset + item_count1);
+
+    for (DWORD i = 0; i < item_count1; ++i) {
+      records_[start_offset + i].process_id =
+          process_id_data[i].FmtValue.longValue;
+      // Integer overflow can happen here if a 32-bit process is monitoring a
+      // 64-bit process. Pdh always returns working set in a 64-bit LONGLONG so
+      // we set the result to the min of that and the maximum size_t value.
+      LONGLONG max_size_t = std::numeric_limits<size_t>::max();
+      records_[start_offset + i].private_ws = std::min(max_size_t,

[ncarter (slow), 2015/06/26 21:46:16]

Could this be a saturated_cast<size_t>(private_ws_data[i].FmtValue.largeValue)?

https://code.google.com/p/chromium/codesearch#chromium/src/base/numerics/safe...

[brucedawson, 2015/06/27 00:13:05]

Ooh -- much better.

+          private_ws_data[i].FmtValue.largeValue);
+    }
+  }
+
+  // The results will include all processes that match the passed in name,
+  // regardless of whether they are spawned by the calling process.
+  // The results must be sorted by process ID for efficient lookup.
+  std::sort(records_.begin(), records_.end());
+}
+
+// Ask for the working set for a specific process, from the most recent call
+// to Sample. If no data is available then zero will be returned. The result
+// is in bytes.

[ncarter (slow), 2015/06/26 21:46:16]

This comment also is a duplicate of the one in the .h. You only need it in the
.h.

[brucedawson, 2015/06/27 00:13:05]

Done.

+size_t PrivateWorkingSetSnapshot::GetPrivateWorkingSet(
+    base::ProcessId process_id) const
+{
+  // Do a binary search for the requested process ID and return the working set
+  // if found.
+  auto p = std::lower_bound(records_.begin(), records_.end(), process_id);
+  if (p != records_.end() && p->process_id == process_id)
+    return p->private_ws;
+
+  return 0;
+}
+
+}  // namespace base