Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
149973002:
[chromeos/about:power] Collect cpuidle and cpufreq stats (Closed)
|
Created:
6 years, 10 months ago by Siva Chandra Modified:
6 years, 9 months ago CC:
chromium-reviews, derat+watch_chromium.org, stevenjb+watch_chromium.org, davemoore+watch_chromium.org, oshima+watch_chromium.org, nkostylev+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Description[chromeos/about:power] Collect and display cpuidle and cpufreq stats
Salient changes introduced by this CL:
1. PowerDataCollector collects CPU idle and CPU freq state
information.
2. The about:power page is organized into expandable
sections.
3. Two sections, "Idle State Data" and "Frequency State
Data" are added to the page.
4. Plots have a legend.
BUG=335816
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=256294
Patch Set 1 #Patch Set 2 : #
Total comments: 3
Patch Set 3 : #Patch Set 4 : #
Total comments: 68
Patch Set 5 : Address comments #
Total comments: 16
Patch Set 6 : #Patch Set 7 : Address comments #
Total comments: 28
Patch Set 8 : Address comments #Patch Set 9 : Rebase #Patch Set 10 : Retry upload #Patch Set 11 : Another rebase #Patch Set 12 : Fix build after rebase #
Total comments: 36
Patch Set 13 : Address comments #Patch Set 14 : #
Total comments: 12
Patch Set 15 : Address nits #Patch Set 16 : Accomodate tests #
Total comments: 1
Patch Set 17 : #Patch Set 18 : read sysfs on blocking pool via a non-member function #Messages
Total messages: 41 (0 generated)
Since the suspend time adjustment CL has not landed, I have not tried to move
the chromeos/power to c/b/chromeos/power. In the meanwhile, I am trying to work
on collecting cpuidle and cpufreq stats. This CL is towards that. However, it is
incomplete in many ways. I am sending it out early so that I can get early
feedback on the approach and the new concepts that I am using.
You will notice a new class CpuDataCollector. The power data collector will hold
an instance of this and provide an accessor method like this:
const CpuDataCollector& cpu_data_collector();
From power_ui.cc, we could do this:
cpu_data_collector =
power_data_collector->cpu_data_collector();
cpu_data_collector.Lock();
// Read CPU data and post it to power.js.
cpu_data_collector.Unlock();
https://chromiumcodereview.appspot.com/149973002/diff/20001/chrome/browser/ch... File chrome/browser/chromeos/power/cpu_data_collector.cc (right): https://chromiumcodereview.appspot.com/149973002/diff/20001/chrome/browser/ch... chrome/browser/chromeos/power/cpu_data_collector.cc:24: time_.Stop(); is this supposed to be timer_? you don't need to stop it; it'll stop automatically when it's destroyed https://chromiumcodereview.appspot.com/149973002/diff/20001/chrome/browser/ch... chrome/browser/chromeos/power/cpu_data_collector.cc:38: content::BrowserThread::PostBlockingPoolSequencedTask( use PostBlockingPoolTaskAndReply() instead, like i think i recommended earlier. you don't need sequencing semantics here, since all of your tasks are identical. and you should use the AndReply variant so the data will be posted back to the ui thread, eliminating the need for synchronization. // first, add a WeakPtrFactory member // then, do this here: IdleSample* idle = new IdleSample; FreqSample* freq = new FreqSample; content::BrowserThread::PostBlockingPoolTaskAndReply( FROM_HERE, base::Bind(&CpuDataCollector::ReadDataOnBlockingPool, weak_factory_.GetWeakPtr(), idle, freq), base::Bind(&CpuDataCollector::SaveData, weak_factory_.GetWeakPtr(), base::Owned(idle), base::Owned(freq)); void CpuDataCollector::ReadDataOnBlockingPool(IdleSample* idle, FreqSample* freq) { DCHECK(!BrowserThread::CurrentlyOn(BrowserThread::UI)); // read from sysfs and update |*idle| and |*freq| } void CpuDataCollector::SaveData(IdleSample* idle, FreqSample* freq) { DCHECK(content::BrowserThread::CurrentlyOn(BrowserThread::UI)); cpu_idle_state_data_.push_back(*idle); cpu_freq_state_data_.push_back(*freq); } https://chromiumcodereview.appspot.com/149973002/diff/20001/chrome/browser/ch... File chrome/browser/chromeos/power/cpu_data_collector.h (right): https://chromiumcodereview.appspot.com/149973002/diff/20001/chrome/browser/ch... chrome/browser/chromeos/power/cpu_data_collector.h:32: // Returns a vector whose elements correspond to a deque of idle state nit: move these descriptions down to the point where the members are defined -- accessors usually don't have any comments in chrome
Please review patch set 4. Added snanda@ so that he can verify my formulas in power.js to calculate state occupancy percenatges. I will add arv@ after the C++ parts are reviewed.
On 2014/02/15 02:03:04, Siva Chandra wrote: > Please review patch set 4. > > Added snanda@ so that he can verify my formulas in power.js to calculate state > occupancy percenatges. I will add arv@ after the C++ parts are reviewed. The screenshot of the new UI: http://tinypic.com/r/2d003dy/8
i just looked at the c++ code https://chromiumcodereview.appspot.com/149973002/diff/70001/chrome/app/genera... File chrome/app/generated_resources.grd (right): https://chromiumcodereview.appspot.com/149973002/diff/70001/chrome/app/genera... chrome/app/generated_resources.grd:14725: + Hide the opposite of "expand" should be "collapse" (alternately, use "show" and "hide") https://chromiumcodereview.appspot.com/149973002/diff/70001/chrome/browser/ch... File chrome/browser/chromeos/power/cpu_data_collector.cc (right): https://chromiumcodereview.appspot.com/149973002/diff/70001/chrome/browser/ch... chrome/browser/chromeos/power/cpu_data_collector.cc:22: const int kCpuOnlineStatus = 1; add a comment describing what this corresponds to (is it a value of a particular file in sysfs?) https://chromiumcodereview.appspot.com/149973002/diff/70001/chrome/browser/ch... chrome/browser/chromeos/power/cpu_data_collector.cc:24: #define CHROMEOS_CPU_DATA_COLLECTOR_CPU_DATA_PATH_BASE "/sys/devices/system/cpu" why is this a macro instead of a const char[]? https://chromiumcodereview.appspot.com/149973002/diff/70001/chrome/browser/ch... chrome/browser/chromeos/power/cpu_data_collector.cc:27: const char kPossibleCpuPath[] = CHROMEOS_CPU_DATA_COLLECTOR_CPU_DATA_PATH_BASE if the answer to the previous question is "so it can be concatenated with literals", you should just make these be suffixes instead, e.g. const char kPossibleCpuPathSuffix[] = "/possible"; https://chromiumcodereview.appspot.com/149973002/diff/70001/chrome/browser/ch... chrome/browser/chromeos/power/cpu_data_collector.cc:33: CHROMEOS_CPU_DATA_COLLECTOR_CPU_DATA_PATH_BASE same here https://chromiumcodereview.appspot.com/149973002/diff/70001/chrome/browser/ch... chrome/browser/chromeos/power/cpu_data_collector.cc:73: base::ReadFileToString(base::FilePath(cpu_online_file), &cpu_online_string); check the return value of reads https://chromiumcodereview.appspot.com/149973002/diff/70001/chrome/browser/ch... chrome/browser/chromeos/power/cpu_data_collector.cc:75: if (online == kCpuOnlineStatus) { just do "return online == kCpuOnlineStatus" https://chromiumcodereview.appspot.com/149973002/diff/70001/chrome/browser/ch... chrome/browser/chromeos/power/cpu_data_collector.cc:86: const int cpu_count, don't need const for pass-by-value https://chromiumcodereview.appspot.com/149973002/diff/70001/chrome/browser/ch... chrome/browser/chromeos/power/cpu_data_collector.cc:88: base::Time startTime = base::Time::Now(); start_time https://chromiumcodereview.appspot.com/149973002/diff/70001/chrome/browser/ch... chrome/browser/chromeos/power/cpu_data_collector.cc:97: int k = 0; give this variable a meaningful name https://chromiumcodereview.appspot.com/149973002/diff/70001/chrome/browser/ch... chrome/browser/chromeos/power/cpu_data_collector.cc:110: base::ReadFileToString(base::FilePath(name_file_path), &state_name); check return values https://chromiumcodereview.appspot.com/149973002/diff/70001/chrome/browser/ch... chrome/browser/chromeos/power/cpu_data_collector.cc:114: base::StringToInt64(occupancy_time_string, &occupancy_time); check return value https://chromiumcodereview.appspot.com/149973002/diff/70001/chrome/browser/ch... chrome/browser/chromeos/power/cpu_data_collector.cc:116: idle_sample.state_occupancy[state_name] = occupancy_time/1000; it'd probably be better to keep a separate map from from int id to state name, and key this by id. otherwise, a bunch of duplicate strings are getting stored here. https://chromiumcodereview.appspot.com/149973002/diff/70001/chrome/browser/ch... chrome/browser/chromeos/power/cpu_data_collector.cc:130: SampleCpuIdleData(cpu_count, idle_samples); i'd strongly prefer you didn't use recursion here -- if something consistently goes wrong, you could loop forever. what's wrong with just skipping the sample if too much time passed? https://chromiumcodereview.appspot.com/149973002/diff/70001/chrome/browser/ch... chrome/browser/chromeos/power/cpu_data_collector.cc:140: base::Time startTime = base::Time::Now(); start_time https://chromiumcodereview.appspot.com/149973002/diff/70001/chrome/browser/ch... chrome/browser/chromeos/power/cpu_data_collector.cc:152: DCHECK(!base::PathExists(base::FilePath(time_in_state_path))); i don't understand. why are you asserting that a file doesn't exist just before you try to read it? https://chromiumcodereview.appspot.com/149973002/diff/70001/chrome/browser/ch... chrome/browser/chromeos/power/cpu_data_collector.cc:159: base::ReadFileToString(base::FilePath(time_in_state_path), check return value https://chromiumcodereview.appspot.com/149973002/diff/70001/chrome/browser/ch... chrome/browser/chromeos/power/cpu_data_collector.cc:167: unsigned int state_count = lines.size(); s/unsigned int/size_t/ https://chromiumcodereview.appspot.com/149973002/diff/70001/chrome/browser/ch... chrome/browser/chromeos/power/cpu_data_collector.cc:168: if (state_count > 0 && lines.back().empty()) { omit curly brackets here https://chromiumcodereview.appspot.com/149973002/diff/70001/chrome/browser/ch... chrome/browser/chromeos/power/cpu_data_collector.cc:174: base::SplitString(lines[k], ' ', &pair); check that you got two parts https://chromiumcodereview.appspot.com/149973002/diff/70001/chrome/browser/ch... chrome/browser/chromeos/power/cpu_data_collector.cc:179: base::StringToInt64(pair[1], &occupancy_time); check return values https://chromiumcodereview.appspot.com/149973002/diff/70001/chrome/browser/ch... chrome/browser/chromeos/power/cpu_data_collector.cc:182: freq_sample.state_occupancy[base::IntToString(freq_in_khz / 1000)] = why do you need to convert this to a string? keeping it as an int would be more efficient in terms of storage space. https://chromiumcodereview.appspot.com/149973002/diff/70001/chrome/browser/ch... chrome/browser/chromeos/power/cpu_data_collector.cc:194: SampleCpuFreqData(cpu_count, freq_samples); same comment about recursion here https://chromiumcodereview.appspot.com/149973002/diff/70001/chrome/browser/ch... chrome/browser/chromeos/power/cpu_data_collector.cc:201: DCHECK(base::PathExists(base::FilePath(kPossibleCpuPath))); it'd be better to just log an error here and set cpu_count_ to 0 instead of crashing https://chromiumcodereview.appspot.com/149973002/diff/70001/chrome/browser/ch... chrome/browser/chromeos/power/cpu_data_collector.cc:204: if (possible_string.find("-") != std::string::npos) { also check that possible_string is long enough so you don't crash later if you index beyond the end https://chromiumcodereview.appspot.com/149973002/diff/70001/chrome/browser/ch... chrome/browser/chromeos/power/cpu_data_collector.cc:208: base::StringToInt(possible_string.substr(2), &max_cpu); check return value https://chromiumcodereview.appspot.com/149973002/diff/70001/chrome/browser/ch... chrome/browser/chromeos/power/cpu_data_collector.cc:213: for (int i = 0; i < cpu_count_; ++i) { just do: cpu_idle_state_data_.resize(cpu_count_); etc. https://chromiumcodereview.appspot.com/149973002/diff/70001/chrome/browser/ch... chrome/browser/chromeos/power/cpu_data_collector.cc:257: DCHECK(idle_samples->size() == cpu_idle_state_data_.size()); nit: DCHECK_EQ https://chromiumcodereview.appspot.com/149973002/diff/70001/chrome/browser/ch... chrome/browser/chromeos/power/cpu_data_collector.cc:258: for (unsigned int i = 0; i < cpu_idle_state_data_.size(); ++i) { nit: omit curly brackets https://chromiumcodereview.appspot.com/149973002/diff/70001/chrome/browser/ch... chrome/browser/chromeos/power/cpu_data_collector.cc:259: AddSample(&cpu_idle_state_data_[i], (*idle_samples)[i]); does this compile? i don't see where AddSample is declared or defined https://chromiumcodereview.appspot.com/149973002/diff/70001/chrome/browser/ch... chrome/browser/chromeos/power/cpu_data_collector.cc:262: DCHECK(freq_samples->size() == cpu_freq_state_data_.size()); nit: DCHECK_EQ https://chromiumcodereview.appspot.com/149973002/diff/70001/chrome/browser/ch... chrome/browser/chromeos/power/cpu_data_collector.cc:263: for (unsigned int i = 0; i < cpu_freq_state_data_.size(); ++i) { nit: omit curly brackets https://chromiumcodereview.appspot.com/149973002/diff/70001/chrome/browser/ui... File chrome/browser/ui/webui/chromeos/power_ui.cc (right): https://chromiumcodereview.appspot.com/149973002/diff/70001/chrome/browser/ui... chrome/browser/ui/webui/chromeos/power_ui.cc:167: for (; it != sample.state_occupancy.end(); ++it) { nit: omit curly brackets
https://chromiumcodereview.appspot.com/149973002/diff/70001/chrome/app/genera... File chrome/app/generated_resources.grd (right): https://chromiumcodereview.appspot.com/149973002/diff/70001/chrome/app/genera... chrome/app/generated_resources.grd:14725: + Hide On 2014/02/15 15:31:25, Daniel Erat wrote: > the opposite of "expand" should be "collapse" > > (alternately, use "show" and "hide") Done. https://chromiumcodereview.appspot.com/149973002/diff/70001/chrome/browser/ch... File chrome/browser/chromeos/power/cpu_data_collector.cc (right): https://chromiumcodereview.appspot.com/149973002/diff/70001/chrome/browser/ch... chrome/browser/chromeos/power/cpu_data_collector.cc:22: const int kCpuOnlineStatus = 1; On 2014/02/15 15:31:25, Daniel Erat wrote: > add a comment describing what this corresponds to (is it a value of a particular > file in sysfs?) Done. https://chromiumcodereview.appspot.com/149973002/diff/70001/chrome/browser/ch... chrome/browser/chromeos/power/cpu_data_collector.cc:24: #define CHROMEOS_CPU_DATA_COLLECTOR_CPU_DATA_PATH_BASE "/sys/devices/system/cpu" On 2014/02/15 15:31:25, Daniel Erat wrote: > why is this a macro instead of a const char[]? Done. https://chromiumcodereview.appspot.com/149973002/diff/70001/chrome/browser/ch... chrome/browser/chromeos/power/cpu_data_collector.cc:27: const char kPossibleCpuPath[] = CHROMEOS_CPU_DATA_COLLECTOR_CPU_DATA_PATH_BASE On 2014/02/15 15:31:25, Daniel Erat wrote: > if the answer to the previous question is "so it can be concatenated with > literals", you should just make these be suffixes instead, e.g. > > const char kPossibleCpuPathSuffix[] = "/possible"; Done. https://chromiumcodereview.appspot.com/149973002/diff/70001/chrome/browser/ch... chrome/browser/chromeos/power/cpu_data_collector.cc:33: CHROMEOS_CPU_DATA_COLLECTOR_CPU_DATA_PATH_BASE On 2014/02/15 15:31:25, Daniel Erat wrote: > same here Done. https://chromiumcodereview.appspot.com/149973002/diff/70001/chrome/browser/ch... chrome/browser/chromeos/power/cpu_data_collector.cc:73: base::ReadFileToString(base::FilePath(cpu_online_file), &cpu_online_string); On 2014/02/15 15:31:25, Daniel Erat wrote: > check the return value of reads Done. https://chromiumcodereview.appspot.com/149973002/diff/70001/chrome/browser/ch... chrome/browser/chromeos/power/cpu_data_collector.cc:75: if (online == kCpuOnlineStatus) { On 2014/02/15 15:31:25, Daniel Erat wrote: > just do "return online == kCpuOnlineStatus" Done. https://chromiumcodereview.appspot.com/149973002/diff/70001/chrome/browser/ch... chrome/browser/chromeos/power/cpu_data_collector.cc:86: const int cpu_count, On 2014/02/15 15:31:25, Daniel Erat wrote: > don't need const for pass-by-value Done. https://chromiumcodereview.appspot.com/149973002/diff/70001/chrome/browser/ch... chrome/browser/chromeos/power/cpu_data_collector.cc:88: base::Time startTime = base::Time::Now(); On 2014/02/15 15:31:25, Daniel Erat wrote: > start_time Done. https://chromiumcodereview.appspot.com/149973002/diff/70001/chrome/browser/ch... chrome/browser/chromeos/power/cpu_data_collector.cc:97: int k = 0; On 2014/02/15 15:31:25, Daniel Erat wrote: > give this variable a meaningful name Done. https://chromiumcodereview.appspot.com/149973002/diff/70001/chrome/browser/ch... chrome/browser/chromeos/power/cpu_data_collector.cc:110: base::ReadFileToString(base::FilePath(name_file_path), &state_name); On 2014/02/15 15:31:25, Daniel Erat wrote: > check return values Done. https://chromiumcodereview.appspot.com/149973002/diff/70001/chrome/browser/ch... chrome/browser/chromeos/power/cpu_data_collector.cc:114: base::StringToInt64(occupancy_time_string, &occupancy_time); On 2014/02/15 15:31:25, Daniel Erat wrote: > check return value Done. https://chromiumcodereview.appspot.com/149973002/diff/70001/chrome/browser/ch... chrome/browser/chromeos/power/cpu_data_collector.cc:116: idle_sample.state_occupancy[state_name] = occupancy_time/1000; On 2014/02/15 15:31:25, Daniel Erat wrote: > it'd probably be better to keep a separate map from from int id to state name, > and key this by id. otherwise, a bunch of duplicate strings are getting stored > here. Which duplicate strings are you referring to? Also, I am not sure if all CPUs will have exactly similar states on different platforms. https://chromiumcodereview.appspot.com/149973002/diff/70001/chrome/browser/ch... chrome/browser/chromeos/power/cpu_data_collector.cc:130: SampleCpuIdleData(cpu_count, idle_samples); On 2014/02/15 15:31:25, Daniel Erat wrote: > i'd strongly prefer you didn't use recursion here -- if something consistently > goes wrong, you could loop forever. what's wrong with just skipping the sample > if too much time passed? Removed recursion now. Just discarding the sample. https://chromiumcodereview.appspot.com/149973002/diff/70001/chrome/browser/ch... chrome/browser/chromeos/power/cpu_data_collector.cc:140: base::Time startTime = base::Time::Now(); On 2014/02/15 15:31:25, Daniel Erat wrote: > start_time Done. https://chromiumcodereview.appspot.com/149973002/diff/70001/chrome/browser/ch... chrome/browser/chromeos/power/cpu_data_collector.cc:152: DCHECK(!base::PathExists(base::FilePath(time_in_state_path))); On 2014/02/15 15:31:25, Daniel Erat wrote: > i don't understand. why are you asserting that a file doesn't exist just before > you try to read it? I am lost too. Having '!' here is an error, but I don't understand why this has been working at all! https://chromiumcodereview.appspot.com/149973002/diff/70001/chrome/browser/ch... chrome/browser/chromeos/power/cpu_data_collector.cc:159: base::ReadFileToString(base::FilePath(time_in_state_path), On 2014/02/15 15:31:25, Daniel Erat wrote: > check return value Done. https://chromiumcodereview.appspot.com/149973002/diff/70001/chrome/browser/ch... chrome/browser/chromeos/power/cpu_data_collector.cc:167: unsigned int state_count = lines.size(); On 2014/02/15 15:31:25, Daniel Erat wrote: > s/unsigned int/size_t/ Done. https://chromiumcodereview.appspot.com/149973002/diff/70001/chrome/browser/ch... chrome/browser/chromeos/power/cpu_data_collector.cc:168: if (state_count > 0 && lines.back().empty()) { On 2014/02/15 15:31:25, Daniel Erat wrote: > omit curly brackets here Done. https://chromiumcodereview.appspot.com/149973002/diff/70001/chrome/browser/ch... chrome/browser/chromeos/power/cpu_data_collector.cc:174: base::SplitString(lines[k], ' ', &pair); On 2014/02/15 15:31:25, Daniel Erat wrote: > check that you got two parts Done. https://chromiumcodereview.appspot.com/149973002/diff/70001/chrome/browser/ch... chrome/browser/chromeos/power/cpu_data_collector.cc:179: base::StringToInt64(pair[1], &occupancy_time); On 2014/02/15 15:31:25, Daniel Erat wrote: > check return values Done. https://chromiumcodereview.appspot.com/149973002/diff/70001/chrome/browser/ch... chrome/browser/chromeos/power/cpu_data_collector.cc:182: freq_sample.state_occupancy[base::IntToString(freq_in_khz / 1000)] = On 2014/02/15 15:31:25, Daniel Erat wrote: > why do you need to convert this to a string? keeping it as an int would be more > efficient in terms of storage space. I did have it as an int initially. I changed it to being a string as idle states are strings anyway. If we made these also strings, then we have only one sample type and the rest of the code (as in power_ui.cc) is simpler/smaller as consequence. https://chromiumcodereview.appspot.com/149973002/diff/70001/chrome/browser/ch... chrome/browser/chromeos/power/cpu_data_collector.cc:194: SampleCpuFreqData(cpu_count, freq_samples); On 2014/02/15 15:31:25, Daniel Erat wrote: > same comment about recursion here Done. https://chromiumcodereview.appspot.com/149973002/diff/70001/chrome/browser/ch... chrome/browser/chromeos/power/cpu_data_collector.cc:201: DCHECK(base::PathExists(base::FilePath(kPossibleCpuPath))); On 2014/02/15 15:31:25, Daniel Erat wrote: > it'd be better to just log an error here and set cpu_count_ to 0 instead of > crashing Done. https://chromiumcodereview.appspot.com/149973002/diff/70001/chrome/browser/ch... chrome/browser/chromeos/power/cpu_data_collector.cc:204: if (possible_string.find("-") != std::string::npos) { On 2014/02/15 15:31:25, Daniel Erat wrote: > also check that possible_string is long enough so you don't crash later if you > index beyond the end Done. https://chromiumcodereview.appspot.com/149973002/diff/70001/chrome/browser/ch... chrome/browser/chromeos/power/cpu_data_collector.cc:208: base::StringToInt(possible_string.substr(2), &max_cpu); On 2014/02/15 15:31:25, Daniel Erat wrote: > check return value Done. https://chromiumcodereview.appspot.com/149973002/diff/70001/chrome/browser/ch... chrome/browser/chromeos/power/cpu_data_collector.cc:213: for (int i = 0; i < cpu_count_; ++i) { On 2014/02/15 15:31:25, Daniel Erat wrote: > just do: > > cpu_idle_state_data_.resize(cpu_count_); > etc. Done. https://chromiumcodereview.appspot.com/149973002/diff/70001/chrome/browser/ch... chrome/browser/chromeos/power/cpu_data_collector.cc:257: DCHECK(idle_samples->size() == cpu_idle_state_data_.size()); On 2014/02/15 15:31:25, Daniel Erat wrote: > nit: DCHECK_EQ Done. https://chromiumcodereview.appspot.com/149973002/diff/70001/chrome/browser/ch... chrome/browser/chromeos/power/cpu_data_collector.cc:258: for (unsigned int i = 0; i < cpu_idle_state_data_.size(); ++i) { On 2014/02/15 15:31:25, Daniel Erat wrote: > nit: omit curly brackets Done. https://chromiumcodereview.appspot.com/149973002/diff/70001/chrome/browser/ch... chrome/browser/chromeos/power/cpu_data_collector.cc:259: AddSample(&cpu_idle_state_data_[i], (*idle_samples)[i]); On 2014/02/15 15:31:25, Daniel Erat wrote: > does this compile? i don't see where AddSample is declared or defined AddSample is a template function defined in power_data_collector.h. https://chromiumcodereview.appspot.com/149973002/diff/70001/chrome/browser/ch... chrome/browser/chromeos/power/cpu_data_collector.cc:262: DCHECK(freq_samples->size() == cpu_freq_state_data_.size()); On 2014/02/15 15:31:25, Daniel Erat wrote: > nit: DCHECK_EQ Done. https://chromiumcodereview.appspot.com/149973002/diff/70001/chrome/browser/ch... chrome/browser/chromeos/power/cpu_data_collector.cc:263: for (unsigned int i = 0; i < cpu_freq_state_data_.size(); ++i) { On 2014/02/15 15:31:25, Daniel Erat wrote: > nit: omit curly brackets Done. https://chromiumcodereview.appspot.com/149973002/diff/70001/chrome/browser/ui... File chrome/browser/ui/webui/chromeos/power_ui.cc (right): https://chromiumcodereview.appspot.com/149973002/diff/70001/chrome/browser/ui... chrome/browser/ui/webui/chromeos/power_ui.cc:167: for (; it != sample.state_occupancy.end(); ++it) { On 2014/02/15 15:31:25, Daniel Erat wrote: > nit: omit curly brackets Done.
https://chromiumcodereview.appspot.com/149973002/diff/70001/chrome/browser/ch... File chrome/browser/chromeos/power/cpu_data_collector.cc (right): https://chromiumcodereview.appspot.com/149973002/diff/70001/chrome/browser/ch... chrome/browser/chromeos/power/cpu_data_collector.cc:116: idle_sample.state_occupancy[state_name] = occupancy_time/1000; On 2014/02/19 20:02:30, Siva Chandra wrote: > On 2014/02/15 15:31:25, Daniel Erat wrote: > > it'd probably be better to keep a separate map from from int id to state name, > > and key this by id. otherwise, a bunch of duplicate strings are getting stored > > here. > > Which duplicate strings are you referring to? Also, I am not sure if all CPUs > will have exactly similar states on different platforms. i meant that each state name will be duplicated in each sample right now. you can avoid this if you do something like this instead (untested, and assuming that there are few enough states and frequencies that scanning through a vector is faster than using std::map): std::vector<std::string> state_names_; ... std::string state_name = ...; int state_index = -1; for (size_t i = 0; i < state_names_.size(); ++i) { if (state_names_[i] == state_name) { state_index = static_cast<int>(i); break; } } if (state_index < 0) { state_index = static_cast<int>(state_names_.size()); state_names_.push_back(state_name); } then make state_occupancy be keyed by int instead of string and use state_index instead of state_name when saving data to it. you can provide an accessor for state_names_ so you can pass it to the JS code. this is a bit more code than just using string keys like what you have here, but i think it's worth it to avoid duplicating these strings across many different maps. https://chromiumcodereview.appspot.com/149973002/diff/150001/chrome/browser/c... File chrome/browser/chromeos/power/cpu_data_collector.cc (right): https://chromiumcodereview.appspot.com/149973002/diff/150001/chrome/browser/c... chrome/browser/chromeos/power/cpu_data_collector.cc:36: // Format of the path to the file which contains information about a particular nit: update all of these comments to clarify that these are path suffixes https://chromiumcodereview.appspot.com/149973002/diff/150001/chrome/browser/c... chrome/browser/chromeos/power/cpu_data_collector.cc:59: std::string online_file_format = base::StringPrintf( nit: probably best to make these const https://chromiumcodereview.appspot.com/149973002/diff/150001/chrome/browser/c... chrome/browser/chromeos/power/cpu_data_collector.cc:90: for (int i = 0; i < cpu_count; ++i) { nit: s/i/cpu_index/ ? https://chromiumcodereview.appspot.com/149973002/diff/150001/chrome/browser/c... chrome/browser/chromeos/power/cpu_data_collector.cc:122: &occupancy_time_string)) { nit: invert this to !base::ReadFileToString() || !base::ReadFileToString() so you can bail out early on failure instead of needing to indent this code so much https://chromiumcodereview.appspot.com/149973002/diff/150001/chrome/browser/c... chrome/browser/chromeos/power/cpu_data_collector.cc:147: idle_state_dir_format.c_str(), i, state_count); i think you can avoid duplicating the code to generate idle_state_dir. just do something like: for (int state_index = 0; ; ++state_index) { const std::string idle_state_dir = base::StringPrintf( idle_state_dir_format.c_str(), cpu_index, state_index); if (!base::DirectoryExists(base::FilePath(idle_state_dir)) break; ... } https://chromiumcodereview.appspot.com/149973002/diff/150001/chrome/browser/c... chrome/browser/chromeos/power/cpu_data_collector.cc:158: LOG(WARNING) << "Dropped an idle state sample due to excessive time delay."; nit: include the delay in the warning https://chromiumcodereview.appspot.com/149973002/diff/150001/chrome/browser/c... chrome/browser/chromeos/power/cpu_data_collector.cc:169: for (int i = 0; i < cpu_count; ++i) { nit: s/i/cpu_index/ https://chromiumcodereview.appspot.com/149973002/diff/150001/chrome/browser/c... chrome/browser/chromeos/power/cpu_data_collector.cc:206: for (size_t k = 0; k < state_count; ++k) { nit: s/k/state_index/
ping.
On 2014/03/04 23:15:18, Siva Chandra wrote: > ping. taking a look now (don't forget to send a PTAL message whenever you want the reviewer to make another pass -- unlike gerrit, rietveld doesn't send email whenever a new patch set is uploaded)
PTaL at patch set 7. Sorry for the previous ping. As I explained offline, it is my mistake. https://chromiumcodereview.appspot.com/149973002/diff/70001/chrome/browser/ch... File chrome/browser/chromeos/power/cpu_data_collector.cc (right): https://chromiumcodereview.appspot.com/149973002/diff/70001/chrome/browser/ch... chrome/browser/chromeos/power/cpu_data_collector.cc:116: idle_sample.state_occupancy[state_name] = occupancy_time/1000; On 2014/02/21 02:29:37, Daniel Erat wrote: > On 2014/02/19 20:02:30, Siva Chandra wrote: > > On 2014/02/15 15:31:25, Daniel Erat wrote: > > > it'd probably be better to keep a separate map from from int id to state > name, > > > and key this by id. otherwise, a bunch of duplicate strings are getting > stored > > > here. > > > > Which duplicate strings are you referring to? Also, I am not sure if all CPUs > > will have exactly similar states on different platforms. > > i meant that each state name will be duplicated in each sample right now. you > can avoid this if you do something like this instead (untested, and assuming > that there are few enough states and frequencies that scanning through a vector > is faster than using std::map): > > std::vector<std::string> state_names_; > > ... > std::string state_name = ...; > int state_index = -1; > for (size_t i = 0; i < state_names_.size(); ++i) { > if (state_names_[i] == state_name) { > state_index = static_cast<int>(i); > break; > } > } > if (state_index < 0) { > state_index = static_cast<int>(state_names_.size()); > state_names_.push_back(state_name); > } > > then make state_occupancy be keyed by int instead of string and use state_index > instead of state_name when saving data to it. you can provide an accessor for > state_names_ so you can pass it to the JS code. > > this is a bit more code than just using string keys like what you have here, but > i think it's worth it to avoid duplicating these strings across many different > maps. Done. https://chromiumcodereview.appspot.com/149973002/diff/150001/chrome/browser/c... File chrome/browser/chromeos/power/cpu_data_collector.cc (right): https://chromiumcodereview.appspot.com/149973002/diff/150001/chrome/browser/c... chrome/browser/chromeos/power/cpu_data_collector.cc:36: // Format of the path to the file which contains information about a particular On 2014/02/21 02:29:37, Daniel Erat wrote: > nit: update all of these comments to clarify that these are path suffixes Done. https://chromiumcodereview.appspot.com/149973002/diff/150001/chrome/browser/c... chrome/browser/chromeos/power/cpu_data_collector.cc:59: std::string online_file_format = base::StringPrintf( On 2014/02/21 02:29:37, Daniel Erat wrote: > nit: probably best to make these const Done. https://chromiumcodereview.appspot.com/149973002/diff/150001/chrome/browser/c... chrome/browser/chromeos/power/cpu_data_collector.cc:90: for (int i = 0; i < cpu_count; ++i) { On 2014/02/21 02:29:37, Daniel Erat wrote: > nit: s/i/cpu_index/ ? Done. https://chromiumcodereview.appspot.com/149973002/diff/150001/chrome/browser/c... chrome/browser/chromeos/power/cpu_data_collector.cc:122: &occupancy_time_string)) { On 2014/02/21 02:29:37, Daniel Erat wrote: > nit: invert this to !base::ReadFileToString() || !base::ReadFileToString() so > you can bail out early on failure instead of needing to indent this code so much Done. https://chromiumcodereview.appspot.com/149973002/diff/150001/chrome/browser/c... chrome/browser/chromeos/power/cpu_data_collector.cc:147: idle_state_dir_format.c_str(), i, state_count); On 2014/02/21 02:29:37, Daniel Erat wrote: > i think you can avoid duplicating the code to generate idle_state_dir. just do > something like: > > for (int state_index = 0; ; ++state_index) { > const std::string idle_state_dir = base::StringPrintf( > idle_state_dir_format.c_str(), cpu_index, state_index); > if (!base::DirectoryExists(base::FilePath(idle_state_dir)) > break; > > ... > } Done. https://chromiumcodereview.appspot.com/149973002/diff/150001/chrome/browser/c... chrome/browser/chromeos/power/cpu_data_collector.cc:158: LOG(WARNING) << "Dropped an idle state sample due to excessive time delay."; On 2014/02/21 02:29:37, Daniel Erat wrote: > nit: include the delay in the warning Done. https://chromiumcodereview.appspot.com/149973002/diff/150001/chrome/browser/c... chrome/browser/chromeos/power/cpu_data_collector.cc:169: for (int i = 0; i < cpu_count; ++i) { On 2014/02/21 02:29:37, Daniel Erat wrote: > nit: s/i/cpu_index/ Done. https://chromiumcodereview.appspot.com/149973002/diff/150001/chrome/browser/c... chrome/browser/chromeos/power/cpu_data_collector.cc:206: for (size_t k = 0; k < state_count; ++k) { On 2014/02/21 02:29:37, Daniel Erat wrote: > nit: s/k/state_index/ Done.
https://chromiumcodereview.appspot.com/149973002/diff/420001/chrome/browser/c... File chrome/browser/chromeos/power/cpu_data_collector.cc (right): https://chromiumcodereview.appspot.com/149973002/diff/420001/chrome/browser/c... chrome/browser/chromeos/power/cpu_data_collector.cc:77: if (!base::PathExists(base::FilePath(cpu_online_file))) { is it worth checking that the cpu directory at least exists? otherwise, i think that this may return true if asked if e.g. the 99th cpu is online. https://chromiumcodereview.appspot.com/149973002/diff/420001/chrome/browser/c... chrome/browser/chromeos/power/cpu_data_collector.cc:87: base::TrimString(cpu_online_string, " \n", &cpu_online_string); i think you can just use: base::TrimWhitespace(cpu_online_string, base::TRIM_ALL, &cpu_online_string); here https://chromiumcodereview.appspot.com/149973002/diff/420001/chrome/browser/c... chrome/browser/chromeos/power/cpu_data_collector.cc:149: base::TrimString(state_name, " \n", &state_name); base::TrimWhitespace() https://chromiumcodereview.appspot.com/149973002/diff/420001/chrome/browser/c... chrome/browser/chromeos/power/cpu_data_collector.cc:151: occupancy_time_string, " \n", &occupancy_time_string); base::TrimWhitespace() https://chromiumcodereview.appspot.com/149973002/diff/420001/chrome/browser/c... chrome/browser/chromeos/power/cpu_data_collector.cc:154: int64 time_in_state = occupancy_time/1000; nit: add spaces to either side of '/' https://chromiumcodereview.appspot.com/149973002/diff/420001/chrome/browser/c... chrome/browser/chromeos/power/cpu_data_collector.cc:173: if (delay > 500) { nit: move 500 to a constant at the top of the file https://chromiumcodereview.appspot.com/149973002/diff/420001/chrome/browser/c... chrome/browser/chromeos/power/cpu_data_collector.cc:233: base::TrimString(pair[s], " \n", &pair[s]); base::TrimWhitespace() https://chromiumcodereview.appspot.com/149973002/diff/420001/chrome/browser/c... chrome/browser/chromeos/power/cpu_data_collector.cc:256: if (delay > 500) { use a constant here too https://chromiumcodereview.appspot.com/149973002/diff/420001/chrome/browser/c... chrome/browser/chromeos/power/cpu_data_collector.cc:268: if (!base::PathExists(base::FilePath(possible_cpu_path))) { you probably shouldn't be doing this file access from the UI thread. while it's a bit gross, maybe you should re-read this from the blocking pool every time you collect data https://chromiumcodereview.appspot.com/149973002/diff/420001/chrome/browser/c... chrome/browser/chromeos/power/cpu_data_collector.cc:327: SampleCpuIdleData(cpu_count_, &cpu_idle_state_names_, idle_samples); i think that you're now accessing *_state_names_ from multiple threads (reading/writing to it from the blocking pool and reading it from the UI thread). you should probably generate new copies on the blocking pool and then merge them back in SaveCpuStateSamplesOnUIThread() https://chromiumcodereview.appspot.com/149973002/diff/420001/chrome/browser/c... File chrome/browser/chromeos/power/cpu_data_collector.h (right): https://chromiumcodereview.appspot.com/149973002/diff/420001/chrome/browser/c... chrome/browser/chromeos/power/cpu_data_collector.h:24: // occupancy information using suspend time data got from nit: delete 'got' https://chromiumcodereview.appspot.com/149973002/diff/420001/chrome/browser/c... chrome/browser/chromeos/power/cpu_data_collector.h:42: std::map<int, int64> time_in_state; assuming that there aren't a bunch of states and this map isn't sparse, it'd probably be much more space-efficient to use an std::vector<int64> here, where the i-th entry corresponds to the i-th value in *_state_names_ https://chromiumcodereview.appspot.com/149973002/diff/420001/chrome/browser/c... chrome/browser/chromeos/power/cpu_data_collector.h:70: // Samples CPU idle and CPU freq data from sysfs. This function should run on nit: delete extra space before 'run' https://chromiumcodereview.appspot.com/149973002/diff/420001/chrome/browser/u... File chrome/browser/ui/webui/chromeos/power_ui.cc (right): https://chromiumcodereview.appspot.com/149973002/diff/420001/chrome/browser/u... chrome/browser/ui/webui/chromeos/power_ui.cc:142: DCHECK(data != NULL); nit: just do DCHECK(data)
Addressed all comments. PTaL at patch set 8. https://chromiumcodereview.appspot.com/149973002/diff/420001/chrome/browser/c... File chrome/browser/chromeos/power/cpu_data_collector.cc (right): https://chromiumcodereview.appspot.com/149973002/diff/420001/chrome/browser/c... chrome/browser/chromeos/power/cpu_data_collector.cc:77: if (!base::PathExists(base::FilePath(cpu_online_file))) { On 2014/03/06 00:46:40, Daniel Erat wrote: > is it worth checking that the cpu directory at least exists? otherwise, i think > that this may return true if asked if e.g. the 99th cpu is online. This function is called only if sysfs reports that such a CPU is possible. https://chromiumcodereview.appspot.com/149973002/diff/420001/chrome/browser/c... chrome/browser/chromeos/power/cpu_data_collector.cc:87: base::TrimString(cpu_online_string, " \n", &cpu_online_string); On 2014/03/06 00:46:40, Daniel Erat wrote: > i think you can just use: > > base::TrimWhitespace(cpu_online_string, > base::TRIM_ALL, > &cpu_online_string); > > here Done. https://chromiumcodereview.appspot.com/149973002/diff/420001/chrome/browser/c... chrome/browser/chromeos/power/cpu_data_collector.cc:149: base::TrimString(state_name, " \n", &state_name); On 2014/03/06 00:46:40, Daniel Erat wrote: > base::TrimWhitespace() Done. https://chromiumcodereview.appspot.com/149973002/diff/420001/chrome/browser/c... chrome/browser/chromeos/power/cpu_data_collector.cc:151: occupancy_time_string, " \n", &occupancy_time_string); On 2014/03/06 00:46:40, Daniel Erat wrote: > base::TrimWhitespace() Done. https://chromiumcodereview.appspot.com/149973002/diff/420001/chrome/browser/c... chrome/browser/chromeos/power/cpu_data_collector.cc:154: int64 time_in_state = occupancy_time/1000; On 2014/03/06 00:46:40, Daniel Erat wrote: > nit: add spaces to either side of '/' Done. https://chromiumcodereview.appspot.com/149973002/diff/420001/chrome/browser/c... chrome/browser/chromeos/power/cpu_data_collector.cc:173: if (delay > 500) { On 2014/03/06 00:46:40, Daniel Erat wrote: > nit: move 500 to a constant at the top of the file Done. https://chromiumcodereview.appspot.com/149973002/diff/420001/chrome/browser/c... chrome/browser/chromeos/power/cpu_data_collector.cc:233: base::TrimString(pair[s], " \n", &pair[s]); On 2014/03/06 00:46:40, Daniel Erat wrote: > base::TrimWhitespace() Done. https://chromiumcodereview.appspot.com/149973002/diff/420001/chrome/browser/c... chrome/browser/chromeos/power/cpu_data_collector.cc:256: if (delay > 500) { On 2014/03/06 00:46:40, Daniel Erat wrote: > use a constant here too Done. https://chromiumcodereview.appspot.com/149973002/diff/420001/chrome/browser/c... chrome/browser/chromeos/power/cpu_data_collector.cc:268: if (!base::PathExists(base::FilePath(possible_cpu_path))) { On 2014/03/06 00:46:40, Daniel Erat wrote: > you probably shouldn't be doing this file access from the UI thread. while it's > a bit gross, maybe you should re-read this from the blocking pool every time you > collect data Moved to SampleCpuStateOnBlockingPool. https://chromiumcodereview.appspot.com/149973002/diff/420001/chrome/browser/c... chrome/browser/chromeos/power/cpu_data_collector.cc:327: SampleCpuIdleData(cpu_count_, &cpu_idle_state_names_, idle_samples); On 2014/03/06 00:46:40, Daniel Erat wrote: > i think that you're now accessing *_state_names_ from multiple threads > (reading/writing to it from the blocking pool and reading it from the UI > thread). you should probably generate new copies on the blocking pool and then > merge them back in SaveCpuStateSamplesOnUIThread() Done. https://chromiumcodereview.appspot.com/149973002/diff/420001/chrome/browser/c... File chrome/browser/chromeos/power/cpu_data_collector.h (right): https://chromiumcodereview.appspot.com/149973002/diff/420001/chrome/browser/c... chrome/browser/chromeos/power/cpu_data_collector.h:24: // occupancy information using suspend time data got from On 2014/03/06 00:46:40, Daniel Erat wrote: > nit: delete 'got' Done. https://chromiumcodereview.appspot.com/149973002/diff/420001/chrome/browser/c... chrome/browser/chromeos/power/cpu_data_collector.h:42: std::map<int, int64> time_in_state; On 2014/03/06 00:46:40, Daniel Erat wrote: > assuming that there aren't a bunch of states and this map isn't sparse, it'd > probably be much more space-efficient to use an std::vector<int64> here, where > the i-th entry corresponds to the i-th value in *_state_names_ Done. https://chromiumcodereview.appspot.com/149973002/diff/420001/chrome/browser/c... chrome/browser/chromeos/power/cpu_data_collector.h:70: // Samples CPU idle and CPU freq data from sysfs. This function should run on On 2014/03/06 00:46:40, Daniel Erat wrote: > nit: delete extra space before 'run' Done. https://chromiumcodereview.appspot.com/149973002/diff/420001/chrome/browser/u... File chrome/browser/ui/webui/chromeos/power_ui.cc (right): https://chromiumcodereview.appspot.com/149973002/diff/420001/chrome/browser/u... chrome/browser/ui/webui/chromeos/power_ui.cc:142: DCHECK(data != NULL); On 2014/03/06 00:46:40, Daniel Erat wrote: > nit: just do DCHECK(data) Done.
thanks! this generally looks okay to me now; just a few more comments https://chromiumcodereview.appspot.com/149973002/diff/520001/chrome/app/chrom... File chrome/app/chromeos_strings.grdp (right): https://chromiumcodereview.appspot.com/149973002/diff/520001/chrome/app/chrom... chrome/app/chromeos_strings.grdp:5036: <message name="IDS_ABOUT_POWER_SHOW_BUTTON" desc="Text on the 'Show' button"> nit: the descriptions for these new strings (and probably all of the others, too) should probably mention that they're for the about:power page; otherwise i could imagine translators not having enough context https://chromiumcodereview.appspot.com/149973002/diff/520001/chrome/browser/c... File chrome/browser/chromeos/power/cpu_data_collector.cc (right): https://chromiumcodereview.appspot.com/149973002/diff/520001/chrome/browser/c... chrome/browser/chromeos/power/cpu_data_collector.cc:23: const int kSamplingDurationLimitMillisec = 500; nit: s/Millisec/Ms/ (abbreviating "milliseconds" just halfway seems a bit weird) :-) https://chromiumcodereview.appspot.com/149973002/diff/520001/chrome/browser/c... chrome/browser/chromeos/power/cpu_data_collector.cc:139: int64 occupancy_time; nit: rename to occupancy_time_usec https://chromiumcodereview.appspot.com/149973002/diff/520001/chrome/browser/c... chrome/browser/chromeos/power/cpu_data_collector.cc:141: &state_name) || nit: fix indenting (should be indented one more space) https://chromiumcodereview.appspot.com/149973002/diff/520001/chrome/browser/c... chrome/browser/chromeos/power/cpu_data_collector.cc:143: &occupancy_time_string)) { nit: here too https://chromiumcodereview.appspot.com/149973002/diff/520001/chrome/browser/c... chrome/browser/chromeos/power/cpu_data_collector.cc:158: int64 time_in_state = occupancy_time / 1000; nit: rename to time_in_state_ms https://chromiumcodereview.appspot.com/149973002/diff/520001/chrome/browser/c... chrome/browser/chromeos/power/cpu_data_collector.cc:234: int64 occupancy_time; nit: rename this to include the units https://chromiumcodereview.appspot.com/149973002/diff/520001/chrome/browser/c... chrome/browser/chromeos/power/cpu_data_collector.cc:247: freq_sample.time_in_state[index] = occupancy_time * 10; multiplying by 10 seems a bit strange -- is this correct? https://chromiumcodereview.appspot.com/149973002/diff/520001/chrome/browser/c... chrome/browser/chromeos/power/cpu_data_collector.cc:286: std::vector<std::string>* cpu_idle_state_names = new std::vector<std::string>; these should probably initialized with the existing vectors, like "std::vector<std::string>(cpu_idle_state_names_)", right? it'd be bad if the order changed... https://chromiumcodereview.appspot.com/149973002/diff/520001/chrome/browser/c... chrome/browser/chromeos/power/cpu_data_collector.cc:289: std::vector<std::string>* cpu_freq_state_names = new std::vector<std::string>; same here https://chromiumcodereview.appspot.com/149973002/diff/520001/chrome/browser/c... chrome/browser/chromeos/power/cpu_data_collector.cc:317: // later. A system will atleast have one CPU. Hence, a value of 1 here nit: s/atleast/at least/ https://chromiumcodereview.appspot.com/149973002/diff/520001/chrome/browser/c... chrome/browser/chromeos/power/cpu_data_collector.cc:349: cpu_idle_state_data_.resize(cpu_count_); this seems wrong -- what happens if the ui thread accesses these while they're being resized? at the very least, i think that you'd only want to call this in the block where you initialize cpu_count_ above, but it'd probably be safest to pass cpu_count_ to the blocking thread as well and then pass it back to the ui thread, and only resize these on the ui thread (i.e. don't let the blocking thread access any members of the class -- you could even move SampleCpuStateOnBlockingPool() to the anonymous namespace instead of keeping it as a instance method to make sure that there's no risk of races between the two threads) https://chromiumcodereview.appspot.com/149973002/diff/520001/chrome/browser/c... chrome/browser/chromeos/power/cpu_data_collector.cc:366: if (idle_samples->size() > 0) { nit: !idle_samples->empty() https://chromiumcodereview.appspot.com/149973002/diff/520001/chrome/browser/c... chrome/browser/chromeos/power/cpu_data_collector.cc:374: if (freq_samples->size() > 0) { nit: !freq_samples->empty() https://chromiumcodereview.appspot.com/149973002/diff/520001/chrome/browser/c... File chrome/browser/chromeos/power/cpu_data_collector.h (right): https://chromiumcodereview.appspot.com/149973002/diff/520001/chrome/browser/c... chrome/browser/chromeos/power/cpu_data_collector.h:29: StateOccupancySample(); also declare a d'tor and define it in the .cc file; see http://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Stop-in.... https://chromiumcodereview.appspot.com/149973002/diff/520001/chrome/browser/c... chrome/browser/chromeos/power/cpu_data_collector.h:63: CpuDataCollector(); declare a d'tor for this too https://chromiumcodereview.appspot.com/149973002/diff/520001/chrome/browser/c... chrome/browser/chromeos/power/cpu_data_collector.h:65: void Start(); add a brief comment documenting that this starts the timer https://chromiumcodereview.appspot.com/149973002/diff/520001/chrome/browser/c... chrome/browser/chromeos/power/cpu_data_collector.h:68: void PostSampleCpuState(); add a brief comment documenting what this method does
PTaL at patch set 13. https://chromiumcodereview.appspot.com/149973002/diff/520001/chrome/browser/c... File chrome/browser/chromeos/power/cpu_data_collector.cc (right): https://chromiumcodereview.appspot.com/149973002/diff/520001/chrome/browser/c... chrome/browser/chromeos/power/cpu_data_collector.cc:23: const int kSamplingDurationLimitMillisec = 500; On 2014/03/07 00:49:18, Daniel Erat wrote: > nit: s/Millisec/Ms/ (abbreviating "milliseconds" just halfway seems a bit weird) > :-) Done. https://chromiumcodereview.appspot.com/149973002/diff/520001/chrome/browser/c... chrome/browser/chromeos/power/cpu_data_collector.cc:139: int64 occupancy_time; On 2014/03/07 00:49:18, Daniel Erat wrote: > nit: rename to occupancy_time_usec Done. https://chromiumcodereview.appspot.com/149973002/diff/520001/chrome/browser/c... chrome/browser/chromeos/power/cpu_data_collector.cc:141: &state_name) || On 2014/03/07 00:49:18, Daniel Erat wrote: > nit: fix indenting (should be indented one more space) Done. https://chromiumcodereview.appspot.com/149973002/diff/520001/chrome/browser/c... chrome/browser/chromeos/power/cpu_data_collector.cc:143: &occupancy_time_string)) { On 2014/03/07 00:49:18, Daniel Erat wrote: > nit: here too Done. https://chromiumcodereview.appspot.com/149973002/diff/520001/chrome/browser/c... chrome/browser/chromeos/power/cpu_data_collector.cc:158: int64 time_in_state = occupancy_time / 1000; On 2014/03/07 00:49:18, Daniel Erat wrote: > nit: rename to time_in_state_ms Done. https://chromiumcodereview.appspot.com/149973002/diff/520001/chrome/browser/c... chrome/browser/chromeos/power/cpu_data_collector.cc:234: int64 occupancy_time; On 2014/03/07 00:49:18, Daniel Erat wrote: > nit: rename this to include the units Done. https://chromiumcodereview.appspot.com/149973002/diff/520001/chrome/browser/c... chrome/browser/chromeos/power/cpu_data_collector.cc:247: freq_sample.time_in_state[index] = occupancy_time * 10; On 2014/03/07 00:49:18, Daniel Erat wrote: > multiplying by 10 seems a bit strange -- is this correct? From Documentation/cpu-freq/cpufreq-stats.txt: "This gives the amount of time spent in each of the frequencies supported by this CPU. The cat output will have "<frequency> <time>" pair in each line, which will mean this CPU spent <time> usertime units of time at <frequency>. Output will have one line for each of the supported frequencies. usertime units here is 10mS (similar to other time exported in /proc)." I have added a comment here. https://chromiumcodereview.appspot.com/149973002/diff/520001/chrome/browser/c... chrome/browser/chromeos/power/cpu_data_collector.cc:286: std::vector<std::string>* cpu_idle_state_names = new std::vector<std::string>; On 2014/03/07 00:49:18, Daniel Erat wrote: > these should probably initialized with the existing vectors, like > "std::vector<std::string>(cpu_idle_state_names_)", right? it'd be bad if the > order changed... Done. https://chromiumcodereview.appspot.com/149973002/diff/520001/chrome/browser/c... chrome/browser/chromeos/power/cpu_data_collector.cc:289: std::vector<std::string>* cpu_freq_state_names = new std::vector<std::string>; On 2014/03/07 00:49:18, Daniel Erat wrote: > same here Done. https://chromiumcodereview.appspot.com/149973002/diff/520001/chrome/browser/c... chrome/browser/chromeos/power/cpu_data_collector.cc:317: // later. A system will atleast have one CPU. Hence, a value of 1 here On 2014/03/07 00:49:18, Daniel Erat wrote: > nit: s/atleast/at least/ Done. https://chromiumcodereview.appspot.com/149973002/diff/520001/chrome/browser/c... chrome/browser/chromeos/power/cpu_data_collector.cc:349: cpu_idle_state_data_.resize(cpu_count_); On 2014/03/07 00:49:18, Daniel Erat wrote: > this seems wrong -- what happens if the ui thread accesses these while they're > being resized? at the very least, i think that you'd only want to call this in > the block where you initialize cpu_count_ above, but it'd probably be safest to > pass cpu_count_ to the blocking thread as well and then pass it back to the ui > thread, and only resize these on the ui thread (i.e. don't let the blocking > thread access any members of the class -- you could even move > SampleCpuStateOnBlockingPool() to the anonymous namespace instead of keeping it > as a instance method to make sure that there's no risk of races between the two > threads) I moved this to SaveCpuStateSamplesOnUIThread. https://chromiumcodereview.appspot.com/149973002/diff/520001/chrome/browser/c... chrome/browser/chromeos/power/cpu_data_collector.cc:366: if (idle_samples->size() > 0) { On 2014/03/07 00:49:18, Daniel Erat wrote: > nit: !idle_samples->empty() Done. https://chromiumcodereview.appspot.com/149973002/diff/520001/chrome/browser/c... chrome/browser/chromeos/power/cpu_data_collector.cc:374: if (freq_samples->size() > 0) { On 2014/03/07 00:49:18, Daniel Erat wrote: > nit: !freq_samples->empty() Done. https://chromiumcodereview.appspot.com/149973002/diff/520001/chrome/browser/c... File chrome/browser/chromeos/power/cpu_data_collector.h (right): https://chromiumcodereview.appspot.com/149973002/diff/520001/chrome/browser/c... chrome/browser/chromeos/power/cpu_data_collector.h:29: StateOccupancySample(); On 2014/03/07 00:49:18, Daniel Erat wrote: > also declare a d'tor and define it in the .cc file; see > http://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Stop-in.... Done. https://chromiumcodereview.appspot.com/149973002/diff/520001/chrome/browser/c... chrome/browser/chromeos/power/cpu_data_collector.h:63: CpuDataCollector(); On 2014/03/07 00:49:18, Daniel Erat wrote: > declare a d'tor for this too Done. https://chromiumcodereview.appspot.com/149973002/diff/520001/chrome/browser/c... chrome/browser/chromeos/power/cpu_data_collector.h:65: void Start(); On 2014/03/07 00:49:18, Daniel Erat wrote: > add a brief comment documenting that this starts the timer Done. https://chromiumcodereview.appspot.com/149973002/diff/520001/chrome/browser/c... chrome/browser/chromeos/power/cpu_data_collector.h:68: void PostSampleCpuState(); On 2014/03/07 00:49:18, Daniel Erat wrote: > add a brief comment documenting what this method does Done.
Sorry, look at patch set 14.
lgtm for c++, but please ask someone like arv@ to review the rest https://chromiumcodereview.appspot.com/149973002/diff/520001/chrome/browser/c... File chrome/browser/chromeos/power/cpu_data_collector.cc (right): https://chromiumcodereview.appspot.com/149973002/diff/520001/chrome/browser/c... chrome/browser/chromeos/power/cpu_data_collector.cc:247: freq_sample.time_in_state[index] = occupancy_time * 10; On 2014/03/07 22:20:01, Siva Chandra wrote: > On 2014/03/07 00:49:18, Daniel Erat wrote: > > multiplying by 10 seems a bit strange -- is this correct? > > From Documentation/cpu-freq/cpufreq-stats.txt: > > "This gives the amount of time spent in each of the frequencies supported by > this CPU. The cat output will have "<frequency> <time>" pair in each line, which > will mean this CPU spent <time> usertime units of time at <frequency>. Output > will have one line for each of the supported frequencies. usertime units here > is 10mS (similar to other time exported in /proc)." > > I have added a comment here. thanks! https://chromiumcodereview.appspot.com/149973002/diff/560001/chrome/browser/c... File chrome/browser/chromeos/power/cpu_data_collector.h (right): https://chromiumcodereview.appspot.com/149973002/diff/560001/chrome/browser/c... chrome/browser/chromeos/power/cpu_data_collector.h:112: int cpu_count_; nit: add a comment documenting that this should only be read or written from the blocking pool
+r arv@ for JavaScript/HTML/CSS I will address derat@'s nit when I upload the next patch set.
oh, and one other question: have you estimated how much memory this consumes after samples have been collected for a day? just want to double-check that it doesn't get huge. https://codereview.chromium.org/149973002/diff/560001/chrome/browser/chromeos... File chrome/browser/chromeos/power/cpu_data_collector.cc (right): https://codereview.chromium.org/149973002/diff/560001/chrome/browser/chromeos... chrome/browser/chromeos/power/cpu_data_collector.cc:107: std::vector<CpuDataCollector::StateOccupancySample>* idle_samples) { one more thing i forgot to mention: if you expect all of the states to be represented in each sample, i think that you could avoid some overallocations by std::vector by doing something like: idle_samples->reserve(cpu_idle_state_names->size()); at the beginning of this method. ditto for SampleCpuFreqData(). (it may not matter if there are just a few states, but i think that std::vector typically doubles the existing capacity whenever it runs out of space -- you can avoid this if you know the probable exact size ahead of time.)
On 2014/03/07 22:36:38, Daniel Erat wrote: > oh, and one other question: have you estimated how much memory this consumes > after samples have been collected for a day? just want to double-check that it > doesn't get huge. It should be around 2.5MB per day worst case. Using just the sizes the fields of the classes involved and assuming vectors are arrays, it comes to ~1.2MB per day. Assuming a worst case of 100% size overhead due to vectors and deques involved, I am saying that it should be a worst case of 2.5MB per day.
On 2014/03/07 22:59:20, Siva Chandra wrote: > On 2014/03/07 22:36:38, Daniel Erat wrote: > > oh, and one other question: have you estimated how much memory this consumes > > after samples have been collected for a day? just want to double-check that it > > doesn't get huge. > > It should be around 2.5MB per day worst case. Using just the sizes the fields of > the classes involved and assuming vectors are arrays, it comes to ~1.2MB per > day. Assuming a worst case of 100% size overhead due to vectors and deques > involved, I am saying that it should be a worst case of 2.5MB per day. And, this is for a pixel which has 4 idle states and 7 frequency states.
thanks, that sounds okay to me.
+r arv@ for JS/HTML/CSS. Don't know why it didn't happen on Friday.
js/html/css LGTM with nits https://chromiumcodereview.appspot.com/149973002/diff/560001/chrome/browser/r... File chrome/browser/resources/chromeos/power.css (right): https://chromiumcodereview.appspot.com/149973002/diff/560001/chrome/browser/r... chrome/browser/resources/chromeos/power.css:39: border-bottom-width: 0; These can be combined into border-width https://chromiumcodereview.appspot.com/149973002/diff/560001/chrome/browser/r... File chrome/browser/resources/chromeos/power.js (right): https://chromiumcodereview.appspot.com/149973002/diff/560001/chrome/browser/r... chrome/browser/resources/chromeos/power.js:105: // Show a legend only if atleast one individual plot have a name. at least https://chromiumcodereview.appspot.com/149973002/diff/560001/chrome/browser/r... chrome/browser/resources/chromeos/power.js:407: var br = document.createElement('br'); I doubt this br is needed https://chromiumcodereview.appspot.com/149973002/diff/560001/chrome/browser/r... chrome/browser/resources/chromeos/power.js:811: remove empty line
On 2014/03/10 16:43:21, Siva Chandra wrote: > +r arv@ for JS/HTML/CSS. > Don't know why it didn't happen on Friday. What do you mean?
On 2014/03/10 17:01:04, arv wrote: > On 2014/03/10 16:43:21, Siva Chandra wrote: > > +r arv@ for JS/HTML/CSS. > > Don't know why it didn't happen on Friday. > > What do you mean? I was just wondering why you didn't get added on Friday; I distinctly remember adding you and sending a mail.
You did as me on Friday. I just didn't get it before leaving. On Mar 10, 2014 1:02 PM, <sivachandra@chromium.org> wrote: > On 2014/03/10 17:01:04, arv wrote: > >> On 2014/03/10 16:43:21, Siva Chandra wrote: >> > +r arv@ for JS/HTML/CSS. >> > Don't know why it didn't happen on Friday. >> > > What do you mean? >> > > I was just wondering why you didn't get added on Friday; I distinctly > remember > adding you and sending a mail. > > https://chromiumcodereview.appspot.com/149973002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/03/10 17:03:50, arv wrote: > You did as me on Friday. I just didn't get it before leaving. Yes. My mailbox does show I did it. But, when I looked at the review page, I did not see 'arv' under "Reviewers". That is why I added you again.
The CQ bit was checked by sivachandra@chromium.org
Checked the commit box after addressing nits. https://chromiumcodereview.appspot.com/149973002/diff/560001/chrome/browser/c... File chrome/browser/chromeos/power/cpu_data_collector.cc (right): https://chromiumcodereview.appspot.com/149973002/diff/560001/chrome/browser/c... chrome/browser/chromeos/power/cpu_data_collector.cc:107: std::vector<CpuDataCollector::StateOccupancySample>* idle_samples) { On 2014/03/07 22:36:39, Daniel Erat wrote: > one more thing i forgot to mention: if you expect all of the states to be > represented in each sample, i think that you could avoid some overallocations by > std::vector by doing something like: > > idle_samples->reserve(cpu_idle_state_names->size()); > > at the beginning of this method. ditto for SampleCpuFreqData(). > > (it may not matter if there are just a few states, but i think that std::vector > typically doubles the existing capacity whenever it runs out of space -- you can > avoid this if you know the probable exact size ahead of time.) I think you mean idle_sample.time_in_state vector below. Made this change for that vector and for freq samples as well. https://chromiumcodereview.appspot.com/149973002/diff/560001/chrome/browser/c... File chrome/browser/chromeos/power/cpu_data_collector.h (right): https://chromiumcodereview.appspot.com/149973002/diff/560001/chrome/browser/c... chrome/browser/chromeos/power/cpu_data_collector.h:112: int cpu_count_; On 2014/03/07 22:28:34, Daniel Erat wrote: > nit: add a comment documenting that this should only be read or written from the > blocking pool Done. https://chromiumcodereview.appspot.com/149973002/diff/560001/chrome/browser/r... File chrome/browser/resources/chromeos/power.css (right): https://chromiumcodereview.appspot.com/149973002/diff/560001/chrome/browser/r... chrome/browser/resources/chromeos/power.css:39: border-bottom-width: 0; On 2014/03/10 17:00:46, arv wrote: > These can be combined into border-width Borders on all sides are not the same. https://chromiumcodereview.appspot.com/149973002/diff/560001/chrome/browser/r... File chrome/browser/resources/chromeos/power.js (right): https://chromiumcodereview.appspot.com/149973002/diff/560001/chrome/browser/r... chrome/browser/resources/chromeos/power.js:105: // Show a legend only if atleast one individual plot have a name. On 2014/03/10 17:00:46, arv wrote: > at least Done. https://chromiumcodereview.appspot.com/149973002/diff/560001/chrome/browser/r... chrome/browser/resources/chromeos/power.js:407: var br = document.createElement('br'); On 2014/03/10 17:00:46, arv wrote: > I doubt this br is needed Done. https://chromiumcodereview.appspot.com/149973002/diff/560001/chrome/browser/r... chrome/browser/resources/chromeos/power.js:811: On 2014/03/10 17:00:46, arv wrote: > remove empty line Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sivachandra@chromium.org/149973002/580001
The CQ bit was unchecked by sivachandra@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on win for step(s) compile http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...
Dan, Can you PTaL. I have modified power_data_collector* to accommodate tests. Thanks, Siva Chandra
lgtm https://codereview.chromium.org/149973002/diff/600001/chrome/browser/chromeos... File chrome/browser/chromeos/power/power_data_collector.h (right): https://codereview.chromium.org/149973002/diff/600001/chrome/browser/chromeos... chrome/browser/chromeos/power/power_data_collector.h:94: explicit PowerDataCollector(const bool testing); nit: s/testing/start_collector/ (and invert 'if' condition in c'tor impl)
The CQ bit was checked by sivachandra@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sivachandra@chromium.org/149973002/620001
Message was sent while issue was closed.
Change committed as 256294
Message was sent while issue was closed.
On 2014/03/11 20:17:53, I haz the power (commit-bot) wrote:
> Change committed as 256294
Hi! This reliably causes a crash on linux-chromeos build a few seconds after
startup with the following trace:
[9078:9078:0311/220307:FATAL:weak_ptr.cc(26)] Check failed:
sequence_checker_.CalledOnValidSequencedThread(). WeakPtrs must be checked on
the same sequenced thread.
[0x7f07864f96ce] base::debug::StackTrace::StackTrace()
[0x7f078657c242] logging::LogMessage::~LogMessage()
[0x7f078659c745] base::internal::WeakReference::Flag::IsValid()
[0x7f078659c85b] base::internal::WeakReference::is_valid()
[0x7f078c14f96f] base::WeakPtr<>::get()
[0x7f078c14f8a1] base::internal::InvokeHelper<>::MakeItSo()
[0x7f078c14f82e] base::internal::Invoker<>::Run()
[0x7f07864e0a9e] base::Callback<>::Run()
[0x7f078666ea88] base::(anonymous
namespace)::PostTaskAndReplyRelay::RunReplyAndSelfDestruct()
[0x7f078666e872] base::internal::RunnableAdapter<>::Run()
[0x7f078666e7dc] base::internal::InvokeHelper<>::MakeItSo()
[0x7f078666e78a] base::internal::Invoker<>::Run()
[0x7f07864e0a9e] base::Callback<>::Run()
[0x7f07865a1e48] base::MessageLoop::RunTask()
[0x7f07865a216b] base::MessageLoop::DeferOrRunPendingTask()
[0x7f07865a2395] base::MessageLoop::DoWork()
[0x7f07864a853c] base::MessagePumpGlib::HandleDispatch()
[0x7f07864a89c1] base::(anonymous namespace)::WorkSourceDispatch()
[0x7f0775885d13] g_main_context_dispatch
[0x7f0775886060] <unknown>
[0x7f0775886124] g_main_context_iteration
[0x7f07864a8010] base::MessagePumpGlib::RunWithDispatcher()
[0x7f07864a85d9] base::MessagePumpGlib::Run()
[0x7f07865a1787] base::MessageLoop::RunHandler()
[0x7f07866187f2] base::RunLoop::Run()
[0x7f078cdf9db9] ChromeBrowserMainParts::MainMessageLoopRun()
[0x7f0779e55a5f] content::BrowserMainLoop::RunMainMessageLoopParts()
[0x7f0779e5f6c7] content::BrowserMainRunnerImpl::Run()
[0x7f0779e4fe01] content::BrowserMain()
[0x7f0779deeabf] content::RunNamedProcessTypeMain()
[0x7f0779df160d] content::ContentMainRunnerImpl::Run()
[0x7f0779dee0f5] content::ContentMain()
[0x7f07891c4d55] ChromeMain
[0x7f07891c4d02] main
[0x7f0774a8676d] __libc_start_main
[0x7f07891c4c09] <unknown>
I added some debug code, and looks like the issue is that a WeakPtr is created
in the UI thread, that is attempted to be used in a non-UI thread for the
CpuDataCollector::SampleCpuStateOnBlockingPool() task. This is not allowed,
since the WeakPtr must be dereferenced only in the thread it is created on.
A temporary hack of:
@ -300,7 +300,7 @@ void CpuDataCollector::PostSampleCpuState() {
content::BrowserThread::PostBlockingPoolTaskAndReply(
FROM_HERE,
base::Bind(&CpuDataCollector::SampleCpuStateOnBlockingPool,
- weak_ptr_factory_.GetWeakPtr(),
+ base::Unretained(this),
base::Unretained(cpu_idle_state_names),
base::Unretained(idle_samples),
base::Unretained(cpu_freq_state_names),
gets rid of the crash, but it's probably not the right fix. Are you already
aware of this issue, and do you have a proper fix for this? If not, would you
mind reverting from ToT while you investigate? Thanks.
Message was sent while issue was closed.
On 2014/03/12 02:12:28, sadrul wrote:
> On 2014/03/11 20:17:53, I haz the power (commit-bot) wrote:
> > Change committed as 256294
>
> Hi! This reliably causes a crash on linux-chromeos build a few seconds after
> startup with the following trace:
>
> [9078:9078:0311/220307:FATAL:weak_ptr.cc(26)] Check failed:
> sequence_checker_.CalledOnValidSequencedThread(). WeakPtrs must be checked on
> the same sequenced thread.
> [0x7f07864f96ce] base::debug::StackTrace::StackTrace()
> [0x7f078657c242] logging::LogMessage::~LogMessage()
> [0x7f078659c745] base::internal::WeakReference::Flag::IsValid()
> [0x7f078659c85b] base::internal::WeakReference::is_valid()
> [0x7f078c14f96f] base::WeakPtr<>::get()
> [0x7f078c14f8a1] base::internal::InvokeHelper<>::MakeItSo()
> [0x7f078c14f82e] base::internal::Invoker<>::Run()
> [0x7f07864e0a9e] base::Callback<>::Run()
> [0x7f078666ea88] base::(anonymous
> namespace)::PostTaskAndReplyRelay::RunReplyAndSelfDestruct()
> [0x7f078666e872] base::internal::RunnableAdapter<>::Run()
> [0x7f078666e7dc] base::internal::InvokeHelper<>::MakeItSo()
> [0x7f078666e78a] base::internal::Invoker<>::Run()
> [0x7f07864e0a9e] base::Callback<>::Run()
> [0x7f07865a1e48] base::MessageLoop::RunTask()
> [0x7f07865a216b] base::MessageLoop::DeferOrRunPendingTask()
> [0x7f07865a2395] base::MessageLoop::DoWork()
> [0x7f07864a853c] base::MessagePumpGlib::HandleDispatch()
> [0x7f07864a89c1] base::(anonymous namespace)::WorkSourceDispatch()
> [0x7f0775885d13] g_main_context_dispatch
> [0x7f0775886060] <unknown>
> [0x7f0775886124] g_main_context_iteration
> [0x7f07864a8010] base::MessagePumpGlib::RunWithDispatcher()
> [0x7f07864a85d9] base::MessagePumpGlib::Run()
> [0x7f07865a1787] base::MessageLoop::RunHandler()
> [0x7f07866187f2] base::RunLoop::Run()
> [0x7f078cdf9db9] ChromeBrowserMainParts::MainMessageLoopRun()
> [0x7f0779e55a5f] content::BrowserMainLoop::RunMainMessageLoopParts()
> [0x7f0779e5f6c7] content::BrowserMainRunnerImpl::Run()
> [0x7f0779e4fe01] content::BrowserMain()
> [0x7f0779deeabf] content::RunNamedProcessTypeMain()
> [0x7f0779df160d] content::ContentMainRunnerImpl::Run()
> [0x7f0779dee0f5] content::ContentMain()
> [0x7f07891c4d55] ChromeMain
> [0x7f07891c4d02] main
> [0x7f0774a8676d] __libc_start_main
> [0x7f07891c4c09] <unknown>
>
> I added some debug code, and looks like the issue is that a WeakPtr is created
> in the UI thread, that is attempted to be used in a non-UI thread for the
> CpuDataCollector::SampleCpuStateOnBlockingPool() task. This is not allowed,
> since the WeakPtr must be dereferenced only in the thread it is created on.
>
> A temporary hack of:
>
> @ -300,7 +300,7 @@ void CpuDataCollector::PostSampleCpuState() {
> content::BrowserThread::PostBlockingPoolTaskAndReply(
> FROM_HERE,
> base::Bind(&CpuDataCollector::SampleCpuStateOnBlockingPool,
> - weak_ptr_factory_.GetWeakPtr(),
> + base::Unretained(this),
> base::Unretained(cpu_idle_state_names),
> base::Unretained(idle_samples),
> base::Unretained(cpu_freq_state_names),
>
> gets rid of the crash, but it's probably not the right fix. Are you already
> aware of this issue, and do you have a proper fix for this? If not, would you
> mind reverting from ToT while you investigate? Thanks.
crashing at startup seems bad. probably best to revert quickly (siva appears to
be away right now)
Message was sent while issue was closed.
A revert of this CL has been created in https://codereview.chromium.org/196613002/ by derat@chromium.org. The reason for reverting is: Per Sadrul's comment, this causes a crash several seconds after startup.. |
