Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2486573003:
memory coordinator: Add metrics for Android's onTrimMemory() (Closed)
Descriptionmemory coordinator: Add metrics for Android's onTrimMemory()
Record levels of onTrimMemory()[1]. Define separate histograms for each
memory state so that we can examine the behavior of the memory
coordinator. For example, the number of onTrimMemory() calls should be
smaller in NORMAL state than THROTTLED/SUSPENDED states.
[1]
https://developer.android.com/reference/android/content/ComponentCallbacks2.html#onTrimMemory(int)
Design doc:
https://docs.google.com/document/d/1Mlx7q_8thkoQtg_C7-CNA_y0SxSK0yp_ja9Zs4HmcwU/edit#heading=h.7g1az9n5ec1m
BUG=662772
Committed: https://crrev.com/d351969bdb53d2ecf8f61db2987280a826cca738
Cr-Commit-Position: refs/heads/master@{#431188}
Patch Set 1 #
Total comments: 4
Patch Set 2 : comments addressed #
Messages
Total messages: 24 (12 generated)
bashi@chromium.org changed reviewers: + chrisha@chromium.org, haraken@chromium.org
PTAL Other options would be having one histogram (called Memory.Coordinator.TrimMemoryLevel) and define an enum like: RUNNING_MODERATE_ON_NORMAL, RUNNING_LOW_ON_NORMAL, ... RUNNING_MODERATE_ON_THROTTLED, ... RUNNING_MODERATE_ON_SUSPENDED, But we have to define a lot of values with this approach. What do you think?
On 2016/11/08 09:33:10, bashi1 wrote: > PTAL > > Other options would be having one histogram (called > Memory.Coordinator.TrimMemoryLevel) and define an enum like: > > RUNNING_MODERATE_ON_NORMAL, > RUNNING_LOW_ON_NORMAL, > ... > RUNNING_MODERATE_ON_THROTTLED, > ... > RUNNING_MODERATE_ON_SUSPENDED, > > But we have to define a lot of values with this approach. What do you think? Maybe we are more interested in logging global states when we received BACKGROUND/MODERATE/COMPLETE etc? i.e., record Memory.Coordinator.MemoryStateWhenWeReceivedOnTrimMemory.Background instead of Memory.Coordinator.TrimMemoryLevel.Normal
On 2016/11/08 10:56:22, haraken wrote: > On 2016/11/08 09:33:10, bashi1 wrote: > > PTAL > > > > Other options would be having one histogram (called > > Memory.Coordinator.TrimMemoryLevel) and define an enum like: > > > > RUNNING_MODERATE_ON_NORMAL, > > RUNNING_LOW_ON_NORMAL, > > ... > > RUNNING_MODERATE_ON_THROTTLED, > > ... > > RUNNING_MODERATE_ON_SUSPENDED, > > > > But we have to define a lot of values with this approach. What do you think? > > Maybe we are more interested in logging global states when we received > BACKGROUND/MODERATE/COMPLETE etc? > > i.e., record Memory.Coordinator.MemoryStateWhenWeReceivedOnTrimMemory.Background > instead of Memory.Coordinator.TrimMemoryLevel.Normal I think they are the same? For example, followings seems the same. - count(TrimMemoryLevel.Normal == BACKGROUND) - count(MemoryStateOnTrimMemory.Background == Normal) Actually I tried something similar to your suggestion first but I gave up that approach because metrics names are much longer and we have to define many histograms.
On 2016/11/09 23:38:54, bashi1 wrote: > On 2016/11/08 10:56:22, haraken wrote: > > On 2016/11/08 09:33:10, bashi1 wrote: > > > PTAL > > > > > > Other options would be having one histogram (called > > > Memory.Coordinator.TrimMemoryLevel) and define an enum like: > > > > > > RUNNING_MODERATE_ON_NORMAL, > > > RUNNING_LOW_ON_NORMAL, > > > ... > > > RUNNING_MODERATE_ON_THROTTLED, > > > ... > > > RUNNING_MODERATE_ON_SUSPENDED, > > > > > > But we have to define a lot of values with this approach. What do you think? > > > > Maybe we are more interested in logging global states when we received > > BACKGROUND/MODERATE/COMPLETE etc? > > > > i.e., record > Memory.Coordinator.MemoryStateWhenWeReceivedOnTrimMemory.Background > > instead of Memory.Coordinator.TrimMemoryLevel.Normal > > I think they are the same? For example, followings seems the same. > > - count(TrimMemoryLevel.Normal == BACKGROUND) > - count(MemoryStateOnTrimMemory.Background == Normal) > > Actually I tried something similar to your suggestion first but I gave up that > approach because metrics names are much longer and we have to define many > histograms. LGTM They are the same but I thought we're more interested in observing memory states when we receive OnTrimMemory notifications rather than the type of OnTrimMemory notifications when we are in some memory state.
Description was changed from ========== memory coordinator: Add metrics for Android's onTrimMemory() Record levels of onTrimMemory()[1]. Define separate histograms for each memory state so that we can examine the behavior of the memory coordinator. For example, the number of onTrimMemory() calls should be smaller in NORMAL state than THROTTLED/SUSPENDED states. [1] https://developer.android.com/reference/android/content/ComponentCallbacks2.h... Design doc: https://docs.google.com/document/d/1Mlx7q_8thkoQtg_C7-CNA_y0SxSK0yp_ja9Zs4Hmc... BUG=662772 ========== to ========== memory coordinator: Add metrics for Android's onTrimMemory() Record levels of onTrimMemory()[1]. Define separate histograms for each memory state so that we can examine the behavior of the memory coordinator. For example, the number of onTrimMemory() calls should be smaller in NORMAL state than THROTTLED/SUSPENDED states. [1] https://developer.android.com/reference/android/content/ComponentCallbacks2.h... Design doc: https://docs.google.com/document/d/1Mlx7q_8thkoQtg_C7-CNA_y0SxSK0yp_ja9Zs4Hmc... BUG=662772 ==========
bashi@chromium.org changed reviewers: + rkaplow@chromium.org
On 2016/11/10 01:44:09, haraken wrote: > On 2016/11/09 23:38:54, bashi1 wrote: > > On 2016/11/08 10:56:22, haraken wrote: > > > On 2016/11/08 09:33:10, bashi1 wrote: > > > > PTAL > > > > > > > > Other options would be having one histogram (called > > > > Memory.Coordinator.TrimMemoryLevel) and define an enum like: > > > > > > > > RUNNING_MODERATE_ON_NORMAL, > > > > RUNNING_LOW_ON_NORMAL, > > > > ... > > > > RUNNING_MODERATE_ON_THROTTLED, > > > > ... > > > > RUNNING_MODERATE_ON_SUSPENDED, > > > > > > > > But we have to define a lot of values with this approach. What do you > think? > > > > > > Maybe we are more interested in logging global states when we received > > > BACKGROUND/MODERATE/COMPLETE etc? > > > > > > i.e., record > > Memory.Coordinator.MemoryStateWhenWeReceivedOnTrimMemory.Background > > > instead of Memory.Coordinator.TrimMemoryLevel.Normal > > > > I think they are the same? For example, followings seems the same. > > > > - count(TrimMemoryLevel.Normal == BACKGROUND) > > - count(MemoryStateOnTrimMemory.Background == Normal) > > > > Actually I tried something similar to your suggestion first but I gave up that > > approach because metrics names are much longer and we have to define many > > histograms. > > LGTM > > They are the same but I thought we're more interested in observing memory states > when we receive OnTrimMemory notifications rather than the type of OnTrimMemory > notifications when we are in some memory state. Yeah, the latter is intuitive but I think the former is simpler in terms of recording histograms. rkaplow@: could you review histograms.xml?
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.
On 2016/11/10 01:55:09, bashi1 wrote: > On 2016/11/10 01:44:09, haraken wrote: > > On 2016/11/09 23:38:54, bashi1 wrote: > > > On 2016/11/08 10:56:22, haraken wrote: > > > > On 2016/11/08 09:33:10, bashi1 wrote: > > > > > PTAL > > > > > > > > > > Other options would be having one histogram (called > > > > > Memory.Coordinator.TrimMemoryLevel) and define an enum like: > > > > > > > > > > RUNNING_MODERATE_ON_NORMAL, > > > > > RUNNING_LOW_ON_NORMAL, > > > > > ... > > > > > RUNNING_MODERATE_ON_THROTTLED, > > > > > ... > > > > > RUNNING_MODERATE_ON_SUSPENDED, > > > > > > > > > > But we have to define a lot of values with this approach. What do you > > think? > > > > > > > > Maybe we are more interested in logging global states when we received > > > > BACKGROUND/MODERATE/COMPLETE etc? > > > > > > > > i.e., record > > > Memory.Coordinator.MemoryStateWhenWeReceivedOnTrimMemory.Background > > > > instead of Memory.Coordinator.TrimMemoryLevel.Normal > > > > > > I think they are the same? For example, followings seems the same. > > > > > > - count(TrimMemoryLevel.Normal == BACKGROUND) > > > - count(MemoryStateOnTrimMemory.Background == Normal) > > > > > > Actually I tried something similar to your suggestion first but I gave up > that > > > approach because metrics names are much longer and we have to define many > > > histograms. > > > > LGTM > > > > They are the same but I thought we're more interested in observing memory > states > > when we receive OnTrimMemory notifications rather than the type of > OnTrimMemory > > notifications when we are in some memory state. > > Yeah, the latter is intuitive but I think the former is simpler in terms of > recording histograms. > > rkaplow@: could you review histograms.xml? As haraken@ is an OWNER of histograms.xml. Let me commit this CL.
bashi@chromium.org changed reviewers: + boliu@chromium.org
boliu@: could you review content/public/android?
lgtm https://codereview.chromium.org/2486573003/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/MemoryMonitorAndroid.java (right): https://codereview.chromium.org/2486573003/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/MemoryMonitorAndroid.java:19: class MemoryMonitorAndroid { nothing instantiates this class, right? perhaps should add a private default constructor to prevent accidents https://codereview.chromium.org/2486573003/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/MemoryMonitorAndroid.java:44: assert sCallbacks == null; fyi java asserts are pretty useless these days, since I believe asserts are disabled on art, there isn't a generic equivalent to DCHECKs in java right now :(
Thanks! Also added a comment for registerComponentCallbacks(). https://codereview.chromium.org/2486573003/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/MemoryMonitorAndroid.java (right): https://codereview.chromium.org/2486573003/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/MemoryMonitorAndroid.java:19: class MemoryMonitorAndroid { On 2016/11/10 04:06:27, boliu wrote: > nothing instantiates this class, right? perhaps should add a private default > constructor to prevent accidents Yes. Added private default constructor. https://codereview.chromium.org/2486573003/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/MemoryMonitorAndroid.java:44: assert sCallbacks == null; On 2016/11/10 04:06:27, boliu wrote: > fyi java asserts are pretty useless these days, since I believe asserts are > disabled on art, there isn't a generic equivalent to DCHECKs in java right now > :( Thanks! I didn't know that. Removed.
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, boliu@chromium.org Link to the patchset: https://codereview.chromium.org/2486573003/#ps20001 (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 ========== memory coordinator: Add metrics for Android's onTrimMemory() Record levels of onTrimMemory()[1]. Define separate histograms for each memory state so that we can examine the behavior of the memory coordinator. For example, the number of onTrimMemory() calls should be smaller in NORMAL state than THROTTLED/SUSPENDED states. [1] https://developer.android.com/reference/android/content/ComponentCallbacks2.h... Design doc: https://docs.google.com/document/d/1Mlx7q_8thkoQtg_C7-CNA_y0SxSK0yp_ja9Zs4Hmc... BUG=662772 ========== to ========== memory coordinator: Add metrics for Android's onTrimMemory() Record levels of onTrimMemory()[1]. Define separate histograms for each memory state so that we can examine the behavior of the memory coordinator. For example, the number of onTrimMemory() calls should be smaller in NORMAL state than THROTTLED/SUSPENDED states. [1] https://developer.android.com/reference/android/content/ComponentCallbacks2.h... 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 #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== memory coordinator: Add metrics for Android's onTrimMemory() Record levels of onTrimMemory()[1]. Define separate histograms for each memory state so that we can examine the behavior of the memory coordinator. For example, the number of onTrimMemory() calls should be smaller in NORMAL state than THROTTLED/SUSPENDED states. [1] https://developer.android.com/reference/android/content/ComponentCallbacks2.h... Design doc: https://docs.google.com/document/d/1Mlx7q_8thkoQtg_C7-CNA_y0SxSK0yp_ja9Zs4Hmc... BUG=662772 ========== to ========== memory coordinator: Add metrics for Android's onTrimMemory() Record levels of onTrimMemory()[1]. Define separate histograms for each memory state so that we can examine the behavior of the memory coordinator. For example, the number of onTrimMemory() calls should be smaller in NORMAL state than THROTTLED/SUSPENDED states. [1] https://developer.android.com/reference/android/content/ComponentCallbacks2.h... Design doc: https://docs.google.com/document/d/1Mlx7q_8thkoQtg_C7-CNA_y0SxSK0yp_ja9Zs4Hmc... BUG=662772 Committed: https://crrev.com/d351969bdb53d2ecf8f61db2987280a826cca738 Cr-Commit-Position: refs/heads/master@{#431188} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/d351969bdb53d2ecf8f61db2987280a826cca738 Cr-Commit-Position: refs/heads/master@{#431188} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
