Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2476223002:
Add metrics for memory coordinator global state change (Closed)
DescriptionAdd metrics for memory coordinator global state change
Record some metrics when the memory coordinator changes its global
state. Added metrics are:
- Total private working set memory of the browser/renderers
- Elapsed time between state changes
Design doc: https://docs.google.com/document/d/1Mlx7q_8thkoQtg_C7-CNA_y0SxSK0yp_ja9Zs4HmcwU/edit?usp=sharing
BUG=662772
Committed: https://crrev.com/3e7b2c2e9ad184a7e541f3eb1a30b9db34408f7a
Cr-Commit-Position: refs/heads/master@{#431709}
Patch Set 1 #Patch Set 2 : more metrics #
Total comments: 3
Patch Set 3 : Update histograms.xml #Patch Set 4 : Dont count private memory on mac #
Total comments: 4
Patch Set 5 : comments addressed #
Messages
Total messages: 29 (14 generated)
Description was changed from ========== WIP: Add metrics for memory coordinator global state change Record some metrics when the memory coordinator changes its global state. Added metrics are: - Total private working set memory of the browser/renderers - Number of state changes BUG=662772 ========== to ========== WIP: Add metrics for memory coordinator global state change Record some metrics when the memory coordinator changes its global state. Added metrics are: - Total private working set memory of the browser/renderers - Elapsed time between state changes - Number of state changes BUG=662772 ==========
Description was changed from ========== WIP: Add metrics for memory coordinator global state change Record some metrics when the memory coordinator changes its global state. Added metrics are: - Total private working set memory of the browser/renderers - Elapsed time between state changes - Number of state changes BUG=662772 ========== to ========== WIP: Add metrics for memory coordinator global state change Record some metrics when the memory coordinator changes its global state. Added metrics are: - Total private working set memory of the browser/renderers - Elapsed time between state changes - Number of state changes Design doc: https://docs.google.com/document/d/1Mlx7q_8thkoQtg_C7-CNA_y0SxSK0yp_ja9Zs4Hmc... BUG=662772 ==========
bashi@chromium.org changed reviewers: + chrisha@chromium.org, haraken@chromium.org
PTAL What do you think about adding these metrics? If this looks OK, I'll update histograms.xml.
https://codereview.chromium.org/2476223002/diff/20001/content/browser/memory/... File content/browser/memory/memory_coordinator_impl.cc (right): https://codereview.chromium.org/2476223002/diff/20001/content/browser/memory/... content/browser/memory/memory_coordinator_impl.cc:287: size_t total_private_kb = 0; I don't count shared memory as it will be double-counted when we simply add them. Memory.Total2 also uses private memory only IIUC.
Looks good. https://codereview.chromium.org/2476223002/diff/20001/content/browser/memory/... File content/browser/memory/memory_coordinator_impl.cc (right): https://codereview.chromium.org/2476223002/diff/20001/content/browser/memory/... content/browser/memory/memory_coordinator_impl.cc:287: size_t total_private_kb = 0; On 2016/11/07 10:32:13, bashi1 wrote: > I don't count shared memory as it will be double-counted when we simply add > them. Memory.Total2 also uses private memory only IIUC. Is there any way to count the sum of PSS? (which will not double-count shared memory)
https://codereview.chromium.org/2476223002/diff/20001/content/browser/memory/... File content/browser/memory/memory_coordinator_impl.cc (right): https://codereview.chromium.org/2476223002/diff/20001/content/browser/memory/... content/browser/memory/memory_coordinator_impl.cc:287: size_t total_private_kb = 0; On 2016/11/07 10:36:00, haraken wrote: > On 2016/11/07 10:32:13, bashi1 wrote: > > I don't count shared memory as it will be double-counted when we simply add > > them. Memory.Total2 also uses private memory only IIUC. > > Is there any way to count the sum of PSS? (which will not double-count shared > memory) On Linux, yes. I'm not sure it's possible on Win and Mac as PSS is Linux-specific notion.
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...
Description was changed from ========== WIP: Add metrics for memory coordinator global state change Record some metrics when the memory coordinator changes its global state. Added metrics are: - Total private working set memory of the browser/renderers - Elapsed time between state changes - Number of state changes Design doc: https://docs.google.com/document/d/1Mlx7q_8thkoQtg_C7-CNA_y0SxSK0yp_ja9Zs4Hmc... BUG=662772 ========== to ========== Add metrics for memory coordinator global state change Record some metrics when the memory coordinator changes its global state. Added metrics are: - Total private working set memory of the browser/renderers - Elapsed time between state changes - Number of state changes Design doc: https://docs.google.com/document/d/1Mlx7q_8thkoQtg_C7-CNA_y0SxSK0yp_ja9Zs4Hmc... BUG=662772 ==========
Updated histograms.xml and I think this CL is ready to review. PTAL?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
LGTM
bashi@chromium.org changed reviewers: + isherman@chromium.org
isherman@: could you review histograms.xml? According to https://chromium.googlesource.com/chromium/src.git/+/HEAD/tools/metrics/histo..., an enum is a typical workaround for multi-dimensional histograms but it seems it doesn't work well for our purpose. We want to record some data (e.g. memory and duration) when a state is changed (where state = {Normal,Throttled,Suspended}). What's why I added many metrics.
isherman@: Friendly ping. (Please let me know if this approach isn't good and there is a better way to record memory and elapsed time when state is changed.)
https://codereview.chromium.org/2476223002/diff/50001/content/browser/memory/... File content/browser/memory/memory_coordinator_impl.cc (right): https://codereview.chromium.org/2476223002/diff/50001/content/browser/memory/... content/browser/memory/memory_coordinator_impl.cc:66: UMA_HISTOGRAM_COUNTS_1000("Memory.Coordinator." group ".Count", 1) You're currently always recording to the same bucket within this histogram. What's the purpose of this histogram, and how does it differ from just looking at the total count for either of the other two histograms? https://codereview.chromium.org/2476223002/diff/50001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2476223002/diff/50001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:25682: +</histogram> Could you please use a histogram_suffixes element in this file to reduce the amount of repetition?
And, sorry for the code review delay!
Thank you for review! https://codereview.chromium.org/2476223002/diff/50001/content/browser/memory/... File content/browser/memory/memory_coordinator_impl.cc (right): https://codereview.chromium.org/2476223002/diff/50001/content/browser/memory/... content/browser/memory/memory_coordinator_impl.cc:66: UMA_HISTOGRAM_COUNTS_1000("Memory.Coordinator." group ".Count", 1) On 2016/11/11 00:00:38, Ilya Sherman wrote: > You're currently always recording to the same bucket within this histogram. > What's the purpose of this histogram, and how does it differ from just looking > at the total count for either of the other two histograms? The purpose of this histogram is to understand how many times the global state is changed, and you are right I can count the total of either of the other two histograms to see that. Removed. https://codereview.chromium.org/2476223002/diff/50001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2476223002/diff/50001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:25682: +</histogram> On 2016/11/11 00:00:38, Ilya Sherman wrote: > Could you please use a histogram_suffixes element in this file to reduce the > amount of repetition? I didn't aware that histogram_suffixes exists. Thanks for the info! I rearranged histogram names to reduce the dups.
Description was changed from ========== Add metrics for memory coordinator global state change Record some metrics when the memory coordinator changes its global state. Added metrics are: - Total private working set memory of the browser/renderers - Elapsed time between state changes - Number of state changes Design doc: https://docs.google.com/document/d/1Mlx7q_8thkoQtg_C7-CNA_y0SxSK0yp_ja9Zs4Hmc... BUG=662772 ========== to ========== Add metrics for memory coordinator global state change Record some metrics when the memory coordinator changes its global state. Added metrics are: - Total private working set memory of the browser/renderers - Elapsed time between state changes Design doc: https://docs.google.com/document/d/1Mlx7q_8thkoQtg_C7-CNA_y0SxSK0yp_ja9Zs4Hmc... BUG=662772 ==========
metrics lgtm, thanks
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 Link to the patchset: https://codereview.chromium.org/2476223002/#ps70001 (title: "comments addressed")
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 ========== Add metrics for memory coordinator global state change Record some metrics when the memory coordinator changes its global state. Added metrics are: - Total private working set memory of the browser/renderers - Elapsed time between state changes Design doc: https://docs.google.com/document/d/1Mlx7q_8thkoQtg_C7-CNA_y0SxSK0yp_ja9Zs4Hmc... BUG=662772 ========== to ========== Add metrics for memory coordinator global state change Record some metrics when the memory coordinator changes its global state. Added metrics are: - Total private working set memory of the browser/renderers - Elapsed time between state changes Design doc: https://docs.google.com/document/d/1Mlx7q_8thkoQtg_C7-CNA_y0SxSK0yp_ja9Zs4Hmc... BUG=662772 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:70001)
Message was sent while issue was closed.
Description was changed from ========== Add metrics for memory coordinator global state change Record some metrics when the memory coordinator changes its global state. Added metrics are: - Total private working set memory of the browser/renderers - Elapsed time between state changes Design doc: https://docs.google.com/document/d/1Mlx7q_8thkoQtg_C7-CNA_y0SxSK0yp_ja9Zs4Hmc... BUG=662772 ========== to ========== Add metrics for memory coordinator global state change Record some metrics when the memory coordinator changes its global state. Added metrics are: - Total private working set memory of the browser/renderers - Elapsed time between state changes Design doc: https://docs.google.com/document/d/1Mlx7q_8thkoQtg_C7-CNA_y0SxSK0yp_ja9Zs4Hmc... BUG=662772 Committed: https://crrev.com/3e7b2c2e9ad184a7e541f3eb1a30b9db34408f7a Cr-Commit-Position: refs/heads/master@{#431709} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/3e7b2c2e9ad184a7e541f3eb1a30b9db34408f7a Cr-Commit-Position: refs/heads/master@{#431709} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
