Refine systemInfo.cpu API defintions and provide systemInfo.cpu.get impl for Windows and Linux

Refine systemInfo.cpu API and provide systemInfo.cpu.get impl for Windows and Linux.


BUG=136519
TEST=browser_test --gtest_filter=SystemInfoCpuApiTest.*

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=155575

[Hongbo Min, 2012-09-03 14:53:02]

Mihai,

Since the systemInfo.cpu API definition has some changes as described in
https://docs.google.com/a/intel.com/document/pub?id=13RUe2D67p4bZx9UvDeGzh2u6...,
this patch is to update the systemInfo.cpu.get implementation accordingly.

Since the load of the logic processors actually needs to sampling. In this
patch, I add two sampling interface for systemInfo.cpu.onUpdated event.

For cpu model name, it is now merged into SysInfo namespace. 

Pls have a review. Thanks.

[Mihai Parparita -not on Chrome, 2012-09-04 20:23:47]

http://codereview.chromium.org/10914060/diff/3017/chrome/browser/extensions/a...
File chrome/browser/extensions/api/system_info_cpu/cpu_info_provider.h (right):

http://codereview.chromium.org/10914060/diff/3017/chrome/browser/extensions/a...
chrome/browser/extensions/api/system_info_cpu/cpu_info_provider.h:26: // Start
sampling the cpu usage. The callback gets called when one sampling
Capitalize CPU here and elsewhere.

http://codereview.chromium.org/10914060/diff/3017/chrome/common/extensions/ap...
File chrome/common/extensions/api/experimental_system_info_cpu.idl (right):

http://codereview.chromium.org/10914060/diff/3017/chrome/common/extensions/ap...
chrome/common/extensions/api/experimental_system_info_cpu.idl:19: long avgLoad;
Don't abbreviate "average".

Also, "load" seems like a confusing term, since it could mean the Linux load
average, which is not what is returned here. "usage" perhaps?

