Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2396743002:
Leak reports collect information about the memory usage (Closed)
DescriptionLeak reports collect information about the memory usage
LeakDetectorController calls the MetricsMemoryDetails
to collect information about the memory usage and writes
it in the received leak reports.
BUG=650347
TEST=1. Modify kDefaultProbability and kDefaultNumProcessesEnabled in leak_detector_controller.cc to always launch the detector.
2. Add a dummy report in leak_detector.cc before detector->NotifyObservers()
3. Add debug info with LOG(ERROR) in leak_detector_controller.cc:OnMemoryDetailCollectionDone()
4. Launch Chromium with flag enable-features=RuntimeMemoryLeakDetector
Committed: https://crrev.com/ee4005181ef90e80428b3569fd5a364f50f334e6
Cr-Commit-Position: refs/heads/master@{#425212}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Fixed order of class members #Patch Set 3 : Fixed order of initialization #
Total comments: 10
Patch Set 4 : Refactoring after Simon's comments #Patch Set 5 : Refactoring after Simon's comments 2 #
Total comments: 2
Patch Set 6 : Make the unit tests pass #
Total comments: 4
Patch Set 7 : Added flag enable_collect_memory_usage_ #
Total comments: 4
Patch Set 8 : Changing members' names #
Total comments: 9
Patch Set 9 : New MetricsMemoryDetails constructor, no histogram data generated #
Total comments: 3
Patch Set 10 : Fixed scope of 'if defined(OS_CHROMEOS)' #
Total comments: 10
Patch Set 11 : Added setter for |generate_histograms_| #
Messages
Total messages: 75 (52 generated)
The CQ bit was checked by mwlodar@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Leak reports collect information about the memory usage LeakDetectorController calls the MetricsMemoryDetails to collect information about the memory usage and writes it in the received leak reports. BUG=650347 ========== to ========== Leak reports collect information about the memory usage LeakDetectorController calls the MetricsMemoryDetails to collect information about the memory usage and writes it in the received leak reports. BUG=650347 ==========
mwlodar@google.com changed reviewers: + sque@google.com
https://codereview.chromium.org/2396743002/diff/1/chrome/browser/metrics/leak... File chrome/browser/metrics/leak_detector/leak_detector_controller.cc (right): https://codereview.chromium.org/2396743002/diff/1/chrome/browser/metrics/leak... chrome/browser/metrics/leak_detector/leak_detector_controller.cc:249: DCHECK(thread_checker_.CalledOnValidThread()); Is there some guarantee that MetricsMemoryDetails will run the callback in the same thread? https://codereview.chromium.org/2396743002/diff/1/chrome/browser/metrics/leak... chrome/browser/metrics/leak_detector/leak_detector_controller.cc:275: if (!waiting_for_collect_memory_usage_) { I don't know what kind of tests suits this code the best. With some additional code "details->StartFetch()" part could be faked in a unit test. However the more interesting question is if MetricsMemoryDetails always uses the callback and the memory growth tracker in a proper way.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by mwlodar@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
The CQ bit was checked by mwlodar@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2396743002/diff/1/chrome/browser/metrics/leak... File chrome/browser/metrics/leak_detector/leak_detector_controller.cc (right): https://codereview.chromium.org/2396743002/diff/1/chrome/browser/metrics/leak... chrome/browser/metrics/leak_detector/leak_detector_controller.cc:156: bool TotalMemoryGrowthTracker::UpdateSample( Right now we override this method totally in order to suppress generating growth/shrink information and save some time. But maybe we should always generate all the histogram data?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
sque@chromium.org changed reviewers: + sque@chromium.org
https://codereview.chromium.org/2396743002/diff/40001/chrome/browser/metrics/... File chrome/browser/metrics/leak_detector/leak_detector_controller.cc (right): https://codereview.chromium.org/2396743002/diff/40001/chrome/browser/metrics/... chrome/browser/metrics/leak_detector/leak_detector_controller.cc:162: size_t TotalMemoryGrowthTracker::GetTotalUsageKb() const { This can be inlined, and renamed to total_usage_kb(): https://google.github.io/styleguide/cppguide.html#Function_Names https://codereview.chromium.org/2396743002/diff/40001/chrome/browser/metrics/... chrome/browser/metrics/leak_detector/leak_detector_controller.cc:166: void TotalMemoryGrowthTracker::Reset() { Same here. https://codereview.chromium.org/2396743002/diff/40001/chrome/browser/metrics/... chrome/browser/metrics/leak_detector/leak_detector_controller.cc:263: stored_reports_[index].mutable_memory_usage_info()->CopyFrom(mem_info); You could be overwriting mem info in previously stored reports. You'll need a different implementation. Also keep in mind that GetLeakReports() could be called between StoreLeakReports() and OnMemoryDetailCollectionDone. https://codereview.chromium.org/2396743002/diff/40001/chrome/browser/metrics/... File chrome/browser/metrics/leak_detector/leak_detector_controller.h (right): https://codereview.chromium.org/2396743002/diff/40001/chrome/browser/metrics/... chrome/browser/metrics/leak_detector/leak_detector_controller.h:22: class TotalMemoryGrowthTracker : public MemoryGrowthTracker { Make this a private class within LeakDetectorController since it is not used outside of LDC. https://codereview.chromium.org/2396743002/diff/40001/chrome/browser/metrics/... chrome/browser/metrics/leak_detector/leak_detector_controller.h:53: void OnMemoryDetailCollectionDone(size_t index); I think this can be made private.
The CQ bit was checked by mwlodar@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
mwlodar@google.com changed reviewers: + asvitkine@google.com, isherman@chromium.org - sque@chromium.org
https://codereview.chromium.org/2396743002/diff/40001/chrome/browser/metrics/... File chrome/browser/metrics/leak_detector/leak_detector_controller.cc (right): https://codereview.chromium.org/2396743002/diff/40001/chrome/browser/metrics/... chrome/browser/metrics/leak_detector/leak_detector_controller.cc:162: size_t TotalMemoryGrowthTracker::GetTotalUsageKb() const { On 2016/10/05 22:12:43, Simon Que wrote: > This can be inlined, and renamed to total_usage_kb(): > > https://google.github.io/styleguide/cppguide.html#Function_Names Done. https://codereview.chromium.org/2396743002/diff/40001/chrome/browser/metrics/... chrome/browser/metrics/leak_detector/leak_detector_controller.cc:166: void TotalMemoryGrowthTracker::Reset() { On 2016/10/05 22:12:43, Simon Que wrote: > Same here. Done. https://codereview.chromium.org/2396743002/diff/40001/chrome/browser/metrics/... chrome/browser/metrics/leak_detector/leak_detector_controller.cc:263: stored_reports_[index].mutable_memory_usage_info()->CopyFrom(mem_info); We have talked about this. No fix needed. https://codereview.chromium.org/2396743002/diff/40001/chrome/browser/metrics/... File chrome/browser/metrics/leak_detector/leak_detector_controller.h (right): https://codereview.chromium.org/2396743002/diff/40001/chrome/browser/metrics/... chrome/browser/metrics/leak_detector/leak_detector_controller.h:22: class TotalMemoryGrowthTracker : public MemoryGrowthTracker { On 2016/10/05 22:12:43, Simon Que wrote: > Make this a private class within LeakDetectorController since it is not used > outside of LDC. Done. https://codereview.chromium.org/2396743002/diff/40001/chrome/browser/metrics/... chrome/browser/metrics/leak_detector/leak_detector_controller.h:53: void OnMemoryDetailCollectionDone(size_t index); On 2016/10/05 22:12:43, Simon Que wrote: > I think this can be made private. Done. https://codereview.chromium.org/2396743002/diff/80001/chrome/browser/metrics/... File chrome/browser/metrics/leak_detector/leak_detector_controller.cc (right): https://codereview.chromium.org/2396743002/diff/80001/chrome/browser/metrics/... chrome/browser/metrics/leak_detector/leak_detector_controller.cc:282: details->StartFetch(); This method calls DCHECK(!BrowserThread::CurrentlyOn(BrowserThread::IO)) which makes LeakDetectorControllerTest fail. What can be done about this?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by mwlodar@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
https://codereview.chromium.org/2396743002/diff/80001/chrome/browser/metrics/... File chrome/browser/metrics/leak_detector/leak_detector_controller.cc (right): https://codereview.chromium.org/2396743002/diff/80001/chrome/browser/metrics/... chrome/browser/metrics/leak_detector/leak_detector_controller.cc:282: details->StartFetch(); On 2016/10/06 00:07:53, mwlodar wrote: > This method calls DCHECK(!BrowserThread::CurrentlyOn(BrowserThread::IO)) which > makes LeakDetectorControllerTest fail. What can be done about this? This was fixed in patch set 6 by initializing waiting_for_collect_memory_usage_ to true in tests, what blocks running the new code.
sque@chromium.org changed reviewers: + sque@chromium.org
Getting closer. :) https://codereview.chromium.org/2396743002/diff/100001/chrome/browser/metrics... File chrome/browser/metrics/leak_detector/leak_detector_controller.h (right): https://codereview.chromium.org/2396743002/diff/100001/chrome/browser/metrics... chrome/browser/metrics/leak_detector/leak_detector_controller.h:46: bool waiting_for_collect_memory_usage_; This is not a good design pattern because you're exposing a member variable here to a derived class. Instead: 1. Make this a private variable. 2. Create a protected mutator to set this flag. 3. Call the mutator from the derived test class. https://codereview.chromium.org/2396743002/diff/100001/chrome/browser/metrics... File chrome/browser/metrics/leak_detector/leak_detector_controller_unittest.cc (right): https://codereview.chromium.org/2396743002/diff/100001/chrome/browser/metrics... chrome/browser/metrics/leak_detector/leak_detector_controller_unittest.cc:37: LeakDetectorController::waiting_for_collect_memory_usage_ = true; You have the right idea but this is not the best design pattern. This is a state variable that you're treating like an initialization flag. Instead, create a separate boolean in LeakDetectorController to indicate whether memory usage info should be collected. Initialize that to true by default, and then set it to false here.
The CQ bit was checked by mwlodar@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2396743002/diff/100001/chrome/browser/metrics... File chrome/browser/metrics/leak_detector/leak_detector_controller.h (right): https://codereview.chromium.org/2396743002/diff/100001/chrome/browser/metrics... chrome/browser/metrics/leak_detector/leak_detector_controller.h:46: bool waiting_for_collect_memory_usage_; On 2016/10/07 06:24:58, Simon Que wrote: > This is not a good design pattern because you're exposing a member variable here > to a derived class. Instead: > > 1. Make this a private variable. > 2. Create a protected mutator to set this flag. > 3. Call the mutator from the derived test class. Analogous construction done with an initialization flag. https://codereview.chromium.org/2396743002/diff/100001/chrome/browser/metrics... File chrome/browser/metrics/leak_detector/leak_detector_controller_unittest.cc (right): https://codereview.chromium.org/2396743002/diff/100001/chrome/browser/metrics... chrome/browser/metrics/leak_detector/leak_detector_controller_unittest.cc:37: LeakDetectorController::waiting_for_collect_memory_usage_ = true; On 2016/10/07 06:24:58, Simon Que wrote: > You have the right idea but this is not the best design pattern. This is a state > variable that you're treating like an initialization flag. > > Instead, create a separate boolean in LeakDetectorController to indicate whether > memory usage info should be collected. Initialize that to true by default, and > then set it to false here. Done.
cc: chongjiang
lgtm after nits https://codereview.chromium.org/2396743002/diff/120001/chrome/browser/metrics... File chrome/browser/metrics/leak_detector/leak_detector_controller.h (right): https://codereview.chromium.org/2396743002/diff/120001/chrome/browser/metrics... chrome/browser/metrics/leak_detector/leak_detector_controller.h:44: void set_enable_collect_memory_usage_(bool enabled) { Nit: No trailing underscore in mutator name. https://codereview.chromium.org/2396743002/diff/120001/chrome/browser/metrics... chrome/browser/metrics/leak_detector/leak_detector_controller.h:119: bool enable_collect_memory_usage_; Nit: incorrect grammar, try: enable_memory_usage_collection_
The CQ bit was checked by mwlodar@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2396743002/diff/120001/chrome/browser/metrics... File chrome/browser/metrics/leak_detector/leak_detector_controller.h (right): https://codereview.chromium.org/2396743002/diff/120001/chrome/browser/metrics... chrome/browser/metrics/leak_detector/leak_detector_controller.h:44: void set_enable_collect_memory_usage_(bool enabled) { On 2016/10/07 20:24:25, Simon Que wrote: > Nit: No trailing underscore in mutator name. Done. https://codereview.chromium.org/2396743002/diff/120001/chrome/browser/metrics... chrome/browser/metrics/leak_detector/leak_detector_controller.h:119: bool enable_collect_memory_usage_; On 2016/10/07 20:24:25, Simon Que wrote: > Nit: incorrect grammar, try: enable_memory_usage_collection_ Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
asvitkine, isherman: PTAL at changes to chrome/browser/metrics/metrics_memory_details.h
asvitkine@chromium.org changed reviewers: + asvitkine@chromium.org
https://codereview.chromium.org/2396743002/diff/140001/chrome/browser/metrics... File chrome/browser/metrics/leak_detector/leak_detector_controller.cc (right): https://codereview.chromium.org/2396743002/diff/140001/chrome/browser/metrics... chrome/browser/metrics/leak_detector/leak_detector_controller.cc:150: LeakDetectorController::TotalMemoryGrowthTracker::TotalMemoryGrowthTracker() Nit: Order these same as in the .h file in relation to methods of the outer class. https://codereview.chromium.org/2396743002/diff/140001/chrome/browser/metrics... chrome/browser/metrics/leak_detector/leak_detector_controller.cc:277: // data is generated. I don't think we want this to be the case. The metrics are currently logged only once per UMA log, but this will change things, which will be confusing. Can you implement a mechanism to suppress logging the metrics when the class is used for your use case? https://codereview.chromium.org/2396743002/diff/140001/chrome/browser/metrics... File chrome/browser/metrics/leak_detector/leak_detector_controller.h (right): https://codereview.chromium.org/2396743002/diff/140001/chrome/browser/metrics... chrome/browser/metrics/leak_detector/leak_detector_controller.h:64: }; DISALLOW_COPY_AND_ASSIGN(). https://codereview.chromium.org/2396743002/diff/140001/chrome/browser/metrics... File chrome/browser/metrics/metrics_memory_details.h (right): https://codereview.chromium.org/2396743002/diff/140001/chrome/browser/metrics... chrome/browser/metrics/metrics_memory_details.h:20: ~MemoryGrowthTracker(); Make this virtual and mark it override in the subclass now that you're inheriting from this.
https://codereview.chromium.org/2396743002/diff/140001/chrome/browser/metrics... File chrome/browser/metrics/leak_detector/leak_detector_controller.cc (right): https://codereview.chromium.org/2396743002/diff/140001/chrome/browser/metrics... chrome/browser/metrics/leak_detector/leak_detector_controller.cc:277: // data is generated. On 2016/10/11 16:47:48, Alexei Svitkine (slow) wrote: > I don't think we want this to be the case. The metrics are currently logged only > once per UMA log, but this will change things, which will be confusing. Can you > implement a mechanism to suppress logging the metrics when the class is used for > your use case? I can, however I would have to modify the interface of MetricsMemoryDetails. One solution is to add a flag |generate_histograms_| that would be true on default and add a new constructor to set it. What do you think?
Sure, that seems fine. On Tue, Oct 11, 2016 at 4:08 PM, <mwlodar@google.com> wrote: > > https://codereview.chromium.org/2396743002/diff/140001/ > chrome/browser/metrics/leak_detector/leak_detector_controller.cc > File chrome/browser/metrics/leak_detector/leak_detector_controller.cc > (right): > > https://codereview.chromium.org/2396743002/diff/140001/ > chrome/browser/metrics/leak_detector/leak_detector_ > controller.cc#newcode277 > chrome/browser/metrics/leak_detector/leak_detector_controller.cc:277: // > data is generated. > On 2016/10/11 16:47:48, Alexei Svitkine (slow) wrote: > > I don't think we want this to be the case. The metrics are currently > logged only > > once per UMA log, but this will change things, which will be > confusing. Can you > > implement a mechanism to suppress logging the metrics when the class > is used for > > your use case? > > I can, however I would have to modify the interface of > MetricsMemoryDetails. One solution is to add a flag > |generate_histograms_| that would be true on default and add a new > constructor to set it. What do you think? > > https://codereview.chromium.org/2396743002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2016/10/11 20:13:41, chromium-reviews wrote: > Sure, that seems fine. > > On Tue, Oct 11, 2016 at 4:08 PM, <mailto:mwlodar@google.com> wrote: > > > > > https://codereview.chromium.org/2396743002/diff/140001/ > > chrome/browser/metrics/leak_detector/leak_detector_controller.cc > > File chrome/browser/metrics/leak_detector/leak_detector_controller.cc > > (right): > > > > https://codereview.chromium.org/2396743002/diff/140001/ > > chrome/browser/metrics/leak_detector/leak_detector_ > > controller.cc#newcode277 > > chrome/browser/metrics/leak_detector/leak_detector_controller.cc:277: // > > data is generated. > > On 2016/10/11 16:47:48, Alexei Svitkine (slow) wrote: > > > I don't think we want this to be the case. The metrics are currently > > logged only > > > once per UMA log, but this will change things, which will be > > confusing. Can you > > > implement a mechanism to suppress logging the metrics when the class > > is used for > > > your use case? > > > > I can, however I would have to modify the interface of > > MetricsMemoryDetails. One solution is to add a flag > > |generate_histograms_| that would be true on default and add a new > > constructor to set it. What do you think? > > > > https://codereview.chromium.org/2396743002/ > > > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. Ok. As I modify MetricsMemoryDetails constructor, I have a related question. In metrics_memory_details.cc:62 there is an assignment that seems redundant. Can I delete it or is there some explanation behind it?
The CQ bit was checked by mwlodar@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Leak reports collect information about the memory usage LeakDetectorController calls the MetricsMemoryDetails to collect information about the memory usage and writes it in the received leak reports. BUG=650347 ========== to ========== Leak reports collect information about the memory usage LeakDetectorController calls the MetricsMemoryDetails to collect information about the memory usage and writes it in the received leak reports. BUG=650347 TEST=This feature touches many classes. Follow these steps to test it: 1. Modify kDefaultProbability and kDefaultNumProcessesEnabled in leak_detector_controller.cc to always launch the detector. 2. Add a dummy report in leak_detector.cc before detector->NotifyObservers() 3. Add debug info with LOG(ERROR) in leak_detector_controller.cc:OnMemoryDetailCollectionDone() 4. Launch Chromium with flag enable-features=RuntimeMemoryLeakDetector ==========
Description was changed from ========== Leak reports collect information about the memory usage LeakDetectorController calls the MetricsMemoryDetails to collect information about the memory usage and writes it in the received leak reports. BUG=650347 TEST=This feature touches many classes. Follow these steps to test it: 1. Modify kDefaultProbability and kDefaultNumProcessesEnabled in leak_detector_controller.cc to always launch the detector. 2. Add a dummy report in leak_detector.cc before detector->NotifyObservers() 3. Add debug info with LOG(ERROR) in leak_detector_controller.cc:OnMemoryDetailCollectionDone() 4. Launch Chromium with flag enable-features=RuntimeMemoryLeakDetector ========== to ========== Leak reports collect information about the memory usage LeakDetectorController calls the MetricsMemoryDetails to collect information about the memory usage and writes it in the received leak reports. BUG=650347 TEST=1. Modify kDefaultProbability and kDefaultNumProcessesEnabled in leak_detector_controller.cc to always launch the detector. 2. Add a dummy report in leak_detector.cc before detector->NotifyObservers() 3. Add debug info with LOG(ERROR) in leak_detector_controller.cc:OnMemoryDetailCollectionDone() 4. Launch Chromium with flag enable-features=RuntimeMemoryLeakDetector ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2396743002/diff/160001/chrome/browser/metrics... File chrome/browser/metrics/metrics_memory_details.cc (right): https://codereview.chromium.org/2396743002/diff/160001/chrome/browser/metrics... chrome/browser/metrics/metrics_memory_details.cc:75: AnalyzeMemoryGrowth(); Since AnalyzeMemoryGrowth() is defined in a #if defined(OS_CHROMEOS) section, you should put the #if defined() here too.
The CQ bit was checked by mwlodar@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2396743002/diff/180001/chrome/browser/metrics... File chrome/browser/metrics/leak_detector/leak_detector_controller.h (right): https://codereview.chromium.org/2396743002/diff/180001/chrome/browser/metrics... chrome/browser/metrics/leak_detector/leak_detector_controller.h:65: DISALLOW_COPY_AND_ASSIGN(TotalMemoryGrowthTracker); Nit: Indent is wrong. You can run "git cl format" to fix. https://codereview.chromium.org/2396743002/diff/180001/chrome/browser/metrics... File chrome/browser/metrics/metrics_memory_details.cc (right): https://codereview.chromium.org/2396743002/diff/180001/chrome/browser/metrics... chrome/browser/metrics/metrics_memory_details.cc:59: const base::Closure& callback, MemoryGrowthTracker* memory_growth_tracker) Nit: I think convention is 1 param per line if first param is on a new line. But if this is what "git cl format" generates, I'm ok with this too. https://codereview.chromium.org/2396743002/diff/180001/chrome/browser/metrics... chrome/browser/metrics/metrics_memory_details.cc:67: generate_histograms_(generate_histograms) {} Hmm, instead of a separate constructor, why not just make a setter? https://codereview.chromium.org/2396743002/diff/180001/chrome/browser/metrics... chrome/browser/metrics/metrics_memory_details.cc:314: for (size_t index = 0; index < browser.processes.size(); index++) { Nit: use a C++ for loop: for (const auto& process_entry : GetBrowser()->processes) { } https://codereview.chromium.org/2396743002/diff/180001/chrome/browser/metrics... chrome/browser/metrics/metrics_memory_details.cc:320: if (generate_histograms_) { Nit: Merge this if with the one you have above.
https://codereview.chromium.org/2396743002/diff/140001/chrome/browser/metrics... File chrome/browser/metrics/leak_detector/leak_detector_controller.cc (right): https://codereview.chromium.org/2396743002/diff/140001/chrome/browser/metrics... chrome/browser/metrics/leak_detector/leak_detector_controller.cc:150: LeakDetectorController::TotalMemoryGrowthTracker::TotalMemoryGrowthTracker() On 2016/10/11 16:47:47, Alexei Svitkine (slow) wrote: > Nit: Order these same as in the .h file in relation to methods of the outer > class. Done. https://codereview.chromium.org/2396743002/diff/140001/chrome/browser/metrics... chrome/browser/metrics/leak_detector/leak_detector_controller.cc:277: // data is generated. On 2016/10/11 20:08:09, mwlodar wrote: > On 2016/10/11 16:47:48, Alexei Svitkine (slow) wrote: > > I don't think we want this to be the case. The metrics are currently logged > only > > once per UMA log, but this will change things, which will be confusing. Can > you > > implement a mechanism to suppress logging the metrics when the class is used > for > > your use case? > > I can, however I would have to modify the interface of MetricsMemoryDetails. One > solution is to add a flag |generate_histograms_| that would be true on default > and add a new constructor to set it. What do you think? Done. https://codereview.chromium.org/2396743002/diff/140001/chrome/browser/metrics... File chrome/browser/metrics/leak_detector/leak_detector_controller.h (right): https://codereview.chromium.org/2396743002/diff/140001/chrome/browser/metrics... chrome/browser/metrics/leak_detector/leak_detector_controller.h:64: }; On 2016/10/11 16:47:48, Alexei Svitkine (slow) wrote: > DISALLOW_COPY_AND_ASSIGN(). Done. https://codereview.chromium.org/2396743002/diff/140001/chrome/browser/metrics... File chrome/browser/metrics/metrics_memory_details.h (right): https://codereview.chromium.org/2396743002/diff/140001/chrome/browser/metrics... chrome/browser/metrics/metrics_memory_details.h:20: ~MemoryGrowthTracker(); On 2016/10/11 16:47:48, Alexei Svitkine (slow) wrote: > Make this virtual and mark it override in the subclass now that you're > inheriting from this. Done. https://codereview.chromium.org/2396743002/diff/160001/chrome/browser/metrics... File chrome/browser/metrics/metrics_memory_details.cc (left): https://codereview.chromium.org/2396743002/diff/160001/chrome/browser/metrics... chrome/browser/metrics/metrics_memory_details.cc:62: memory_growth_tracker_ = memory_growth_tracker; Deleted as redundant. https://codereview.chromium.org/2396743002/diff/160001/chrome/browser/metrics... File chrome/browser/metrics/metrics_memory_details.cc (right): https://codereview.chromium.org/2396743002/diff/160001/chrome/browser/metrics... chrome/browser/metrics/metrics_memory_details.cc:75: AnalyzeMemoryGrowth(); On 2016/10/12 21:15:56, Simon Que wrote: > Since AnalyzeMemoryGrowth() is defined in a #if defined(OS_CHROMEOS) section, > you should put the #if defined() here too. Both should be outside this #if. Fixed.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by mwlodar@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2396743002/diff/180001/chrome/browser/metrics... File chrome/browser/metrics/leak_detector/leak_detector_controller.h (right): https://codereview.chromium.org/2396743002/diff/180001/chrome/browser/metrics... chrome/browser/metrics/leak_detector/leak_detector_controller.h:65: DISALLOW_COPY_AND_ASSIGN(TotalMemoryGrowthTracker); On 2016/10/12 21:46:12, Alexei Svitkine (slow) wrote: > Nit: Indent is wrong. > > You can run "git cl format" to fix. I did not know about this command, thanks. https://codereview.chromium.org/2396743002/diff/180001/chrome/browser/metrics... File chrome/browser/metrics/metrics_memory_details.cc (right): https://codereview.chromium.org/2396743002/diff/180001/chrome/browser/metrics... chrome/browser/metrics/metrics_memory_details.cc:59: const base::Closure& callback, MemoryGrowthTracker* memory_growth_tracker) On 2016/10/12 21:46:12, Alexei Svitkine (slow) wrote: > Nit: I think convention is 1 param per line if first param is on a new line. But > if this is what "git cl format" generates, I'm ok with this too. Done. https://codereview.chromium.org/2396743002/diff/180001/chrome/browser/metrics... chrome/browser/metrics/metrics_memory_details.cc:67: generate_histograms_(generate_histograms) {} On 2016/10/12 21:46:12, Alexei Svitkine (slow) wrote: > Hmm, instead of a separate constructor, why not just make a setter? Done. https://codereview.chromium.org/2396743002/diff/180001/chrome/browser/metrics... chrome/browser/metrics/metrics_memory_details.cc:314: for (size_t index = 0; index < browser.processes.size(); index++) { On 2016/10/12 21:46:12, Alexei Svitkine (slow) wrote: > Nit: use a C++ for loop: > > for (const auto& process_entry : GetBrowser()->processes) { > > } Done. https://codereview.chromium.org/2396743002/diff/180001/chrome/browser/metrics... chrome/browser/metrics/metrics_memory_details.cc:320: if (generate_histograms_) { Done. This part is non-trivial though and I added a comment to warn against changing the order of conditions.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by mwlodar@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from sque@chromium.org Link to the patchset: https://codereview.chromium.org/2396743002/#ps200001 (title: "Added setter for |generate_histograms_|")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Leak reports collect information about the memory usage LeakDetectorController calls the MetricsMemoryDetails to collect information about the memory usage and writes it in the received leak reports. BUG=650347 TEST=1. Modify kDefaultProbability and kDefaultNumProcessesEnabled in leak_detector_controller.cc to always launch the detector. 2. Add a dummy report in leak_detector.cc before detector->NotifyObservers() 3. Add debug info with LOG(ERROR) in leak_detector_controller.cc:OnMemoryDetailCollectionDone() 4. Launch Chromium with flag enable-features=RuntimeMemoryLeakDetector ========== to ========== Leak reports collect information about the memory usage LeakDetectorController calls the MetricsMemoryDetails to collect information about the memory usage and writes it in the received leak reports. BUG=650347 TEST=1. Modify kDefaultProbability and kDefaultNumProcessesEnabled in leak_detector_controller.cc to always launch the detector. 2. Add a dummy report in leak_detector.cc before detector->NotifyObservers() 3. Add debug info with LOG(ERROR) in leak_detector_controller.cc:OnMemoryDetailCollectionDone() 4. Launch Chromium with flag enable-features=RuntimeMemoryLeakDetector ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== Leak reports collect information about the memory usage LeakDetectorController calls the MetricsMemoryDetails to collect information about the memory usage and writes it in the received leak reports. BUG=650347 TEST=1. Modify kDefaultProbability and kDefaultNumProcessesEnabled in leak_detector_controller.cc to always launch the detector. 2. Add a dummy report in leak_detector.cc before detector->NotifyObservers() 3. Add debug info with LOG(ERROR) in leak_detector_controller.cc:OnMemoryDetailCollectionDone() 4. Launch Chromium with flag enable-features=RuntimeMemoryLeakDetector ========== to ========== Leak reports collect information about the memory usage LeakDetectorController calls the MetricsMemoryDetails to collect information about the memory usage and writes it in the received leak reports. BUG=650347 TEST=1. Modify kDefaultProbability and kDefaultNumProcessesEnabled in leak_detector_controller.cc to always launch the detector. 2. Add a dummy report in leak_detector.cc before detector->NotifyObservers() 3. Add debug info with LOG(ERROR) in leak_detector_controller.cc:OnMemoryDetailCollectionDone() 4. Launch Chromium with flag enable-features=RuntimeMemoryLeakDetector Committed: https://crrev.com/ee4005181ef90e80428b3569fd5a364f50f334e6 Cr-Commit-Position: refs/heads/master@{#425212} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/ee4005181ef90e80428b3569fd5a364f50f334e6 Cr-Commit-Position: refs/heads/master@{#425212} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
