Make task manager memory data more efficient and meaningful.

base/process/private_working_set_snapshot.h

+// Copyright (c) 2009 The Chromium Authors. All rights reserved.

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

2015, and drop the (c) [yeah, seriously :( ]

https://www.chromium.org/developers/coding-style#TOC-File-headers

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

Okay that's funny. Lesson learned about the perils of copying from existing
files. Fixed.

+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef PRIVATE_WORKING_SET_SNAPSHOT_H_

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

This should be:

BASE_PROCESS_PRIVATE_WORKING_SET_SNAPSHOT_

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

Done.

+#define PRIVATE_WORKING_SET_SNAPSHOT_H_
+
+#include <vector>
+
+#include "base/base_export.h"
+#include "base/process/process_handle.h"
+
+#if defined(OS_WIN)
+
+#include <pdh.h>
+
+namespace base {
+
+// This class can be used to do bulk collection of private working sets. This
+// exists because on Windows it is much faster to collect a group of private
+// working sets all at once using PdhOpenQuery than to calculate the private
+// working sets for each process individually.
+class BASE_EXPORT PrivateWorkingSetSnapshot {
+ public:
+  PrivateWorkingSetSnapshot();
+  ~PrivateWorkingSetSnapshot();
+  // Add a process name that this object should monitor, such as "chrome". All
+  // processes whose name starts with this string will be monitored.
+  void AddToMonitorList(const std::string& process_name);
+  // 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.
+  void Sample();

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

I'd add a blank line between each method here. I think that's normal to do when
you're declaring a set of public methods that form the caller's interface to the
class, and -- most importantly -- it seems to be the done thing in the nearby
process.h file.

[As far as I know it's not against the style guide to omit the vertical
whitespace -- and if anything, the style guide says to minimize use of blank
lines. But in Chromium I'm accustomed to seeing the blank lines omitted in cases
where a class is implementing more than one abstract base class, and there's a
desire to tightly group together all the overridden methods of each interface.]

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

Done.

+  // 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.
+  size_t GetPrivateWorkingSet(base::ProcessId process_id) const;
+
+ private:
+  // The handle to the query object.
+  PDH_HQUERY query_handle_ = nullptr;

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

We talked over VC about switching this to win::ScopedHandle (from
base/win/scoped_handle.h). That would call CloseHandle in the dtor. To make it
call PdhCloseQuery on destruction, you'd need to define a traits type that would
call PdhCloseQuery, following this example:
https://code.google.com/p/chromium/codesearch#chromium/src/base/win/shortcut....

The main benefit of ScopedHandle is as a defense against leaks and
double-closes, from the fact that it's a move-only type. It is nearly overkill
here, but I'd probably vote for it it anyway, on the grounds that a seeing a raw
handle type in a .h file tempts a lot of us (myself included) to spend a few
minutes scanning the code to prove that it's closed properly. However I'm open
to arguments to the contrary.

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

Done.

+
+  // This holds a pair of Pdh counters to queries for the process ID and private
+  // working set for a particular process name being monitored. The results from
+  // the two queries can be matched up so that we can associate a private
+  // working set with a process ID.
+  struct PdhCounterPair {

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

A strict reading of the style guide requires these struct definitions to be
before any data member declarations (With the typedefs & enums).
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_O...

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

Done.

+    // The handle to the 'counter' that retrieves process IDs.
+    PDH_HCOUNTER process_id_handle = nullptr;
+    // The handle to the 'counter' that retrieves private working sets.
+    PDH_HCOUNTER private_ws_handle = nullptr;

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

These handles don't need to be closed because their lifetimes are bound to
query_handle_, right? If so, I'd add a comment here explaining that.

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

Done.

+  };
+  std::vector<PdhCounterPair> counter_pairs_;
+
+  struct PidAndPrivateWS {

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

WS -> WorkingSet

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

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

Done.

Although, I also have a few private_ws member variables and locals. I could
change them as well but it's *so* verbose...

+    base::ProcessId process_id;
+    size_t private_ws;
+    // Comparison function for sorting.
+    bool operator<(const PidAndPrivateWS& other) const {
+      return process_id < other.process_id;
+    }
+    // Comparison function for searching.
+    bool operator<(const base::ProcessId other_process_id) const {
+      return process_id < other_process_id;
+    }
+  };
+  // After each call to Sample this will hold the results, sorted by process id.
+  std::vector<PidAndPrivateWS> records_;
+
+  DISALLOW_COPY_AND_ASSIGN(PrivateWorkingSetSnapshot);
+};
+
+}  // namespace base
+
+#endif  // defined(OS_WIN)
+
+#endif  // PRIVATE_WORKING_SET_SNAPSHOT_H_