Task manager should support Idle Wakeups on Windows

chrome/browser/task_management/sampling/shared_sampler_win.cc

+// 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_management/sampling/shared_sampler.h"
+
+#include <windows.h>
+#include <winternl.h>
+
+#include <algorithm>
+
+#include "base/bind.h"
+#include "base/command_line.h"
+#include "base/time/time.h"
+#include "chrome/browser/task_management/task_manager_observer.h"
+#include "content/public/browser/browser_thread.h"
+
+namespace task_management {
+
+namespace {
+
+const wchar_t kNacl64Exe[] = L"nacl64.exe";
+
+// From <wdm.h>
+typedef LONG KPRIORITY;
+typedef LONG KWAIT_REASON;  // Full definition is in wdm.h
+
+// From ntddk.h
+typedef struct _VM_COUNTERS {
+  SIZE_T PeakVirtualSize;
+  SIZE_T VirtualSize;
+  ULONG PageFaultCount;
+  // Padding here in 64-bit
+  SIZE_T PeakWorkingSetSize;
+  SIZE_T WorkingSetSize;
+  SIZE_T QuotaPeakPagedPoolUsage;
+  SIZE_T QuotaPagedPoolUsage;
+  SIZE_T QuotaPeakNonPagedPoolUsage;
+  SIZE_T QuotaNonPagedPoolUsage;
+  SIZE_T PagefileUsage;
+  SIZE_T PeakPagefileUsage;
+} VM_COUNTERS;
+
+// Two possibilities available from here:
+// http://stackoverflow.com/questions/28858849/where-is-system-information-class-defined
+
+typedef enum _SYSTEM_INFORMATION_CLASS {
+  SystemProcessInformation = 5,  // This is the number that we need.
+} SYSTEM_INFORMATION_CLASS;
+
+// https://msdn.microsoft.com/en-us/library/gg750647.aspx?f=255&MSPPError=-2147217396
+typedef struct {
+  HANDLE UniqueProcess;  // Actually process ID
+  HANDLE UniqueThread;   // Actually thread ID
+} CLIENT_ID;
+
+// From http://alax.info/blog/1182, with corrections and modifications
+// Originally from
+// http://undocumented.ntinternals.net/index.html?page=UserMode%2FUndocumented%20Functions%2FSystem%20Information%2FStructures%2FSYSTEM_THREAD.html
+struct SYSTEM_THREAD_INFORMATION {
+  ULONGLONG KernelTime;
+  ULONGLONG UserTime;
+  ULONGLONG CreateTime;
+  ULONG WaitTime;
+  // Padding here in 64-bit
+  PVOID StartAddress;
+  CLIENT_ID ClientId;
+  KPRIORITY Priority;
+  LONG BasePriority;
+  ULONG ContextSwitchCount;
+  ULONG State;
+  KWAIT_REASON WaitReason;
+};
+#if _M_X64
+static_assert(sizeof(SYSTEM_THREAD_INFORMATION) == 80,
+              "Structure size mismatch");
+#else
+static_assert(sizeof(SYSTEM_THREAD_INFORMATION) == 64,
+              "Structure size mismatch");
+#endif
+
+// From http://alax.info/blog/1182, with corrections and modifications
+// Originally from
+// http://undocumented.ntinternals.net/index.html?page=UserMode%2FUndocumented%20Functions%2FSystem%20Information%2FStructures%2FSYSTEM_THREAD.html
+struct SYSTEM_PROCESS_INFORMATION {

[ncarter (slow), 2016/07/28 21:37:26]

This is also declared (slightly differently) in:

  third_party/crashpad/crashpad/util/win/process_structs.h

Which leads me to two questions: (1) can we reuse the crashpad def? and (2) is
one more correct than the other?

[stanisc, 2016/07/28 22:38:44]

I saw the process_structs.h version and slight differences in the struct
definitions, especially around HANDLE used here for process/thread IDs vs.
padded DWORD used in the crashpad version. I wasn't sure if this code could
depend on third_party/crashpad and ended up just copying this from Bruce's
prototype where it was proven to work.
Should process_structs.h be moved into some common location in base?

+  ULONG NextEntryOffset;
+  ULONG NumberOfThreads;
+  // http://processhacker.sourceforge.net/doc/struct___s_y_s_t_e_m___p_r_o_c_e_s_s___i_n_f_o_r_m_a_t_i_o_n.html
+  ULONGLONG WorkingSetPrivateSize;
+  ULONG HardFaultCount;
+  ULONG Reserved1;
+  ULONGLONG CycleTime;
+  ULONGLONG CreateTime;
+  ULONGLONG UserTime;
+  ULONGLONG KernelTime;
+  UNICODE_STRING ImageName;
+  KPRIORITY BasePriority;
+  HANDLE ProcessId;
+  HANDLE ParentProcessId;
+  ULONG HandleCount;
+  ULONG Reserved2[2];
+  // Padding here in 64-bit
+  VM_COUNTERS VirtualMemoryCounters;
+  size_t Reserved3;
+  IO_COUNTERS IoCounters;
+  SYSTEM_THREAD_INFORMATION Threads[1];
+};
+#if _M_X64
+static_assert(sizeof(SYSTEM_PROCESS_INFORMATION) == 336,
+              "Structure size mismatch");
+#else
+static_assert(sizeof(SYSTEM_PROCESS_INFORMATION) == 248,
+              "Structure size mismatch");
+#endif
+
+// ntstatus.h conflicts with windows.h so define this locally.
+#define STATUS_SUCCESS              ((NTSTATUS)0x00000000L)
+#define STATUS_BUFFER_TOO_SMALL     ((NTSTATUS)0xC0000023L)
+#define STATUS_INFO_LENGTH_MISMATCH ((NTSTATUS)0xC0000004L)
+
+// Wrapper for NtQuerySystemProcessInformation with buffer reallocation logic.
+bool QuerySystemProcessInformation(std::vector<BYTE>* data_buffer,
+                                   ULONG* data_size) {
+  typedef NTSTATUS(WINAPI * NTQUERYSYSTEMINFORMATION)(
+      SYSTEM_INFORMATION_CLASS SystemInformationClass, PVOID SystemInformation,
+      ULONG SystemInformationLength, PULONG ReturnLength);
+
+  HMODULE ntdll = ::GetModuleHandle(L"ntdll.dll");
+  if (!ntdll) {
+    NOTREACHED();
+    return false;
+  }
+
+  NTQUERYSYSTEMINFORMATION nt_query_system_information_ptr =
+      reinterpret_cast<NTQUERYSYSTEMINFORMATION>(
+          ::GetProcAddress(ntdll, "NtQuerySystemInformation"));
+  if (!nt_query_system_information_ptr) {
+    NOTREACHED();
+    return false;
+  }
+
+  ULONG buffer_size = data_buffer->size();
+
+  NTSTATUS result;
+
+  // There is a potential race condition between growing the buffer and new
+  // processes being creating. Try a few times before giving up.
+  for (int i = 0; i < 10; i++) {
+    *data_size = 0;
+    result = nt_query_system_information_ptr(
+        SystemProcessInformation,
+        buffer_size > 0 ? data_buffer->data() : nullptr,
+        buffer_size, data_size);
+
+    if (result == STATUS_INFO_LENGTH_MISMATCH ||
+        result == STATUS_BUFFER_TOO_SMALL) {
+      // Insufficient buffer. Resize to the returned |data_size| plus 10% extra
+      // to avoid frequent reallocations and try again.
+      DCHECK_GT(*data_size, buffer_size);
+      buffer_size = static_cast<ULONG>(*data_size * 1.1);
+      data_buffer->resize(buffer_size);
+    } else {
+      // Either STATUS_SUCCESS or an error other than the two above.
+      break;
+    }
+  }
+
+  return result == STATUS_SUCCESS;
+}
+
+}  // namespace
+
+// Per-thread data extracted from SYSTEM_THREAD_INFORMATION
+// and stored in a snapshot.
+// This structure is accessed only on the worker thread.
+struct ThreadData {
+  base::PlatformThreadId thread_id;
+  ULONG context_switches;
+};
+
+// Per-process data extracted from SYSTEM_PROCESS_INFORMATION
+// and stored in a snapshot.
+// This structure is accessed only on the worker thread.
+struct ProcessData {
+  ProcessData() = default;
+  ProcessData(ProcessData&&) = default;
+
+  std::vector<ThreadData> threads;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(ProcessData);
+};
+
+typedef std::map<base::ProcessId, ProcessData> ProcessDataMap;
+
+// ProcessDataSnapshot gets created and accessed only on the worker thread.
+// This is used to calculate metrics like Idle Wakeups / sec that require
+// a delta between two snapshots.
+struct ProcessDataSnapshot {
+  ProcessDataMap processes;
+  base::TimeTicks timestamp;
+};
+
+ULONG CountContextSwitches(const ProcessData& process_data) {
+  ULONG context_switches = 0;
+  for (auto& thread_data : process_data.threads) {
+    context_switches += thread_data.context_switches;
+  }
+
+  return context_switches;
+}
+
+ULONG CountContextSwitchesDelta(const ProcessData& prev_process_data,
+                                const ProcessData& new_process_data) {
+  ULONG delta = 0;
+  // This one pass algorithm relies on the threads vectors to be
+  // ordered by thread_id.
+  size_t prev_index = 0;
+  size_t new_index = 0;
+  while (new_index < new_process_data.threads.size()) {
+    auto& new_thread = new_process_data.threads[new_index];
+
+    if (prev_index < prev_process_data.threads.size()) {
+      auto& prev_thread = prev_process_data.threads[prev_index];
+
+      if (prev_thread.thread_id < new_thread.thread_id) {
+        // Thread exists in previous snapshot only - skip it and don't count
+        // its context switches towards the delta.
+        prev_index++;
+      } else if (prev_thread.thread_id > new_thread.thread_id) {
+        // A new thread that didn't exist in prev_process_data.
+        // Add its entire number of context switches to the delta (since it
+        // started with zero).
+        delta = new_thread.context_switches;
+        new_index++;
+      } else {
+        // Threads match between two snapshots - add the difference between
+        // context switches to the delta.
+        delta += new_thread.context_switches - prev_thread.context_switches;
+        prev_index++;
+        new_index++;
+      }
+    } else {
+      // prev_index reached the end of |prev_process_data.threads| size, so this
+      // is a new thread that didn't exist in prev_process_data.
+      // Add its entire number of context switches to the delta (since it
+      // started with zero).
+      delta = new_thread.context_switches;
+      new_index++;
+    }
+  }
+
+  return delta;
+}
+
+SharedSampler::SharedSampler(
+    const scoped_refptr<base::SequencedTaskRunner>& blocking_pool_runner)
+    : previous_buffer_size_(0),
+      current_process_image_name_(
+          base::CommandLine::ForCurrentProcess()->GetProgram().BaseName()),
+      blocking_pool_runner_(blocking_pool_runner) {
+  DCHECK(blocking_pool_runner.get());
+
+  // This object will be created on the UI thread, however the sequenced checker
+  // will be used to assert we're running the expensive operations on one of the
+  // blocking pool threads.
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  worker_pool_sequenced_checker_.DetachFromSequence();
+}
+
+SharedSampler::~SharedSampler() {}
+
+int64_t SharedSampler::SupportsFlags() const {
+  return REFRESH_TYPE_IDLE_WAKEUPS;
+}
+
+SharedSampler::Callbacks::Callbacks() {}
+
+SharedSampler::Callbacks::~Callbacks() {}
+
+SharedSampler::Callbacks::Callbacks(Callbacks&& other) {
+  on_idle_wakeups = std::move(other.on_idle_wakeups);
+}
+
+void SharedSampler::RegisterCallbacks(
+    base::ProcessId process_id,
+    const OnIdleWakeupsCallback& on_idle_wakeups) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  Callbacks callbacks;
+  callbacks.on_idle_wakeups = on_idle_wakeups;
+  bool result = callbacks_map_.insert(
+      std::make_pair(process_id, std::move(callbacks))).second;
+  DCHECK(result);
+}
+
+void SharedSampler::UnregisterCallbacks(base::ProcessId process_id) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  callbacks_map_.erase(process_id);
+}
+
+void SharedSampler::Refresh(base::ProcessId process_id, int64_t refresh_flags) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  DCHECK(callbacks_map_.find(process_id) != callbacks_map_.end());
+  DCHECK_NE(0, refresh_flags & SupportsFlags());
+
+  bool need_refresh = refresh_flags_.empty();
+
+  refresh_flags_.push_back(std::make_pair(process_id, refresh_flags));
+
+  if (need_refresh) {
+    base::PostTaskAndReplyWithResult(
+        blocking_pool_runner_.get(), FROM_HERE,
+        base::Bind(&SharedSampler::RefreshOnWorkerThread, this),
+        base::Bind(&SharedSampler::OnRefreshDone, this));
+  }
+}
+
+std::unique_ptr<SharedSampler::RefreshResultMap>
+SharedSampler::RefreshOnWorkerThread() {
+  DCHECK(worker_pool_sequenced_checker_.CalledOnValidSequencedThread());
+
+  std::unique_ptr<ProcessDataSnapshot> snapshot = CaptureSnapshot();
+  DCHECK(snapshot);
+
+  std::unique_ptr<RefreshResultMap> results(new RefreshResultMap);
+
+  if (previous_snapshot_) {
+    MakeResultsFromTwoSnapshots(*previous_snapshot_, *snapshot, results.get());
+  } else {
+    MakeResultsFromSnapshot(*snapshot, results.get());

[ncarter (slow), 2016/07/28 21:37:26]

Is MakeResultsFromSnapshot different from just doing
MakeResultsFromTwoSnapshots, where |previous_snapshot_| is empty?

[stanisc, 2016/07/28 22:38:44]

It is different. MakeResultsFromTwoSnapshots would report the current absolute
number of context switches as CS deltas for each process. So we'd end up showing
a huge CS rate.
MakeResultsFromSnapshot reports zeros for each process.

+  }
+
+  previous_snapshot_ = std::move(snapshot);

[ncarter (slow), 2016/07/28 21:37:26]

If we close the TaskManager, or hide the idle wakeups column, it seems like the
SharedSampler remains, and its retained |previous_snapshot_| will get
arbitrarily old. Does this suggest that if we later re-show the column, we'd get
an initial spike in the values, proportional to the time that the column had
been invisible?

[stanisc, 2016/07/28 22:38:44]

Hmm... Yes, that is something I didn't consider. It would divide CS increments
by the time delta between snapshots. So the values would be OK if not on the low
side because it wouldn't discount any context switches for stopped threads. For
new processes the number of CS switches averaged over time would be on the low
side too.

So we could keep the implementation as it. It would show low values for one
second but that isn't much worse than just showing zeros.
If we'd prefer to show zeros for the first second than we'd need to either (1)
delete the old snapshot or (2) not use the old snapshot if the difference
between timestamps is greater than some predetermined threshold interval. 

[stanisc, 2016/08/01 22:34:26]

OK, I've handled the "closing the TaskManager" case by clearing the cached
snapshot when all callbacks are unregistered.
I haven't addressed the case when Idle Wakeups column is removed and then added
back. On the surface the numbers look fine in the second case because, as I
explained above, the actual time delta between the two snapshots is used so the
larger CS increment is divided by a larger time delta. If anything this first
second Idle Wakeups value would be on the lower side because it would count
number of context switches from any threads that terminated during this interval
while the column wasn't visible.

+
+  return results;
+}
+
+bool SharedSampler::IsChromeImageName(const wchar_t* image_name) const {

[ncarter (slow), 2016/07/28 21:37:26]

Using base:: string comparison function would result in code
that's more readable to non-Windows programmers. Maybe like this:

bool SharedSampler::IsChromeImageName(FilePath::StringPieceType image_name) {
  return FilePath::CompareEqualIgnoreCase(image_name,
                                          current_process_image_name_.value())
||
         FilePath::CompareEqualIgnoreCase(image_name, kNacl64Exe);
}

[stanisc, 2016/08/01 22:34:27]

Done.

+  return _wcsnicmp(image_name, current_process_image_name_.value().c_str(),
+                   current_process_image_name_.value().size()) == 0 ||
+         _wcsnicmp(image_name, kNacl64Exe, _countof(kNacl64Exe) - 1) == 0;
+}
+
+std::unique_ptr<ProcessDataSnapshot> SharedSampler::CaptureSnapshot() {
+  DCHECK(worker_pool_sequenced_checker_.CalledOnValidSequencedThread());
+
+  ULONG data_size;
+
+  // Preallocate the buffer with the size determined on the previous call to
+  // QuerySystemProcessInformation. This should be sufficient most of the time.
+  // QuerySystemProcessInformation will grow the buffer if necessary.
+  std::vector<BYTE> data_buffer(previous_buffer_size_);
+
+  if (!QuerySystemProcessInformation(&data_buffer, &data_size))
+    return std::unique_ptr<ProcessDataSnapshot>();

[ncarter (slow), 2016/07/28 21:37:26]

Would it be better to return an empty snapshot here, rather than a null pointer?
That way, the consumer doesn't need to handle two different kinds of nothing.

[stanisc, 2016/07/28 22:38:44]

I realized the current implementation doesn't actually handle a null snapshot.
That is something I'd overlooked.
I'll think about this.

[stanisc, 2016/08/01 22:34:26]

I decided to keep this approach to indicate that something went wrong. 

+
+  previous_buffer_size_ = data_buffer.size();
+
+  std::unique_ptr<ProcessDataSnapshot> snapshot(new ProcessDataSnapshot);
+  snapshot->timestamp = base::TimeTicks::Now();
+
+  for (ULONG offset = 0; offset < data_size; ) {
+    auto pi = reinterpret_cast<const SYSTEM_PROCESS_INFORMATION*>(
+        &data_buffer[offset]);
+
+    // Validate that the offset is valid and all needed data is within
+    // the buffer boundary.
+    if (offset + sizeof(SYSTEM_PROCESS_INFORMATION) > data_size)
+      break;
+    if (offset + sizeof(SYSTEM_PROCESS_INFORMATION) +
+            (pi->NumberOfThreads - 1) * sizeof(SYSTEM_THREAD_INFORMATION) >
+        data_size)
+      break;
+
+    if (pi->ImageName.Buffer) {
+      // Validate that the image name is within the buffer boundary.
+      // ImageName.Length seems to be in bytes rather than characters.
+      ULONG image_name_offset =
+          reinterpret_cast<BYTE*>(pi->ImageName.Buffer) - data_buffer.data();
+      if (image_name_offset + pi->ImageName.Length > data_size)
+        break;
+
+      // Check if this is a chrome process. Ignore all other processes.
+      if (IsChromeImageName(pi->ImageName.Buffer)) {
+        // Collect enough data to be able to do a diff between two snapshots.
+        // Some threads might stop or new threads might be created between two
+        // snapshots. If a thread with a large number of context switches gets
+        // terminated the total number of context switches for the process might
+        // go down and the delta would be negative.
+        // To avoid that we need to compare thread IDs between two snapshots and
+        // not count context switches for threads that are missing in the most
+        // recent snapshot.
+        ProcessData process_data;
+
+        // Iterate over threads and store each thread's ID and number of context
+        // switches.
+        for (ULONG thread_index = 0; thread_index < pi->NumberOfThreads;
+             ++thread_index) {
+          const SYSTEM_THREAD_INFORMATION* ti = &pi->Threads[thread_index];
+          if (ti->ClientId.UniqueProcess != pi->ProcessId)
+            continue;
+
+          ThreadData thread_data;
+          thread_data.thread_id = static_cast<base::PlatformThreadId>(
+              reinterpret_cast<uintptr_t>(ti->ClientId.UniqueThread));
+          thread_data.context_switches = ti->ContextSwitchCount;
+          process_data.threads.push_back(thread_data);
+        }
+
+        // Order thread data by thread ID to help diff two snapshots.
+        std::sort(process_data.threads.begin(), process_data.threads.end(),
+            [](const ThreadData& l, const ThreadData r) {
+              return l.thread_id < r.thread_id;
+            });
+
+        base::ProcessId process_id = static_cast<base::ProcessId>(
+            reinterpret_cast<uintptr_t>(pi->ProcessId));
+        bool inserted = snapshot->processes.insert(
+            std::make_pair(process_id, std::move(process_data))).second;
+        DCHECK(inserted);
+      }
+    }
+
+    // Check for end of the list.
+    if (!pi->NextEntryOffset)
+      break;
+
+    // Jump to the next entry.
+    offset += pi->NextEntryOffset;
+  }
+
+  return snapshot;
+}
+
+void SharedSampler::MakeResultsFromTwoSnapshots(
+    const ProcessDataSnapshot& prev_snapshot,
+    const ProcessDataSnapshot& snapshot,
+    RefreshResultMap* results) {
+  // Time delta in seconds.
+  double time_delta = (snapshot.timestamp - prev_snapshot.timestamp)
+        .InMillisecondsF() / 1000;
+
+  // Iterate over processes in both snapshots in parallel. This algorithm relies
+  // on map entries being ordered by Process ID.
+  ProcessDataMap::const_iterator prev_iter = prev_snapshot.processes.begin();
+  ProcessDataMap::const_iterator iter = snapshot.processes.begin();
+  while (iter != snapshot.processes.end()) {
+    auto process_id = iter->first;

[ncarter (slow), 2016/07/28 21:37:27]

I would use base::ProcessId instead of auto for the process_id variables.

[stanisc, 2016/08/01 22:34:27]

Done.

+
+    // Delta between the old snapshot and the new snapshot.
+    int idle_wakeups_delta;
+
+    if (prev_iter != prev_snapshot.processes.end()) {
+      auto prev_snapshot_process_id = prev_iter->first;
+      if (prev_snapshot_process_id < process_id) {

[ncarter (slow), 2016/07/28 21:37:26]

The two-iterator approach would be natural if we needed to do a merge, but since
we're skipping values that are in |prev_snapshot| but not |snapshot|, it seems
we don't actually need a symmetric loop structure. So another option here, that
might be clearer to understand, is to use an outer loop over |snapshot| paired
with a helper function to advance the |prev_iter|, like this:

// Helper function. Advances the given sorted range past |process_id|, and
returns the
// value that if there 
ProcessData* SeekInPreviousSnapshot(ProcessId process_id,
                                    ProcessDataMap::iterator* it_to_advance,
                                    const ProcessDataMap::iterator& range_end) {
  for (; *it_to_advance != range_end && (*it_to_advance)->first <= process_id;
       ++(*it_to_advance)) {
    if ((*it_to_advance)->first == process_id) {
      return &(((*it_to_advance)++)->second);
    }
  }
  return nullptr;
}

// ...
  // Loop structure.
  old_value_it = prev_snapshot.processes.begin();
  for (auto& current_entry : snapshot.processes) {
    base::ProcessId process_id = current_entry.first;
    const ProcessData& current_value = current_entry.second;
    ProcessData* old_value = SeekInPreviousSnapshot(
        process_id, &old_value_it, prev_snapshot.processes.end()) {
      if (old_value) {
        // Take delta |current_value| - |*old_value|
      } else {
        // Take |current_value|
      }
    }
  }

Maybe there's a fancy <algorithm> way of expressing that helper function?
std::lower_bound is the operation we want, except we don't want a binary search.

[stanisc, 2016/07/28 22:38:44]

Good idea! I'll think about this.

[stanisc, 2016/08/01 22:34:27]

Done.

+        // Process is missing in the last snapshot - skip it and continue.
+        prev_iter++;
+        continue;
+      } else if (prev_snapshot_process_id > process_id) {
+        // Process is missing in the previous snapshot.
+        // Use its entire number of context switches.
+        idle_wakeups_delta = CountContextSwitches(iter->second);
+        iter++;
+      } else {
+        // Processes match between two snapshots.
+        idle_wakeups_delta =
+            CountContextSwitchesDelta(prev_iter->second, iter->second);
+        prev_iter++;
+        iter++;
+      }
+    } else {
+      // Since prev_index is at the end of |prev_snapshot.processes|, this
+      // is a new process that is missing in the previous snapshot.
+      // Use its entire number of context switches.
+      idle_wakeups_delta = CountContextSwitches(iter->second);
+      iter++;
+    }
+
+    RefreshResult result;
+    result.idle_wakeups_per_second =
+        static_cast<int>(round(idle_wakeups_delta / time_delta));
+    bool inserted = results->insert(std::make_pair(process_id, result)).second;
+    DCHECK(inserted);
+  }
+}
+
+void SharedSampler::MakeResultsFromSnapshot(const ProcessDataSnapshot& snapshot,
+                                            RefreshResultMap* results) {
+  for (auto& pair : snapshot.processes) {
+    auto process_id = pair.first;
+    RefreshResult result;
+    // Use 0 for Idle Wakeups / sec in this case. This is consistent with
+    // ProcessMetrics::CalculateIdleWakeupsPerSecond implementation.
+    result.idle_wakeups_per_second = 0;
+    bool inserted = results->insert(std::make_pair(process_id, result)).second;
+    DCHECK(inserted);
+  }
+}
+
+void SharedSampler::OnRefreshDone(
+    std::unique_ptr<RefreshResultMap> refresh_results) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+
+  for (auto& process_flags : refresh_flags_) {

[ncarter (slow), 2016/07/28 21:37:27]

const auto&, unless you intend to mutuate.

[stanisc, 2016/08/01 22:34:27]

Done.

+    RefreshResultMap::iterator result_iter = refresh_results->find(
+        process_flags.first);
+    // TODO(stanisc): what to do if the result for this process is missing?
+    // Apparently when a new tab is created Refresh is called a few times with
+    // thread_id = 0 before the actual thread ID gets assigned.

[ncarter (slow), 2016/07/28 21:37:26]

The question is whether or not to invoke the callback if no data is available?
For other columns, e.g. memory, we flash N/A for the first second (wez has a CL
out to change N/A to a dash, which will be less distracting. I think using a
sentinel value like -1 makes sense here, so that we invoke the callbacks for
each request.

Having said that, I'd defer to Ahmed's judgement here, if he disagrees.

[stanisc, 2016/08/01 22:34:27]

OK. I ended up rewriting this whole method so the iteration goes primarily over
the registered callbacks and that guarantees every callback to be invoked even
if there is no result. This should also handle the case when
NtQuerySystemInformation failed for whatever reason so there are no results at
all. And yes, the sentinel value -1 is used in that case.

+    // Should callback still be called with a default value in that case?
+    if (result_iter == refresh_results->end())
+      continue;
+
+    auto& result = result_iter->second;

[ncarter (slow), 2016/07/28 21:37:27]

This declaration seems far from its first use.

[stanisc, 2016/08/01 22:34:27]

Done.

+
+    CallbacksMap::iterator callback_iter = callbacks_map_.find(
+        process_flags.first);
+    if (callback_iter == callbacks_map_.end())
+      continue;
+
+    if (TaskManagerObserver::IsResourceRefreshEnabled(REFRESH_TYPE_IDLE_WAKEUPS,
+                                                      process_flags.second)) {
+      callback_iter->second.on_idle_wakeups.Run(result.idle_wakeups_per_second);
+    }
+  }
+
+  // Clear refresh_flags_.
+  refresh_flags_.clear();
+}
+
+}  // namespace task_management