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..ee3fd05a05b4195e8b45fc3b4f18ff2c120d86cb
--- /dev/null
+++ b/base/process/private_working_set_snapshot_win.cc
@@ -0,0 +1,150 @@
+// Copyright 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/process/private_working_set_snapshot.h"
+
+#include <algorithm>
+
+#if defined(OS_WIN)
+#include <pdh.h>
+#include <pdhmsg.h>
+#include <windows.h>
+#endif

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

I'm fine For questions like this the best thing to do is to consult code search
for examples. I did a search for [file:_win.cc "include <windows"]
https://code.google.com/p/chromium/codesearch#search/&q=file:_win.cc%20%22inc...

And it looks like what many other _win.cc files do, is to put the the std:: and
win32 #includes in the same block, with no OS ifdef. It stands to reason that if
the presubmit doesn't know that windows.h is platform specific, it is treating
it the same as <algorithm>, etc.

https://code.google.com/p/chromium/codesearch#chromium/src/base/process/proce...

https://code.google.com/p/chromium/codesearch#chromium/src/sandbox/win/src/wi...

So try like this?

#include "base/process/private_working_set_snapshot.h"

#include <algorithm>
#include <pdh.h>
#include <pdhmsg.h>
#include <windows.h>

#include "base/numerics/safe_conversions.h"
#include "base/win/windows_version.h"


--- having said all that, I'm fine with the #if defined(OS_WIN) too, as you have
done.

[brucedawson, 2015/06/29 22:09:52]

I guess if I think of Windows header files as being C system header files then
it all makes sense. I wonder if that is intended?
Note that <pdh.h and <pdhmsg.h> are treated as C system headers so they have to
go in front of <algorithm> (C++ system header). <windows.h> has an exception and
can go literally anywhere! There is no consistent answer as to whether it should
go before other Windows/system header files or after. I'm punting on that
question by removing it (pdh.h includes what it needs) which is also a popular
solution.

Armed with all this knowledge I've also changed the includes in the header file
- on non-Windows compiles it will use a bare minimum of includes.

I'm working on a CL which will give more detail when the include order presubmit
check fails - the sort of information I wish it had given me. It will
distinguish between alphabetization errors and grouping errors - those are too
subtle right now.

Done.

+
+#include "base/numerics/safe_conversions.h"
+#include "base/win/windows_version.h"
+
+namespace base {
+
+PrivateWorkingSetSnapshot::PrivateWorkingSetSnapshot() {
+  // 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
+  PDH_HQUERY query_handle;
+  if (PdhOpenQuery(NULL, NULL, &query_handle) != ERROR_SUCCESS) {
+    return;
+  }
+
+  query_handle_.Set(query_handle);
+}
+
+void PrivateWorkingSetSnapshot::AddToMonitorList(
+    const std::string& process_name) {
+  if (!query_handle_.IsValid())
+    return;
+
+  // 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_.Get(), process_id_query.c_str(), NULL,
+                     &new_counters.process_id_handle) != ERROR_SUCCESS) {
+    return;
+  }
+
+  // If adding the second counter fails then we should remove the first one.
+  if (PdhAddCounterA(query_handle_.Get(), 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);
+}
+
+void PrivateWorkingSetSnapshot::Sample() {
+  if (counter_pairs_.empty())
+    return;
+
+  // Destroy all previous data.
+  records_.resize(0);
+  // Record the requested data into PDH's internal buffers.
+  if (PdhCollectQueryData(query_handle_.Get()) != 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;
+
+    // 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/29 20:03:19]

I'm fine leaving this as &buffer[0], but do we need to do something to defend
against buffer_size1 against being zero?  Lots of ways to solve that, but my
understanding was that buffer[0], if empty, would be undefined behavior in
theory, and could hit debug-mode STL checks in practice.

[brucedawson, 2015/06/29 22:09:52]

Good point. I meant to add a zero-size check to avoid this. I've added one now
(after the first call to PdhGetFormattedCounterArray).

+    DWORD buffer_size2 = buffer_size1;
+    DWORD item_count2 = item_count1;
+    if (PdhGetFormattedCounterArray(counter_pair.process_id_handle,
+                                    PDH_FMT_LONG, &buffer_size2, &item_count2,
+                                    process_id_data) != ERROR_SUCCESS)
+      continue;
+    // Make sure we retrieved exactly the amount of data we expected.
+    if (buffer_size2 != buffer_size1 || item_count2 != item_count1)
+      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_size2, &item_count2,
+                                    private_ws_data) != ERROR_SUCCESS)
+      continue;
+    // Make sure the working-set data size/count matches the process id
+    // size/count - otherwise matching them up won't make sense. They do seem to
+    // always match.
+    if (buffer_size2 != buffer_size1 || item_count2 != item_count1)
+      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 so we do a saturated_cast.
+      records_[start_offset + i].private_ws =
+          saturated_cast<size_t>(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());
+}
+
+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