Implement querying CPU time on Windows for systemInfo.cpu API

chrome/browser/extensions/api/system_info_cpu/cpu_info_provider_win.cc

Index: chrome/browser/extensions/api/system_info_cpu/cpu_info_provider_win.cc
diff --git a/chrome/browser/extensions/api/system_info_cpu/cpu_info_provider_win.cc b/chrome/browser/extensions/api/system_info_cpu/cpu_info_provider_win.cc
index 7a9e086cb03bbfcc4786c00908242fefc30f12ca..8b1d962122a3ef8db60873dbae296e39c2dcdb96 100644
--- a/chrome/browser/extensions/api/system_info_cpu/cpu_info_provider_win.cc
+++ b/chrome/browser/extensions/api/system_info_cpu/cpu_info_provider_win.cc
@@ -4,11 +4,83 @@
 
 #include "chrome/browser/extensions/api/system_info_cpu/cpu_info_provider.h"
 
+#include <windows.h>
+
+#include "base/sys_info.h"
+
 namespace extensions {
 
+namespace {
+
+// SYSTEM_INFORMATION_CLASS enum value for processor information.
+const DWORD kSystemProcessorPerformanceInformation = 0x8;

[alexeypa (please no reviews), 2012/09/11 16:33:16]

This information class is declared in winternl.h. See
SystemProcessorPerformanceInformation.

[Hongbo Min, 2012/09/12 12:31:37]

Done.

+
+// The SYSTEM_PROCESSOR_PERFORMANCE_INFORMATION data structure is originated
+// from MSDN.
+typedef struct _SYSTEM_PROCESSOR_PERFORMANCE_INFORMATION {

[alexeypa (please no reviews), 2012/09/11 16:33:16]

This structure is declared in <winternl.h>.

[Hongbo Min, 2012/09/12 12:31:37]

Done.

+  LARGE_INTEGER IdleTime;
+  LARGE_INTEGER KernelTime;
+  LARGE_INTEGER UserTime;
+  LARGE_INTEGER Reserved1[2];
+  ULONG Reserved2;
+} SYSTEM_PROCESSOR_PERFORMANCE_INFORMATION;
+
+// Undocumented NT API used for querying system information.

[alexeypa (please no reviews), 2012/09/11 16:33:16]

nit: It is partially documented. There us an entry in MSDN and the prototype in
winternl.h. I think you can refer to them instead of describing the function
parameters below.

[Hongbo Min, 2012/09/12 12:31:37]

Done.

+// |system_info_class|: The kind of the system information to be retrieved.
+// |system_info|: The pointer to a buffer that receives the requested info.
+// |system_info_length|: The size of the buffer pointed to by the |system_info|
+// parameter, in bytes.
+// |return_length|: An optional pointer to a location where the function
+// writes the actual size of the information requested.
+typedef DWORD (WINAPI *NtQuerySystemInformationPF)(DWORD system_info_class,
+                                                   PVOID system_info,
+                                                   ULONG system_info_length,
+                                                   PULONG return_length);
+}  // namespace
+
 bool CpuInfoProvider::QueryCpuTimePerProcessor(std::vector<CpuTime>* times) {
-  // TODO(hongbo): use NtQuerySystemInformation to query the cpu time.
-  return false;
+  if (!times) return false;

[alexeypa (please no reviews), 2012/09/11 16:33:16]

Move "return false" to the next line.

[Hongbo Min, 2012/09/12 12:31:37]

Done.

+
+  times->clear();

[alexeypa (please no reviews), 2012/09/11 16:33:16]

nit: you can avoid this by having a local std::vector<CpuTime> variable and
swapping it (std::vertor::swap()) with |*timer| at the very end of the function.
This way you will also avoid touching |*times| in case of an error.

[Hongbo Min, 2012/09/12 12:31:37]

Done.

+
+  HMODULE ntmodule = GetModuleHandle(L"ntdll.dll");

[alexeypa (please no reviews), 2012/09/11 16:33:16]

nit: consider declaring constants for L"ntdll.dll" and
"NtQuerySystemInformation" below.

[alexeypa (please no reviews), 2012/09/11 16:33:16]

nit: Add CHECK(ntmodule != NULL) here.

[Hongbo Min, 2012/09/12 12:31:37]

Done.

+  NtQuerySystemInformationPF NtQuerySystemInformation =
+      reinterpret_cast<NtQuerySystemInformationPF>(
+          ::GetProcAddress(ntmodule, "NtQuerySystemInformation"));
+
+  if (!NtQuerySystemInformation)

[alexeypa (please no reviews), 2012/09/11 16:33:16]

nit: I believe this can be CHECK(NtQuerySystemInformation)

[Hongbo Min, 2012/09/12 12:31:37]

Directly use NtQuerySystemInformation API as defined in winternl.h now.

+    return false;
+
+  int num_of_processors = base::SysInfo::NumberOfProcessors();

[alexeypa (please no reviews), 2012/09/11 16:33:16]

nit: Why is this number signed?

[Hongbo Min, 2012/09/12 12:31:37]

The base::SysInfo::NumberOfProcessors returns signed number.

[alexeypa (please no reviews), 2012/09/12 15:41:55]

I understand but why does it return a signed number? Is it feasible to change
this (in a follow up CL)? If not, consider adding DCHECK_GT(num_of_processors,
0);

+

[alexeypa (please no reviews), 2012/09/11 16:33:16]

nit: no need for an empty line here.

[Hongbo Min, 2012/09/12 12:31:37]

Done.

+  scoped_array<SYSTEM_PROCESSOR_PERFORMANCE_INFORMATION> processor_info(
+      new SYSTEM_PROCESSOR_PERFORMANCE_INFORMATION[num_of_processors]);
+
+  ULONG bytes, returned_bytes;

[alexeypa (please no reviews), 2012/09/11 16:33:16]

nit: Declare both |bytes| and |returned_bytes| on separate lines. Initialize and
declare them in the same statement.

[Hongbo Min, 2012/09/12 12:31:37]

Done.

+  bytes = sizeof(SYSTEM_PROCESSOR_PERFORMANCE_INFORMATION) * num_of_processors;
+  returned_bytes = 0;
+

[alexeypa (please no reviews), 2012/09/11 16:33:16]

nit: no need for an empty line here.

[Hongbo Min, 2012/09/12 12:31:37]

Done.

+  if (NtQuerySystemInformation(kSystemProcessorPerformanceInformation,
+                               processor_info.get(),
+                               bytes,
+                               &returned_bytes) != 0)
+    return false;
+
+  int returned_num_of_processors =
+      returned_bytes / sizeof(SYSTEM_PROCESSOR_PERFORMANCE_INFORMATION);
+
+  DCHECK(returned_num_of_processors == num_of_processors);

[alexeypa (please no reviews), 2012/09/11 16:33:16]

Does |returned_num_of_processors != num_of_processors| mean a catastrophic
failure? Can I happen if say a CPU is hot added or removed
(http://msdn.microsoft.com/en-us/library/bb964703(v=sql.105).aspx)? Will it be
better to retry once?

I'm pretty sure that at least we should return an error nicely here. I'm not
sure about retrying though.

[Hongbo Min, 2012/09/12 12:31:37]

Return false in case of that error happens.

+
+  for (int i = 0; i < returned_num_of_processors; ++i) {
+    CpuTime time;
+

[alexeypa (please no reviews), 2012/09/11 16:33:16]

nit: no need for an empty line.

[Hongbo Min, 2012/09/12 12:31:37]

Done.

+    time.kernel = processor_info[i].KernelTime.QuadPart;
+    time.user = processor_info[i].UserTime.QuadPart;
+    time.idle = processor_info[i].IdleTime.QuadPart;
+
+    times->push_back(time);
+  }
+  return true;
 }
 
 }  // namespace extensions