Also, CPU usage is generally represented with a double (see
http://code.google.com/searchframe#OAMlx_jo-ck/src/base/process_util.h&type=c...)

http://codereview.chromium.org/10914060/diff/3017/chrome/test/data/extensions...
File chrome/test/data/extensions/api_test/systeminfo/cpu/test_cpuinfo_api.js
(right):

http://codereview.chromium.org/10914060/diff/3017/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/systeminfo/cpu/test_cpuinfo_api.js:14:
chrome.test.assertTrue(result.numOfProcessors == 4);
You can use chrome.test.assertEq

[Hongbo Min, 2012-09-05 05:31:03]

Patch updated. Thanks.

http://codereview.chromium.org/10914060/diff/3017/chrome/browser/extensions/a...
File chrome/browser/extensions/api/system_info_cpu/cpu_info_provider.h (right):

http://codereview.chromium.org/10914060/diff/3017/chrome/browser/extensions/a...
chrome/browser/extensions/api/system_info_cpu/cpu_info_provider.h:26: // Start
sampling the cpu usage. The callback gets called when one sampling
On 2012/09/04 20:23:47, Mihai Parparita wrote:
> Capitalize CPU here and elsewhere.

Done.

http://codereview.chromium.org/10914060/diff/3017/chrome/common/extensions/ap...
File chrome/common/extensions/api/experimental_system_info_cpu.idl (right):

http://codereview.chromium.org/10914060/diff/3017/chrome/common/extensions/ap...
chrome/common/extensions/api/experimental_system_info_cpu.idl:19: long avgLoad;
On 2012/09/04 20:23:47, Mihai Parparita wrote:
> Don't abbreviate "average".
> 
> Also, "load" seems like a confusing term, since it could mean the Linux load
> average, which is not what is returned here. "usage" perhaps?
> 
> Also, CPU usage is generally represented with a double (see
>
http://code.google.com/searchframe#OAMlx_jo-ck/src/base/process_util.h&type=c...)

Done.

http://codereview.chromium.org/10914060/diff/3017/chrome/test/data/extensions...
File chrome/test/data/extensions/api_test/systeminfo/cpu/test_cpuinfo_api.js
(right):

http://codereview.chromium.org/10914060/diff/3017/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/systeminfo/cpu/test_cpuinfo_api.js:14:
chrome.test.assertTrue(result.numOfProcessors == 4);
On 2012/09/04 20:23:47, Mihai Parparita wrote:
> You can use chrome.test.assertEq

Done.

[Hongbo Min, 2012-09-05 14:31:50]

Mihai,

If it looks good to you, please call out the reviewers for base and gypi
changes. Thanks.

[Mihai Parparita -not on Chrome, 2012-09-05 23:58:39]

LGTM

As for other OWNERS, Mark has looked at base/sys_info* in the past, and Scott
can approve the gypi change.

http://codereview.chromium.org/10914060/diff/24/base/sys_info_linux.cc
File base/sys_info_linux.cc (right):

http://codereview.chromium.org/10914060/diff/24/base/sys_info_linux.cc#newcode58
base/sys_info_linux.cc:58: while(std::getline(iss, line)){
Nit: space after while.

http://codereview.chromium.org/10914060/diff/24/chrome/common/extensions/api/...
File chrome/common/extensions/api/experimental_system_info_cpu.idl (right):

http://codereview.chromium.org/10914060/diff/24/chrome/common/extensions/api/...
chrome/common/extensions/api/experimental_system_info_cpu.idl:22: double[]
usagesPerProcessor;
Nit: usagePerProcessor is more correct.

http://codereview.chromium.org/10914060/diff/24/chrome/test/data/extensions/a...
File chrome/test/data/extensions/api_test/systeminfo/cpu/test_cpuinfo_api.js
(right):

http://codereview.chromium.org/10914060/diff/24/chrome/test/data/extensions/a...
chrome/test/data/extensions/api_test/systeminfo/cpu/test_cpuinfo_api.js:14:
chrome.test.assertTrue(result.numOfProcessors == 4);
Still not seeing assertEq here.

[sky, 2012-09-06 00:09:22]

LGTM

[Hongbo Min, 2012-09-06 01:15:39]

@willchan, please review the base changes.

[Hongbo Min, 2012-09-06 01:16:41]

@mark, please help review. Thanks.

[Hongbo Min, 2012-09-06 01:43:53]

http://codereview.chromium.org/10914060/diff/24/base/sys_info_linux.cc
File base/sys_info_linux.cc (right):

http://codereview.chromium.org/10914060/diff/24/base/sys_info_linux.cc#newcode58
base/sys_info_linux.cc:58: while(std::getline(iss, line)){
On 2012/09/05 23:58:39, Mihai Parparita wrote:
> Nit: space after while.

Done.

http://codereview.chromium.org/10914060/diff/24/chrome/common/extensions/api/...
File chrome/common/extensions/api/experimental_system_info_cpu.idl (right):

http://codereview.chromium.org/10914060/diff/24/chrome/common/extensions/api/...
chrome/common/extensions/api/experimental_system_info_cpu.idl:22: double[]
usagesPerProcessor;
On 2012/09/05 23:58:39, Mihai Parparita wrote:
> Nit: usagePerProcessor is more correct.

Done.

http://codereview.chromium.org/10914060/diff/24/chrome/test/data/extensions/a...
File chrome/test/data/extensions/api_test/systeminfo/cpu/test_cpuinfo_api.js
(right):

http://codereview.chromium.org/10914060/diff/24/chrome/test/data/extensions/a...
chrome/test/data/extensions/api_test/systeminfo/cpu/test_cpuinfo_api.js:14:
chrome.test.assertTrue(result.numOfProcessors == 4);
On 2012/09/05 23:58:39, Mihai Parparita wrote:
> Still not seeing assertEq here.

Done.

[Mark Mentovai, 2012-09-06 13:39:13]

What is the plan for other-OS support? Subsequent changes? Are you going to do
it?

http://codereview.chromium.org/10914060/diff/4013/base/sys_info_ios.mm
File base/sys_info_ios.mm (right):

http://codereview.chromium.org/10914060/diff/4013/base/sys_info_ios.mm#newcode70
base/sys_info_ios.mm:70: // TODO(hongbo): Retrieve cpu model name.
+stuartmorgan to assist here or just to make him aware of this new API.

http://codereview.chromium.org/10914060/diff/4013/base/sys_info_linux.cc
File base/sys_info_linux.cc (right):

http://codereview.chromium.org/10914060/diff/4013/base/sys_info_linux.cc#newc...
base/sys_info_linux.cc:51: static const char kModelNamePrefix[] = "model name";
“static” is useless here.

http://codereview.chromium.org/10914060/diff/4013/base/sys_info_linux.cc#newc...
base/sys_info_linux.cc:59: if (line.empty()) break;
This is sort of redundant given what happens next, isn’t it?

http://codereview.chromium.org/10914060/diff/4013/base/sys_info_mac.cc
File base/sys_info_mac.cc (right):

http://codereview.chromium.org/10914060/diff/4013/base/sys_info_mac.cc#newcode59
base/sys_info_mac.cc:59: // TODO(hongbo): Retrive cpu model name.
The machdep.cpu.brand_string sysctl is equivalent to the “model name” field in
Linux’ /proc/cpuinfo.

[Hongbo Min, 2012-09-06 13:43:04]

On 2012/09/06 13:39:13, Mark Mentovai wrote:
> What is the plan for other-OS support? Subsequent changes? Are you going to do
> it?
> 

The other-OS support besides Windows and Linux will be done in another CL. I
know someone is already doing that.

[Hongbo Min, 2012-09-06 14:09:09]

The patch is update accordingly. 

The other-OS you mean is android, or freebsd?

http://codereview.chromium.org/10914060/diff/4013/base/sys_info_linux.cc
File base/sys_info_linux.cc (right):

http://codereview.chromium.org/10914060/diff/4013/base/sys_info_linux.cc#newc...
base/sys_info_linux.cc:51: static const char kModelNamePrefix[] = "model name";
On 2012/09/06 13:39:13, Mark Mentovai wrote:
> “static” is useless here.

Remove it already.

http://codereview.chromium.org/10914060/diff/4013/base/sys_info_linux.cc#newc...
base/sys_info_linux.cc:59: if (line.empty()) break;
On 2012/09/06 13:39:13, Mark Mentovai wrote:
> This is sort of redundant given what happens next, isn’t it?

Good catch. Thanks.

http://codereview.chromium.org/10914060/diff/4013/base/sys_info_mac.cc
File base/sys_info_mac.cc (right):

http://codereview.chromium.org/10914060/diff/4013/base/sys_info_mac.cc#newcode59
base/sys_info_mac.cc:59: // TODO(hongbo): Retrive cpu model name.
On 2012/09/06 13:39:13, Mark Mentovai wrote:
> The machdep.cpu.brand_string sysctl is equivalent to the “model
name” field in
> Linux’ /proc/cpuinfo.

Thanks for you hints. The impl is uploaded.

[Mark Mentovai, 2012-09-06 14:11:48]

I meant fully implementing the TODOs you left behind, like the CPU time thing
for Mac and Linux.

[Hongbo Min, 2012-09-06 14:13:28]

On 2012/09/06 14:11:48, Mark Mentovai wrote:
> I meant fully implementing the TODOs you left behind, like the CPU time thing
> for Mac and Linux.

Yes, I am doing it in parallel.

[Mark Mentovai, 2012-09-06 14:17:56]

http://codereview.chromium.org/10914060/diff/7034/base/sys_info_ios.mm
File base/sys_info_ios.mm (right):

http://codereview.chromium.org/10914060/diff/7034/base/sys_info_ios.mm#newcode72
base/sys_info_ios.mm:72: sysctlbyname("machdep.cpu.brand_string", &model_name,
&len, NULL, 0);
Need to #include anything?

http://codereview.chromium.org/10914060/diff/7034/base/sys_info_ios.mm#newcode74
base/sys_info_ios.mm:74: }
Other comments here are the same as in the Mac file.

http://codereview.chromium.org/10914060/diff/7034/base/sys_info_mac.cc
File base/sys_info_mac.cc (right):

http://codereview.chromium.org/10914060/diff/7034/base/sys_info_mac.cc#newcode61
base/sys_info_mac.cc:61: char model_name[256] = { 0 };
You don’t need the { 0 } at all It’s fine to leave this uninitialized because
you should be checking the return value from sysctlbyname.

http://codereview.chromium.org/10914060/diff/7034/base/sys_info_mac.cc#newcode62
base/sys_info_mac.cc:62: size_t len = 256;
Not 256, but arraysize(model_name).

http://codereview.chromium.org/10914060/diff/7034/base/sys_info_mac.cc#newcode63
base/sys_info_mac.cc:63: sysctlbyname("machdep.cpu.brand_string", &model_name,
&len, NULL, 0);
You should check the return value, and return an empty string when it’s nonzero.

[Hongbo Min, 2012-09-06 14:33:45]

Done. Pls have a review again. Thanks.

http://codereview.chromium.org/10914060/diff/7034/base/sys_info_ios.mm
File base/sys_info_ios.mm (right):

http://codereview.chromium.org/10914060/diff/7034/base/sys_info_ios.mm#newcode72
base/sys_info_ios.mm:72: sysctlbyname("machdep.cpu.brand_string", &model_name,
&len, NULL, 0);
On 2012/09/06 14:17:56, Mark Mentovai wrote:
> Need to #include anything?

Done.

http://codereview.chromium.org/10914060/diff/7034/base/sys_info_ios.mm#newcode74
base/sys_info_ios.mm:74: }
On 2012/09/06 14:17:56, Mark Mentovai wrote:
> Other comments here are the same as in the Mac file.

Done.

http://codereview.chromium.org/10914060/diff/7034/base/sys_info_mac.cc
File base/sys_info_mac.cc (right):

http://codereview.chromium.org/10914060/diff/7034/base/sys_info_mac.cc#newcode61
base/sys_info_mac.cc:61: char model_name[256] = { 0 };
On 2012/09/06 14:17:56, Mark Mentovai wrote:
> You don’t need the { 0 } at all It’s fine to leave this uninitialized because
> you should be checking the return value from sysctlbyname.

Done.

http://codereview.chromium.org/10914060/diff/7034/base/sys_info_mac.cc#newcode62
base/sys_info_mac.cc:62: size_t len = 256;
On 2012/09/06 14:17:56, Mark Mentovai wrote:
> Not 256, but arraysize(model_name).

Done.

http://codereview.chromium.org/10914060/diff/7034/base/sys_info_mac.cc#newcode63
base/sys_info_mac.cc:63: sysctlbyname("machdep.cpu.brand_string", &model_name,
&len, NULL, 0);
On 2012/09/06 14:17:56, Mark Mentovai wrote:
> You should check the return value, and return an empty string when it’s
nonzero.

Done.

[Mark Mentovai, 2012-09-06 14:38:14]

http://codereview.chromium.org/10914060/diff/28/base/sys_info.h
File base/sys_info.h (right):

http://codereview.chromium.org/10914060/diff/28/base/sys_info.h#newcode58
base/sys_info.h:58: // Returns the CPU model name of the system.
Document “unknown”.

Although personally I’d prefer the empty string. It’s not clear to me, but it
appears that you would already use the empty string in the case of a failure on
Windows.

[Hongbo Min, 2012-09-06 14:43:24]

Agree. Thanks.

[Hongbo Min, 2012-09-06 14:45:00]

http://codereview.chromium.org/10914060/diff/28/base/sys_info.h
File base/sys_info.h (right):

http://codereview.chromium.org/10914060/diff/28/base/sys_info.h#newcode58
base/sys_info.h:58: // Returns the CPU model name of the system.
On 2012/09/06 14:38:14, Mark Mentovai wrote:
> Document “unknown”.
> 
> Although personally I’d prefer the empty string. It’s not clear to me, but it
> appears that you would already use the empty string in the case of a failure
on
> Windows.

Agree to use empty string directly instead of “unknown". Done

[Mark Mentovai, 2012-09-06 14:45:17]

LGTM otherwise.

http://codereview.chromium.org/10914060/diff/14035/base/sys_info.h
File base/sys_info.h (right):

http://codereview.chromium.org/10914060/diff/14035/base/sys_info.h#newcode58
base/sys_info.h:58: // Returns the CPU model name of the system.
You should still say here that you’ll return the empty string if you can’t
figure out the answer.

http://codereview.chromium.org/10914060/diff/14035/base/sys_info_ios.mm
File base/sys_info_ios.mm (right):

http://codereview.chromium.org/10914060/diff/14035/base/sys_info_ios.mm#newco...
base/sys_info_ios.mm:76: return "";
Rather than an implicit conversion, use std::string().

http://codereview.chromium.org/10914060/diff/14035/base/sys_info_linux.cc
File base/sys_info_linux.cc (right):

http://codereview.chromium.org/10914060/diff/14035/base/sys_info_linux.cc#new...
base/sys_info_linux.cc:65: return "";
Same.

http://codereview.chromium.org/10914060/diff/14035/base/sys_info_mac.cc
File base/sys_info_mac.cc (right):

http://codereview.chromium.org/10914060/diff/14035/base/sys_info_mac.cc#newco...
base/sys_info_mac.cc:65: return "";
Same.

[Hongbo Min, 2012-09-06 14:55:15]

http://codereview.chromium.org/10914060/diff/14035/base/sys_info.h
File base/sys_info.h (right):

http://codereview.chromium.org/10914060/diff/14035/base/sys_info.h#newcode58
base/sys_info.h:58: // Returns the CPU model name of the system.
On 2012/09/06 14:45:18, Mark Mentovai wrote:
> You should still say here that you’ll return the empty string if you can’t
> figure out the answer.

Done.

http://codereview.chromium.org/10914060/diff/14035/base/sys_info_ios.mm
File base/sys_info_ios.mm (right):

http://codereview.chromium.org/10914060/diff/14035/base/sys_info_ios.mm#newco...
base/sys_info_ios.mm:76: return "";
On 2012/09/06 14:45:18, Mark Mentovai wrote:
> Rather than an implicit conversion, use std::string().

Done.

http://codereview.chromium.org/10914060/diff/14035/base/sys_info_linux.cc
File base/sys_info_linux.cc (right):

http://codereview.chromium.org/10914060/diff/14035/base/sys_info_linux.cc#new...
base/sys_info_linux.cc:65: return "";
On 2012/09/06 14:45:18, Mark Mentovai wrote:
> Same.

Done.

http://codereview.chromium.org/10914060/diff/14035/base/sys_info_mac.cc
File base/sys_info_mac.cc (right):

http://codereview.chromium.org/10914060/diff/14035/base/sys_info_mac.cc#newco...
base/sys_info_mac.cc:65: return "";
On 2012/09/06 14:45:18, Mark Mentovai wrote:
> Same.

Done.

[Mark Mentovai, 2012-09-06 15:38:35]

LGTM

[Hongbo Min, 2012-09-07 06:48:56]

On 2012/09/06 15:38:35, Mark Mentovai wrote:
> LGTM

Do you know why can not I commit this patch? No any response although the
'commit' checkbox is checked.

[commit-bot: I haz the power, 2012-09-07 12:38:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hongbo.min@intel.com/10914060/7054

[commit-bot: I haz the power, 2012-09-07 12:56:13]

Try job failure for 10914060-7054 (retry) on win for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[commit-bot: I haz the power, 2012-09-07 16:00:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hongbo.min@intel.com/10914060/3059

[commit-bot: I haz the power, 2012-09-07 17:20:56]

Try job failure for 10914060-3059 (retry) on mac_rel for step "browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-09-08 02:11:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hongbo.min@intel.com/10914060/14060

[commit-bot: I haz the power, 2012-09-08 09:53:03]

Change committed as 155575

[Lei Zhang, 2012-09-13 02:25:21]

Post review comment: have you considered using base/cpu.h instead of adding the
win/mac/linux specific code?

[Hongbo Min, 2012-09-13 04:27:20]

On 2012/09/13 02:25:21, Lei Zhang wrote:
> Post review comment: have you considered using base/cpu.h instead of adding
the
> win/mac/linux specific code?

base/cpu.h seems to be what the cpu info provider wants, and base/cpu.h tries to
provide a detailed low-level information about CPU, will it also provide other
information, like numberOfProcessor and frequency.

In addition, can we make Cpu methods as static?

[Lei Zhang, 2012-09-13 04:33:46]

On 2012/09/13 04:27:20, Hongbo Min wrote:
> On 2012/09/13 02:25:21, Lei Zhang wrote:
> > Post review comment: have you considered using base/cpu.h instead of adding
> the
> > win/mac/linux specific code?
> 
> base/cpu.h seems to be what the cpu info provider wants, and base/cpu.h tries
to
> provide a detailed low-level information about CPU, will it also provide other
> information, like numberOfProcessor and frequency.
> 
> In addition, can we make Cpu methods as static?

Do we need to? Is it expensive to call the cpuid instruction?

[Hongbo Min, 2012-09-13 04:36:23]

On 2012/09/13 04:33:46, Lei Zhang wrote:
> On 2012/09/13 04:27:20, Hongbo Min wrote:
> > On 2012/09/13 02:25:21, Lei Zhang wrote:
> > > Post review comment: have you considered using base/cpu.h instead of
adding
> > the
> > > win/mac/linux specific code?
> > 
> > base/cpu.h seems to be what the cpu info provider wants, and base/cpu.h
tries
> to
> > provide a detailed low-level information about CPU, will it also provide
other
> > information, like numberOfProcessor and frequency.
> > 
> > In addition, can we make Cpu methods as static?
> 
> Do we need to? Is it expensive to call the cpuid instruction?

I mean only calling cpuid instruction once, and saving the result into a static
object, those static methods just the info requested. The cpu information is
unlikely to be changed during the whole application lifecycle.

[Lei Zhang, 2012-09-13 04:42:28]

On 2012/09/13 04:36:23, Hongbo Min wrote:
> On 2012/09/13 04:33:46, Lei Zhang wrote:
> > On 2012/09/13 04:27:20, Hongbo Min wrote:
> > > On 2012/09/13 02:25:21, Lei Zhang wrote:
> > > > Post review comment: have you considered using base/cpu.h instead of
> adding
> > > the
> > > > win/mac/linux specific code?
> > > 
> > > base/cpu.h seems to be what the cpu info provider wants, and base/cpu.h
> tries
> > to
> > > provide a detailed low-level information about CPU, will it also provide
> other
> > > information, like numberOfProcessor and frequency.
> > > 
> > > In addition, can we make Cpu methods as static?
> > 
> > Do we need to? Is it expensive to call the cpuid instruction?
> 
> I mean only calling cpuid instruction once, and saving the result into a
static
> object, those static methods just the info requested. The cpu information is
> unlikely to be changed during the whole application lifecycle.

You probably can. In any case, I'm just trying to suggest a way to avoid having
multiple ways of doing the same thing.

[Hongbo Min, 2012-09-13 04:45:57]

Agree.

btw, could you point me where is using the cpu.h in trunk code?

[Lei Zhang, 2012-09-13 04:53:27]

On 2012/09/13 04:45:57, Hongbo Min wrote:
> Agree.
> 
> btw, could you point me where is using the cpu.h in trunk code?

Search on cs.chromium.org for: base::cpu

[Hongbo Min, 2012-09-13 04:59:36]

On 2012/09/13 04:53:27, Lei Zhang wrote:
> On 2012/09/13 04:45:57, Hongbo Min wrote:
> > Agree.
> > 
> > btw, could you point me where is using the cpu.h in trunk code?
> 
> Search on http://cs.chromium.org for: base::cpu

fired http://crbug.com/148884 for tracking it.