Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10916197:
Implement querying CPU time on Windows for systemInfo.cpu API (Closed)
|
Created:
8 years, 3 months ago by Hongbo Min Modified:
8 years, 3 months ago CC:
chromium-reviews, Aaron Boodman, mihaip-chromium-reviews_chromium.org Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionImplement querying CPU time for each processor on Windows for systemInfo.cpu API.
BUG=136519
TEST=None
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=156748
Patch Set 1 : #
Total comments: 28
Patch Set 2 : updated patch #
Total comments: 15
Patch Set 3 : updated patch #
Total comments: 7
Patch Set 4 : updated patch #
Messages
Total messages: 9 (0 generated)
Pls have a review.
http://codereview.chromium.org/10916197/diff/3001/chrome/browser/extensions/a... File chrome/browser/extensions/api/system_info_cpu/cpu_info_provider_win.cc (right): http://codereview.chromium.org/10916197/diff/3001/chrome/browser/extensions/a... chrome/browser/extensions/api/system_info_cpu/cpu_info_provider_win.cc:16: const DWORD kSystemProcessorPerformanceInformation = 0x8; This information class is declared in winternl.h. See SystemProcessorPerformanceInformation. http://codereview.chromium.org/10916197/diff/3001/chrome/browser/extensions/a... chrome/browser/extensions/api/system_info_cpu/cpu_info_provider_win.cc:20: typedef struct _SYSTEM_PROCESSOR_PERFORMANCE_INFORMATION { This structure is declared in <winternl.h>. http://codereview.chromium.org/10916197/diff/3001/chrome/browser/extensions/a... chrome/browser/extensions/api/system_info_cpu/cpu_info_provider_win.cc:28: // Undocumented NT API used for querying system information. 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. http://codereview.chromium.org/10916197/diff/3001/chrome/browser/extensions/a... chrome/browser/extensions/api/system_info_cpu/cpu_info_provider_win.cc:42: if (!times) return false; Move "return false" to the next line. http://codereview.chromium.org/10916197/diff/3001/chrome/browser/extensions/a... chrome/browser/extensions/api/system_info_cpu/cpu_info_provider_win.cc:44: times->clear(); 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. http://codereview.chromium.org/10916197/diff/3001/chrome/browser/extensions/a... chrome/browser/extensions/api/system_info_cpu/cpu_info_provider_win.cc:46: HMODULE ntmodule = GetModuleHandle(L"ntdll.dll"); nit: consider declaring constants for L"ntdll.dll" and "NtQuerySystemInformation" below. http://codereview.chromium.org/10916197/diff/3001/chrome/browser/extensions/a... chrome/browser/extensions/api/system_info_cpu/cpu_info_provider_win.cc:46: HMODULE ntmodule = GetModuleHandle(L"ntdll.dll"); nit: Add CHECK(ntmodule != NULL) here. http://codereview.chromium.org/10916197/diff/3001/chrome/browser/extensions/a... chrome/browser/extensions/api/system_info_cpu/cpu_info_provider_win.cc:51: if (!NtQuerySystemInformation) nit: I believe this can be CHECK(NtQuerySystemInformation) http://codereview.chromium.org/10916197/diff/3001/chrome/browser/extensions/a... chrome/browser/extensions/api/system_info_cpu/cpu_info_provider_win.cc:54: int num_of_processors = base::SysInfo::NumberOfProcessors(); nit: Why is this number signed? http://codereview.chromium.org/10916197/diff/3001/chrome/browser/extensions/a... chrome/browser/extensions/api/system_info_cpu/cpu_info_provider_win.cc:55: nit: no need for an empty line here. http://codereview.chromium.org/10916197/diff/3001/chrome/browser/extensions/a... chrome/browser/extensions/api/system_info_cpu/cpu_info_provider_win.cc:59: ULONG bytes, returned_bytes; nit: Declare both |bytes| and |returned_bytes| on separate lines. Initialize and declare them in the same statement. http://codereview.chromium.org/10916197/diff/3001/chrome/browser/extensions/a... chrome/browser/extensions/api/system_info_cpu/cpu_info_provider_win.cc:62: nit: no need for an empty line here. http://codereview.chromium.org/10916197/diff/3001/chrome/browser/extensions/a... chrome/browser/extensions/api/system_info_cpu/cpu_info_provider_win.cc:72: DCHECK(returned_num_of_processors == num_of_processors); 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. http://codereview.chromium.org/10916197/diff/3001/chrome/browser/extensions/a... chrome/browser/extensions/api/system_info_cpu/cpu_info_provider_win.cc:76: nit: no need for an empty line.
alex, thanks for your hints which makes the code more cleaner now. http://codereview.chromium.org/10916197/diff/3001/chrome/browser/extensions/a... File chrome/browser/extensions/api/system_info_cpu/cpu_info_provider_win.cc (right): http://codereview.chromium.org/10916197/diff/3001/chrome/browser/extensions/a... chrome/browser/extensions/api/system_info_cpu/cpu_info_provider_win.cc:16: const DWORD kSystemProcessorPerformanceInformation = 0x8; On 2012/09/11 16:33:16, alexeypa wrote: > This information class is declared in winternl.h. See > SystemProcessorPerformanceInformation. Done. http://codereview.chromium.org/10916197/diff/3001/chrome/browser/extensions/a... chrome/browser/extensions/api/system_info_cpu/cpu_info_provider_win.cc:20: typedef struct _SYSTEM_PROCESSOR_PERFORMANCE_INFORMATION { On 2012/09/11 16:33:16, alexeypa wrote: > This structure is declared in <winternl.h>. Done. http://codereview.chromium.org/10916197/diff/3001/chrome/browser/extensions/a... chrome/browser/extensions/api/system_info_cpu/cpu_info_provider_win.cc:28: // Undocumented NT API used for querying system information. On 2012/09/11 16:33:16, alexeypa wrote: > 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. Done. http://codereview.chromium.org/10916197/diff/3001/chrome/browser/extensions/a... chrome/browser/extensions/api/system_info_cpu/cpu_info_provider_win.cc:42: if (!times) return false; On 2012/09/11 16:33:16, alexeypa wrote: > Move "return false" to the next line. Done. http://codereview.chromium.org/10916197/diff/3001/chrome/browser/extensions/a... chrome/browser/extensions/api/system_info_cpu/cpu_info_provider_win.cc:44: times->clear(); On 2012/09/11 16:33:16, alexeypa wrote: > 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. Done. http://codereview.chromium.org/10916197/diff/3001/chrome/browser/extensions/a... chrome/browser/extensions/api/system_info_cpu/cpu_info_provider_win.cc:46: HMODULE ntmodule = GetModuleHandle(L"ntdll.dll"); On 2012/09/11 16:33:16, alexeypa wrote: > nit: consider declaring constants for L"ntdll.dll" and > "NtQuerySystemInformation" below. Done. http://codereview.chromium.org/10916197/diff/3001/chrome/browser/extensions/a... chrome/browser/extensions/api/system_info_cpu/cpu_info_provider_win.cc:51: if (!NtQuerySystemInformation) On 2012/09/11 16:33:16, alexeypa wrote: > nit: I believe this can be CHECK(NtQuerySystemInformation) Directly use NtQuerySystemInformation API as defined in winternl.h now. http://codereview.chromium.org/10916197/diff/3001/chrome/browser/extensions/a... chrome/browser/extensions/api/system_info_cpu/cpu_info_provider_win.cc:54: int num_of_processors = base::SysInfo::NumberOfProcessors(); On 2012/09/11 16:33:16, alexeypa wrote: > nit: Why is this number signed? The base::SysInfo::NumberOfProcessors returns signed number. http://codereview.chromium.org/10916197/diff/3001/chrome/browser/extensions/a... chrome/browser/extensions/api/system_info_cpu/cpu_info_provider_win.cc:55: On 2012/09/11 16:33:16, alexeypa wrote: > nit: no need for an empty line here. Done. http://codereview.chromium.org/10916197/diff/3001/chrome/browser/extensions/a... chrome/browser/extensions/api/system_info_cpu/cpu_info_provider_win.cc:59: ULONG bytes, returned_bytes; On 2012/09/11 16:33:16, alexeypa wrote: > nit: Declare both |bytes| and |returned_bytes| on separate lines. Initialize and > declare them in the same statement. Done. http://codereview.chromium.org/10916197/diff/3001/chrome/browser/extensions/a... chrome/browser/extensions/api/system_info_cpu/cpu_info_provider_win.cc:62: On 2012/09/11 16:33:16, alexeypa wrote: > nit: no need for an empty line here. Done. http://codereview.chromium.org/10916197/diff/3001/chrome/browser/extensions/a... chrome/browser/extensions/api/system_info_cpu/cpu_info_provider_win.cc:72: DCHECK(returned_num_of_processors == num_of_processors); On 2012/09/11 16:33:16, alexeypa wrote: > 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%28v=sql.105%29.aspx%29? 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. Return false in case of that error happens. http://codereview.chromium.org/10916197/diff/3001/chrome/browser/extensions/a... chrome/browser/extensions/api/system_info_cpu/cpu_info_provider_win.cc:76: On 2012/09/11 16:33:16, alexeypa wrote: > nit: no need for an empty line. Done.
http://codereview.chromium.org/10916197/diff/3001/chrome/browser/extensions/a... File chrome/browser/extensions/api/system_info_cpu/cpu_info_provider_win.cc (right): http://codereview.chromium.org/10916197/diff/3001/chrome/browser/extensions/a... chrome/browser/extensions/api/system_info_cpu/cpu_info_provider_win.cc:54: int num_of_processors = base::SysInfo::NumberOfProcessors(); On 2012/09/12 12:31:37, Hongbo Min wrote: > On 2012/09/11 16:33:16, alexeypa wrote: > > nit: Why is this number signed? > > The base::SysInfo::NumberOfProcessors returns signed number. 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); http://codereview.chromium.org/10916197/diff/8003/chrome/browser/extensions/a... File chrome/browser/extensions/api/system_info_cpu/cpu_info_provider_win.cc (right): http://codereview.chromium.org/10916197/diff/8003/chrome/browser/extensions/a... chrome/browser/extensions/api/system_info_cpu/cpu_info_provider_win.cc:28: if (!times) Actually, you don't need to test |times| for being NULL here at all. AV while trying to dereference |times| is a better indication that something is wrong. The return value is too easy to ignore. http://codereview.chromium.org/10916197/diff/8003/chrome/browser/extensions/a... chrome/browser/extensions/api/system_info_cpu/cpu_info_provider_win.cc:34: DCHECK(ntdll != NULL); DCHECK -> CHECK http://codereview.chromium.org/10916197/diff/8003/chrome/browser/extensions/a... chrome/browser/extensions/api/system_info_cpu/cpu_info_provider_win.cc:39: if (!NtQuerySystemInformation) This also can be CHECK(NtQuerySystemInformation != NULL). http://codereview.chromium.org/10916197/diff/8003/chrome/browser/extensions/a... chrome/browser/extensions/api/system_info_cpu/cpu_info_provider_win.cc:46: ULONG bytes = 0; You can merge this line with line #48. http://codereview.chromium.org/10916197/diff/8003/chrome/browser/extensions/a... chrome/browser/extensions/api/system_info_cpu/cpu_info_provider_win.cc:49: if (NtQuerySystemInformation(SystemProcessorPerformanceInformation, Use NT_SUCCESS macro to test whether NtQuerySystemInformation succeeded. http://codereview.chromium.org/10916197/diff/8003/chrome/browser/extensions/a... chrome/browser/extensions/api/system_info_cpu/cpu_info_provider_win.cc:57: DCHECK(returned_num_of_processors == num_of_processors); Remove this DCHECK. It is legal (though unlikely) situation when NtQuerySystemInformation will return different number of processors than requested. http://codereview.chromium.org/10916197/diff/8003/chrome/browser/extensions/a... chrome/browser/extensions/api/system_info_cpu/cpu_info_provider_win.cc:61: for (int i = 0; i < returned_num_of_processors; ++i) { Since you know the number of elements you are going to put to the vector, preallocate them with std::vector::reserve() call before the loop.
Patch is updated. http://codereview.chromium.org/10916197/diff/8003/chrome/browser/extensions/a... File chrome/browser/extensions/api/system_info_cpu/cpu_info_provider_win.cc (right): http://codereview.chromium.org/10916197/diff/8003/chrome/browser/extensions/a... chrome/browser/extensions/api/system_info_cpu/cpu_info_provider_win.cc:28: if (!times) On 2012/09/12 15:41:55, alexeypa wrote: > Actually, you don't need to test |times| for being NULL here at all. AV while > trying to dereference |times| is a better indication that something is wrong. > The return value is too easy to ignore. What does AV stand for? Although it is too easy, it guarantees that the QueryCpuTimePerProcessor works as normal in any case,even a null point is passed. It is just for the API's completeness:) http://codereview.chromium.org/10916197/diff/8003/chrome/browser/extensions/a... chrome/browser/extensions/api/system_info_cpu/cpu_info_provider_win.cc:34: DCHECK(ntdll != NULL); On 2012/09/12 15:41:55, alexeypa wrote: > DCHECK -> CHECK Done. http://codereview.chromium.org/10916197/diff/8003/chrome/browser/extensions/a... chrome/browser/extensions/api/system_info_cpu/cpu_info_provider_win.cc:39: if (!NtQuerySystemInformation) On 2012/09/12 15:41:55, alexeypa wrote: > This also can be CHECK(NtQuerySystemInformation != NULL). Done. http://codereview.chromium.org/10916197/diff/8003/chrome/browser/extensions/a... chrome/browser/extensions/api/system_info_cpu/cpu_info_provider_win.cc:46: ULONG bytes = 0; On 2012/09/12 15:41:55, alexeypa wrote: > You can merge this line with line #48. Done. http://codereview.chromium.org/10916197/diff/8003/chrome/browser/extensions/a... chrome/browser/extensions/api/system_info_cpu/cpu_info_provider_win.cc:49: if (NtQuerySystemInformation(SystemProcessorPerformanceInformation, On 2012/09/12 15:41:55, alexeypa wrote: > Use NT_SUCCESS macro to test whether NtQuerySystemInformation succeeded. Done. http://codereview.chromium.org/10916197/diff/8003/chrome/browser/extensions/a... chrome/browser/extensions/api/system_info_cpu/cpu_info_provider_win.cc:57: DCHECK(returned_num_of_processors == num_of_processors); On 2012/09/12 15:41:55, alexeypa wrote: > Remove this DCHECK. It is legal (though unlikely) situation when > NtQuerySystemInformation will return different number of processors than > requested. Removed the DCHECK stmt, but still return false if their values are different to avoid the inconsistency if the consumer also use SysInfo::NumberOfProcessors to get the number of processors. http://codereview.chromium.org/10916197/diff/8003/chrome/browser/extensions/a... chrome/browser/extensions/api/system_info_cpu/cpu_info_provider_win.cc:61: for (int i = 0; i < returned_num_of_processors; ++i) { On 2012/09/12 15:41:55, alexeypa wrote: > Since you know the number of elements you are going to put to the vector, > preallocate them with std::vector::reserve() call before the loop. Done.
lgtm when my comments are addressed. http://codereview.chromium.org/10916197/diff/8003/chrome/browser/extensions/a... File chrome/browser/extensions/api/system_info_cpu/cpu_info_provider_win.cc (right): http://codereview.chromium.org/10916197/diff/8003/chrome/browser/extensions/a... chrome/browser/extensions/api/system_info_cpu/cpu_info_provider_win.cc:28: if (!times) On 2012/09/13 13:16:59, Hongbo Min wrote: > On 2012/09/12 15:41:55, alexeypa wrote: > > Actually, you don't need to test |times| for being NULL here at all. AV while > > trying to dereference |times| is a better indication that something is wrong. > > The return value is too easy to ignore. > What does AV stand for? Access violation. > Although it is too easy, it guarantees that the > QueryCpuTimePerProcessor works as normal in any case,even a null point is > passed. It is just for the API's completeness:) Probing a pointer parameter is a good idea if the API crosses trust boundary (user -> kernel) or it is a public API. In this case however, no trust boundaries are crossed. Both the caller and the callee live in the same binary. If a caller passes a bogus pointer it will be immediately seen as a crash, and so it will be fixed. Returning an error does not guarantee anything, since it can be easily ignored. There is a different way to put it. Without the check if a caller passes an invalid pointer (either NULL or not-NULL invalid pointer) the function will generate access violation. With the check the function return false if the pointer is NULL and crash if the pointer is invalid but not NULL. So you have an additional variation in behavior that needs to be tested. http://codereview.chromium.org/10916197/diff/1003/chrome/browser/extensions/a... File chrome/browser/extensions/api/system_info_cpu/cpu_info_provider_win.cc (right): http://codereview.chromium.org/10916197/diff/1003/chrome/browser/extensions/a... chrome/browser/extensions/api/system_info_cpu/cpu_info_provider_win.cc:40: if (!NtQuerySystemInformation) nit: remove the if statement. CHECK will crash the process if the pointer is NULL (in both debug and release build), so the condition in the if statement will never be true. http://codereview.chromium.org/10916197/diff/1003/chrome/browser/extensions/a... chrome/browser/extensions/api/system_info_cpu/cpu_info_provider_win.cc:48: ULONG bytes = 0, returned_bytes = 0; nit: Sorry, what I mean is: ULONG bytes = sizeof(SYSTEM_PROCESSOR_PERFORMANCE_INFORMATION) * num_of_processors; ULONG returned_bytes = 0; http://codereview.chromium.org/10916197/diff/1003/chrome/browser/extensions/a... chrome/browser/extensions/api/system_info_cpu/cpu_info_provider_win.cc:55: int returned_num_of_processors = nit: The type should be ULONG since |returned_bytes| and sizeof() are unsigned. http://codereview.chromium.org/10916197/diff/1003/chrome/browser/extensions/a... chrome/browser/extensions/api/system_info_cpu/cpu_info_provider_win.cc:70: results.swap(*times); nit: This probably is more readable if it says times->swap(results).
http://codereview.chromium.org/10916197/diff/1003/chrome/browser/extensions/a... File chrome/browser/extensions/api/system_info_cpu/cpu_info_provider_win.cc (right): http://codereview.chromium.org/10916197/diff/1003/chrome/browser/extensions/a... chrome/browser/extensions/api/system_info_cpu/cpu_info_provider_win.cc:40: if (!NtQuerySystemInformation) On 2012/09/13 16:45:48, alexeypa wrote: > nit: remove the if statement. CHECK will crash the process if the pointer is > NULL (in both debug and release build), so the condition in the if statement > will never be true. Done. http://codereview.chromium.org/10916197/diff/1003/chrome/browser/extensions/a... chrome/browser/extensions/api/system_info_cpu/cpu_info_provider_win.cc:55: int returned_num_of_processors = On 2012/09/13 16:45:48, alexeypa wrote: > nit: The type should be ULONG since |returned_bytes| and sizeof() are unsigned. Yes, but because the num_of_processors is int type, so use int to avoid static_cast when comparison. http://codereview.chromium.org/10916197/diff/1003/chrome/browser/extensions/a... chrome/browser/extensions/api/system_info_cpu/cpu_info_provider_win.cc:70: results.swap(*times); On 2012/09/13 16:45:48, alexeypa wrote: > nit: This probably is more readable if it says times->swap(results). Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hongbo.min@intel.com/10916197/11003
Change committed as 156748 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
