Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2482783002:
Add StateOn{Moderate,Critical}NotificationReceived metrics (Closed)
DescriptionAdd StateOn{Moderate,Critical}NotificationReceived metrics
Record the global state of memory coordinator when memory pressure
notification is received. The purpose of this metrics is to make
sure the memory coordinator changes its global state to THROTTLED
or SUSPENDED before receiving memory pressure notifications.
BUG=662772
Committed: https://crrev.com/051a90313eb6f6851bb463e415d73d8556d3366d
Cr-Commit-Position: refs/heads/master@{#430517}
Patch Set 1 #Patch Set 2 : histograms.xml #
Total comments: 8
Patch Set 3 : comments addressed #Patch Set 4 : Wrap -> Make #
Total comments: 4
Patch Set 5 : comments addressed #Patch Set 6 : Typo #Patch Set 7 : Remove std::move() #
Messages
Total messages: 36 (19 generated)
The CQ bit was checked by bashi@chromium.org 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...
bashi@chromium.org changed reviewers: + chrisha@chromium.org, haraken@chromium.org
PTAL Here is a summary of this metrics: https://docs.google.com/document/d/1Mlx7q_8thkoQtg_C7-CNA_y0SxSK0yp_ja9Zs4Hmc...
LGTM https://codereview.chromium.org/2482783002/diff/20001/base/memory/memory_coor... File base/memory/memory_coordinator_client.h (right): https://codereview.chromium.org/2482783002/diff/20001/base/memory/memory_coor... base/memory/memory_coordinator_client.h:38: SUSPENDED = 2, Maybe can we add MemoryStateMax here instead of defining a const int separately?
https://codereview.chromium.org/2482783002/diff/20001/base/memory/memory_coor... File base/memory/memory_coordinator_client.h (right): https://codereview.chromium.org/2482783002/diff/20001/base/memory/memory_coor... base/memory/memory_coordinator_client.h:38: SUSPENDED = 2, On 2016/11/07 03:11:47, haraken wrote: > > Maybe can we add MemoryStateMax here instead of defining a const int separately? I did that first but found that it's a bit inconvenient. The compiler complains when switch(MemoryState) { ... } doesn't check all possible values. Many MCC use switch(MemoryState) { ... } and I didn't want to force them to check MAX.
bashi@chromium.org changed reviewers: + avi@chromium.org, dcheng@chromium.org, isherman@chromium.org
+isherman@ for tools/metrics +avi@ for content/browser +dcheng@ for base/memory
May as well fix ownership while you're here. https://codereview.chromium.org/2482783002/diff/20001/content/browser/browser... File content/browser/browser_main_loop.cc (right): https://codereview.chromium.org/2482783002/diff/20001/content/browser/browser... content/browser/browser_main_loop.cc:1406: memory_pressure_monitor_.reset(new base::chromeos::MemoryPressureMonitor( memory_pressure_monitor_ = base::MakeUnique<base::chromeos::MemoryPressureMonitor>(...) https://codereview.chromium.org/2482783002/diff/20001/content/browser/browser... content/browser/browser_main_loop.cc:1410: memory_pressure_monitor_.reset(new base::mac::MemoryPressureMonitor()); memory_pressure_monitor_ = base::MakeUnique<base::mac::MemoryPressureMonitor>(...) https://codereview.chromium.org/2482783002/diff/20001/content/browser/browser... content/browser/browser_main_loop.cc:1412: memory_pressure_monitor_.reset(CreateWinMemoryPressureMonitor( Fix CreateWinMemoryPressureMonitor so that it returns a unique_ptr and assign it.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2482783002/diff/20001/content/browser/browser... File content/browser/browser_main_loop.cc (right): https://codereview.chromium.org/2482783002/diff/20001/content/browser/browser... content/browser/browser_main_loop.cc:1406: memory_pressure_monitor_.reset(new base::chromeos::MemoryPressureMonitor( On 2016/11/07 04:32:01, Avi wrote: > memory_pressure_monitor_ = > base::MakeUnique<base::chromeos::MemoryPressureMonitor>(...) Done. https://codereview.chromium.org/2482783002/diff/20001/content/browser/browser... content/browser/browser_main_loop.cc:1410: memory_pressure_monitor_.reset(new base::mac::MemoryPressureMonitor()); On 2016/11/07 04:32:01, Avi wrote: > memory_pressure_monitor_ = > base::MakeUnique<base::mac::MemoryPressureMonitor>(...) Done. https://codereview.chromium.org/2482783002/diff/20001/content/browser/browser... content/browser/browser_main_loop.cc:1412: memory_pressure_monitor_.reset(CreateWinMemoryPressureMonitor( On 2016/11/07 04:32:01, Avi wrote: > Fix CreateWinMemoryPressureMonitor so that it returns a unique_ptr and assign > it. Done.
lgtm Thank you for the cleanup! 👍
Metrics LGTM % nits. Thanks! https://codereview.chromium.org/2482783002/diff/60001/base/memory/memory_coor... File base/memory/memory_coordinator_client.h (right): https://codereview.chromium.org/2482783002/diff/60001/base/memory/memory_coor... base/memory/memory_coordinator_client.h:24: // state can use that as as signal to drop memory caches. Please document that this enum is used to back an UMA histogram, and therefore should be treated as append-only. https://codereview.chromium.org/2482783002/diff/60001/content/browser/memory/... File content/browser/memory/memory_coordinator.cc (right): https://codereview.chromium.org/2482783002/diff/60001/content/browser/memory/... content/browser/memory/memory_coordinator.cc:140: default: nit: It's generally best to omit the default case, and instead to list each possible case explicitly. That way, if a new enum constant is ever added, the author will need to make a conscious decision for how to handle it here.
base lgtm
Thank you all for reviews! https://codereview.chromium.org/2482783002/diff/60001/base/memory/memory_coor... File base/memory/memory_coordinator_client.h (right): https://codereview.chromium.org/2482783002/diff/60001/base/memory/memory_coor... base/memory/memory_coordinator_client.h:24: // state can use that as as signal to drop memory caches. On 2016/11/07 19:28:51, Ilya Sherman wrote: > Please document that this enum is used to back an UMA histogram, and therefore > should be treated as append-only. Done. https://codereview.chromium.org/2482783002/diff/60001/content/browser/memory/... File content/browser/memory/memory_coordinator.cc (right): https://codereview.chromium.org/2482783002/diff/60001/content/browser/memory/... content/browser/memory/memory_coordinator.cc:140: default: On 2016/11/07 19:28:52, Ilya Sherman wrote: > nit: It's generally best to omit the default case, and instead to list each > possible case explicitly. That way, if a new enum constant is ever added, the > author will need to make a conscious decision for how to handle it here. Done.
The CQ bit was checked by bashi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, avi@chromium.org, isherman@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2482783002/#ps80001 (title: "comments addressed")
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
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-...) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
The CQ bit was checked by bashi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, avi@chromium.org, isherman@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2482783002/#ps100001 (title: "Typo")
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
Try jobs failed on following builders: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
The CQ bit was checked by bashi@chromium.org 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.
The CQ bit was checked by bashi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, avi@chromium.org, isherman@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2482783002/#ps120001 (title: "Remove std::move()")
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.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from
==========
Add StateOn{Moderate,Critical}NotificationReceived metrics
Record the global state of memory coordinator when memory pressure
notification is received. The purpose of this metrics is to make
sure the memory coordinator changes its global state to THROTTLED
or SUSPENDED before receiving memory pressure notifications.
BUG=662772
==========
to
==========
Add StateOn{Moderate,Critical}NotificationReceived metrics
Record the global state of memory coordinator when memory pressure
notification is received. The purpose of this metrics is to make
sure the memory coordinator changes its global state to THROTTLED
or SUSPENDED before receiving memory pressure notifications.
BUG=662772
Committed: https://crrev.com/051a90313eb6f6851bb463e415d73d8556d3366d
Cr-Commit-Position: refs/heads/master@{#430517}
==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/051a90313eb6f6851bb463e415d73d8556d3366d Cr-Commit-Position: refs/heads/master@{#430517} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
