Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10907139:
Implement querying CPU time from /proc/stat on Linux for systemInfo.cpu API (Closed)
|
Created:
8 years, 3 months ago by Hongbo Min Modified:
8 years, 2 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 from /proc/stat on Linux for systemInfo.cpu API
BUG=136519
TEST=None
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=157984
Patch Set 1 #
Total comments: 17
Patch Set 2 : #
Total comments: 8
Patch Set 3 : #Patch Set 4 : #Messages
Total messages: 17 (0 generated)
Pls have a review.
http://codereview.chromium.org/10907139/diff/1/chrome/browser/extensions/api/... File chrome/browser/extensions/api/system_info_cpu/cpu_info_provider_linux.cc (right): http://codereview.chromium.org/10907139/diff/1/chrome/browser/extensions/api/... chrome/browser/extensions/api/system_info_cpu/cpu_info_provider_linux.cc:12: if (!times) return false; Why the soft failure when times is null? A CHECK seems more appropriate. http://codereview.chromium.org/10907139/diff/1/chrome/browser/extensions/api/... chrome/browser/extensions/api/system_info_cpu/cpu_info_provider_linux.cc:16: const char* kProcStat = "/proc/stat"; Move this to an anonymous namespace at the head of the file. http://codereview.chromium.org/10907139/diff/1/chrome/browser/extensions/api/... chrome/browser/extensions/api/system_info_cpu/cpu_info_provider_linux.cc:17: std::string contents, line; "line" isn't used until later, its declaration should be closed to where it's used. http://codereview.chromium.org/10907139/diff/1/chrome/browser/extensions/api/... chrome/browser/extensions/api/system_info_cpu/cpu_info_provider_linux.cc:21: std::istringstream iss(contents); Needs an #include. http://codereview.chromium.org/10907139/diff/1/chrome/browser/extensions/api/... chrome/browser/extensions/api/system_info_cpu/cpu_info_provider_linux.cc:24: unsigned long long user, nice, sys, idle; Use uint64 (see http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Intege... about when to use the built-in integer types). Also, it's more idiomatic in Chrome C++ code to have one variable declaration per line, and to also initialize in the declaration (see http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Local_...) http://codereview.chromium.org/10907139/diff/1/chrome/browser/extensions/api/... chrome/browser/extensions/api/system_info_cpu/cpu_info_provider_linux.cc:31: if (line.compare(0, 3, "cpu") == 0) { Turn this into a "if (line.compare(0, 3, "cpu") != 0) {continue}", so that the body doesn't have to be indented as far. http://codereview.chromium.org/10907139/diff/1/chrome/browser/extensions/api/... chrome/browser/extensions/api/system_info_cpu/cpu_info_provider_linux.cc:32: sscanf(line.c_str(), "%*s %llu %llu %llu %llu", Needs an #include
http://codereview.chromium.org/10907139/diff/1/chrome/browser/extensions/api/... File chrome/browser/extensions/api/system_info_cpu/cpu_info_provider_linux.cc (right): http://codereview.chromium.org/10907139/diff/1/chrome/browser/extensions/api/... chrome/browser/extensions/api/system_info_cpu/cpu_info_provider_linux.cc:28: // Ignore the first line because it is just an aggregated number of Also, given this behavior, you may want to remove the "Note that the first element is the total aggregated numbers of all logic processors." comment in cpu_info_provider.h, since it's not actually true.
http://codereview.chromium.org/10907139/diff/1/chrome/browser/extensions/api/... File chrome/browser/extensions/api/system_info_cpu/cpu_info_provider_linux.cc (right): http://codereview.chromium.org/10907139/diff/1/chrome/browser/extensions/api/... chrome/browser/extensions/api/system_info_cpu/cpu_info_provider_linux.cc:12: if (!times) return false; On 2012/09/16 06:01:22, Mihai Parparita wrote: > Why the soft failure when times is null? A CHECK seems more appropriate. No CHECK is used since access violation can help do this when trying to dereference a NULL pointer. http://codereview.chromium.org/10907139/diff/1/chrome/browser/extensions/api/... chrome/browser/extensions/api/system_info_cpu/cpu_info_provider_linux.cc:16: const char* kProcStat = "/proc/stat"; On 2012/09/16 06:01:22, Mihai Parparita wrote: > Move this to an anonymous namespace at the head of the file. Done. http://codereview.chromium.org/10907139/diff/1/chrome/browser/extensions/api/... chrome/browser/extensions/api/system_info_cpu/cpu_info_provider_linux.cc:17: std::string contents, line; On 2012/09/16 06:01:22, Mihai Parparita wrote: > "line" isn't used until later, its declaration should be closed to where it's > used. Done. http://codereview.chromium.org/10907139/diff/1/chrome/browser/extensions/api/... chrome/browser/extensions/api/system_info_cpu/cpu_info_provider_linux.cc:21: std::istringstream iss(contents); On 2012/09/16 06:01:22, Mihai Parparita wrote: > Needs an #include. Done. http://codereview.chromium.org/10907139/diff/1/chrome/browser/extensions/api/... chrome/browser/extensions/api/system_info_cpu/cpu_info_provider_linux.cc:24: unsigned long long user, nice, sys, idle; On 2012/09/16 06:01:22, Mihai Parparita wrote: > Use uint64 (see > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Intege... > about when to use the built-in integer types). > > Also, it's more idiomatic in Chrome C++ code to have one variable declaration > per line, and to also initialize in the declaration (see > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Local_...) Done. http://codereview.chromium.org/10907139/diff/1/chrome/browser/extensions/api/... chrome/browser/extensions/api/system_info_cpu/cpu_info_provider_linux.cc:28: // Ignore the first line because it is just an aggregated number of On 2012/09/16 06:07:47, Mihai Parparita wrote: > Also, given this behavior, you may want to remove the "Note that the first > element is the total aggregated numbers of all logic processors." comment in > cpu_info_provider.h, since it's not actually true. Done. Correct the comment in the cpu_info_provider.h. http://codereview.chromium.org/10907139/diff/1/chrome/browser/extensions/api/... chrome/browser/extensions/api/system_info_cpu/cpu_info_provider_linux.cc:31: if (line.compare(0, 3, "cpu") == 0) { On 2012/09/16 06:01:22, Mihai Parparita wrote: > Turn this into a "if (line.compare(0, 3, "cpu") != 0) {continue}", so that the > body doesn't have to be indented as far. Done. http://codereview.chromium.org/10907139/diff/1/chrome/browser/extensions/api/... chrome/browser/extensions/api/system_info_cpu/cpu_info_provider_linux.cc:31: if (line.compare(0, 3, "cpu") == 0) { On 2012/09/16 06:01:22, Mihai Parparita wrote: > Turn this into a "if (line.compare(0, 3, "cpu") != 0) {continue}", so that the > body doesn't have to be indented as far. Done. http://codereview.chromium.org/10907139/diff/1/chrome/browser/extensions/api/... chrome/browser/extensions/api/system_info_cpu/cpu_info_provider_linux.cc:32: sscanf(line.c_str(), "%*s %llu %llu %llu %llu", On 2012/09/16 06:01:22, Mihai Parparita wrote: > Needs an #include Done.
benwells@ Since mihai is on vocation this week, could you pls review this patch? Thanks.
On 2012/09/20 10:12:00, Hongbo Min wrote: > benwells@ > > Since mihai is on vocation this week, could you pls review this patch? Thanks. Sure, but I'm in Sydney time zone and won't get it until tomorrow (it's 9pm here). Feel free to send to someone else if you don't want to wait, or I'll look at this and the other one tomorrow.
On 2012/09/20 11:00:42, benwells wrote: > On 2012/09/20 10:12:00, Hongbo Min wrote: > > benwells@ > > > > Since mihai is on vocation this week, could you pls review this patch? Thanks. > > Sure, but I'm in Sydney time zone and won't get it until tomorrow (it's 9pm > here). Feel free to send to someone else if you don't want to wait, or I'll look > at this and the other one tomorrow. Tomorrow is also good to me:)
http://codereview.chromium.org/10907139/diff/7001/chrome/browser/extensions/a... File chrome/browser/extensions/api/system_info_cpu/cpu_info_provider_linux.cc (right): http://codereview.chromium.org/10907139/diff/7001/chrome/browser/extensions/a... chrome/browser/extensions/api/system_info_cpu/cpu_info_provider_linux.cc:21: bool CpuInfoProvider::QueryCpuTimePerProcessor(std::vector<CpuTime>* times) { DCHECK times here. This provides (a) a statement of intent in the code that times can't be NULL (b) a nice error in debug mode if it is http://codereview.chromium.org/10907139/diff/7001/chrome/browser/extensions/a... chrome/browser/extensions/api/system_info_cpu/cpu_info_provider_linux.cc:22: std::vector<CpuTime> results; Move results down to just before while loop / after the early return. Why even have a results variable? Can't you just use times? http://codereview.chromium.org/10907139/diff/7001/chrome/browser/extensions/a... chrome/browser/extensions/api/system_info_cpu/cpu_info_provider_linux.cc:29: int count = 0; Replace int count with bool first, since you just want to know if an entry is the first one or not. http://codereview.chromium.org/10907139/diff/7001/chrome/browser/extensions/a... chrome/browser/extensions/api/system_info_cpu/cpu_info_provider_linux.cc:34: // Ignore the first line because it is just an aggregated number of Mihai had asked for a comment in cpu_info_provider.h to be updated but I can't see that file in the patch. Was it updated?
http://codereview.chromium.org/10907139/diff/7001/chrome/browser/extensions/a... File chrome/browser/extensions/api/system_info_cpu/cpu_info_provider_linux.cc (right): http://codereview.chromium.org/10907139/diff/7001/chrome/browser/extensions/a... chrome/browser/extensions/api/system_info_cpu/cpu_info_provider_linux.cc:34: // Ignore the first line because it is just an aggregated number of On 2012/09/21 07:10:10, benwells wrote: > Mihai had asked for a comment in cpu_info_provider.h to be updated but I can't > see that file in the patch. Was it updated? Oh, I see it in the next review.
On 2012/09/21 08:09:14, benwells wrote: > http://codereview.chromium.org/10907139/diff/7001/chrome/browser/extensions/a... > File chrome/browser/extensions/api/system_info_cpu/cpu_info_provider_linux.cc > (right): > > http://codereview.chromium.org/10907139/diff/7001/chrome/browser/extensions/a... > chrome/browser/extensions/api/system_info_cpu/cpu_info_provider_linux.cc:34: // > Ignore the first line because it is just an aggregated number of > On 2012/09/21 07:10:10, benwells wrote: > > Mihai had asked for a comment in cpu_info_provider.h to be updated but I can't > > see that file in the patch. Was it updated? > > Oh, I see it in the next review. It is in the http://codereview.chromium.org/10905171/.
Thanks. Pls review it again. http://codereview.chromium.org/10907139/diff/7001/chrome/browser/extensions/a... File chrome/browser/extensions/api/system_info_cpu/cpu_info_provider_linux.cc (right): http://codereview.chromium.org/10907139/diff/7001/chrome/browser/extensions/a... chrome/browser/extensions/api/system_info_cpu/cpu_info_provider_linux.cc:21: bool CpuInfoProvider::QueryCpuTimePerProcessor(std::vector<CpuTime>* times) { On 2012/09/21 07:10:10, benwells wrote: > DCHECK times here. This provides > (a) a statement of intent in the code that times can't be NULL > (b) a nice error in debug mode if it is Done. http://codereview.chromium.org/10907139/diff/7001/chrome/browser/extensions/a... chrome/browser/extensions/api/system_info_cpu/cpu_info_provider_linux.cc:22: std::vector<CpuTime> results; On 2012/09/21 07:10:10, benwells wrote: > Move results down to just before while loop / after the early return. Why even > have a results variable? Can't you just use times? It is just a coding preference. See http://codereview.chromium.org/10916197/. The reviewer alexeypa seems to prefer that style. http://codereview.chromium.org/10907139/diff/7001/chrome/browser/extensions/a... chrome/browser/extensions/api/system_info_cpu/cpu_info_provider_linux.cc:29: int count = 0; On 2012/09/21 07:10:10, benwells wrote: > Replace int count with bool first, since you just want to know if an entry is > the first one or not. Remove it and call std::getline before the loop to skip the first line instead.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hongbo.min@intel.com/10907139/17001
Retried try job too often for step(s) interactive_ui_tests, jingle_unittests, gpu_unittests, base_unittests, sync_integration_tests, sql_unittests, content_unittests, safe_browsing_tests, ipc_tests, cacheinvalidation_unittests, remoting_unittests, browser_tests, net_unittests, media_unittests, check_deps, crypto_unittests, unit_tests, nacl_integration, content_browsertests, printing_unittests, sandbox_linux_unittests, sync_unit_tests
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hongbo.min@intel.com/10907139/17001
Change committed as 157984 |
