Overhaul MemoryUma and report pressure from child processes |
|||||||||||
Issue descriptionWe have MemoryUma class which reports MemoryAndroid.* memory pressure histograms, but does it only for the browser process. We've implemented propagation of memory pressure signals from the browser to services, and we need to know which signals Android is sending to services to decide whether we can rely exclusively on the browser signals (which also come from pressure polling). So I would like to extend MemoryUma to services, but there are issues that make me think we need to overhaul MemoryUma before using it in services. First, MemoryUma is called from AsyncInitializationActivity, so it overcounts if there are several activities around (e.g. CTA + custom tabs). Yes, activity's onTrimMemory/onLowMemory are called even if the activity is stopped. Next, the histograms are separated by the "background" status of the trim level received. I.e. all TRIM_MEMORY_* levels except for TRIM_MEMORY_RUNNING_* are reported as MemoryAndroid.NotificationBackground, and "running app" levels (TRIM_MEMORY_RUNNING_*) plus onLowMemoryCall() are reported as MemoryAndroid.NotificationForeground. This separation doesn't seem necessary, and doesn't have much sense for services (although services can receive all TRIM_MEMORY_* values). Finally, there is also MemoryAndroid.LowMemoryTimeBetween histogram, which according to description is "Time between two consecutive LowMemory notification in one foreground session". However, it's unreliable due to the fact that MemoryUma is called by all AsyncInitializationActivity activities. I'm also questioning its usefulness considering the fact that onLowMemory() is a legacy signal. Given the above I'm proposing: 1. Report histograms from ComponentCallbacks2, the same way Java MemoryPressureMonitor gets pressure signals. This will prevent overcounting issue and also will "just work" for services (and even for WebView). 2. Merge Background/Foreground histograms into a single histogram, per process type (BrowserProcess, ChildProcess). Right now one has to open them both anyway, plus current histograms are overcounted. 3. Drop (deprecate) MemoryAndroid.LowMemoryTimeBetween.
,
Apr 19 2018
,
Apr 19 2018
,
Apr 19 2018
Talked to hajimehoshi@, and he doesn't care about these metrics. In fact, he (and kouhei@) was bulk-added to a bunch of histograms to replace dmikurube@ in https://codereview.chromium.org/434343003 The metrics itself were added by ppi@ in 2013: https://chromiumcodereview.appspot.com/14143007 ppi@ was removed from histograms.xml in 2016: https://codereview.chromium.org/1920183002 So it looks like the UMA metrics are effectively unowned now.
,
Apr 19 2018
sigh, ok :) Thanks for following up
,
Apr 25 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/2153a8d73bc18f11905a0bc71308805d56672497 commit 2153a8d73bc18f11905a0bc71308805d56672497 Author: Dmitry Skiba <dskiba@chromium.org> Date: Wed Apr 25 18:11:11 2018 Overhaul MemoryUma and report pressure from services. MemoryUma class reports 3 memory pressure related metrics, but there are issues with them (see the bug), plus metrics are reported only for the browser process. We need memory pressure metrics for services too, in order to decide best ways of handling low memory situations. This CL deprecates the following metrics: * MemoryAndroid.NotificationBackground * MemoryAndroid.NotificationForeground * MemoryAndroid.LowMemoryTimeBetween and adds reporting of the following new metrics: * Android.MemoryPressureNotification.Browser * Android.MemoryPressureNotification.ChildService Bug: 834529 Change-Id: If859958ce7adeeb6431f890dea2ffd7a3de8a4f1 Reviewed-on: https://chromium-review.googlesource.com/1013077 Commit-Queue: Dmitry Skiba <dskiba@chromium.org> Reviewed-by: Yaron Friedman <yfriedman@chromium.org> Reviewed-by: Mark Pearson <mpearson@chromium.org> Cr-Commit-Position: refs/heads/master@{#553658} [modify] https://crrev.com/2153a8d73bc18f11905a0bc71308805d56672497/base/BUILD.gn [add] https://crrev.com/2153a8d73bc18f11905a0bc71308805d56672497/base/android/java/src/org/chromium/base/memory/MemoryPressureUma.java [modify] https://crrev.com/2153a8d73bc18f11905a0bc71308805d56672497/chrome/android/java/src/org/chromium/chrome/browser/init/AsyncInitializationActivity.java [modify] https://crrev.com/2153a8d73bc18f11905a0bc71308805d56672497/chrome/android/java/src/org/chromium/chrome/browser/init/ChromeBrowserInitializer.java [delete] https://crrev.com/632461086458b92777294df3874d7c28fe20d68a/chrome/android/java/src/org/chromium/chrome/browser/metrics/MemoryUma.java [modify] https://crrev.com/2153a8d73bc18f11905a0bc71308805d56672497/chrome/android/java_sources.gni [modify] https://crrev.com/2153a8d73bc18f11905a0bc71308805d56672497/content/public/android/java/src/org/chromium/content/app/ContentChildProcessServiceDelegate.java [modify] https://crrev.com/2153a8d73bc18f11905a0bc71308805d56672497/tools/metrics/histograms/enums.xml [modify] https://crrev.com/2153a8d73bc18f11905a0bc71308805d56672497/tools/metrics/histograms/histograms.xml
,
Apr 26 2018
Requesting merge of 2153a8d73bc18f11905a0bc71308805d56672497. This change is needed to monitor effectiveness of a change merged to M67 in crbug.com/813909#c53.
,
Apr 26 2018
The bug is marked as P3 or Feature. It should not be merged as M67 is in beta. Please contact the approriate milestone owner if you have questions. Owners: cmasso@(Android), cmasso@(iOS), kbleicher@(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Apr 26 2018
Bumping priority to get the change in M67.
,
Apr 26 2018
Requesting merge of 2153a8d73bc18f11905a0bc71308805d56672497. This change is needed to monitor effectiveness of a change merged to M67 in crbug.com/813909#c53.
,
Apr 26 2018
This bug requires manual review: M67 has already been promoted to the beta branch, so this requires manual review Please contact the milestone owner if you have questions. Owners: cmasso@(Android), cmasso@(iOS), kbleicher@(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
May 1 2018
Merge approved since the change is only UMA histograms related
,
May 2 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4fdedcc98c5578307e9cf4024f1b96d8d2b42bb3 commit 4fdedcc98c5578307e9cf4024f1b96d8d2b42bb3 Author: Dmitry Skiba <dskiba@chromium.org> Date: Wed May 02 17:20:21 2018 [Merge to M67] Overhaul MemoryUma and report pressure from services. MemoryUma class reports 3 memory pressure related metrics, but there are issues with them (see the bug), plus metrics are reported only for the browser process. We need memory pressure metrics for services too, in order to decide best ways of handling low memory situations. This CL deprecates the following metrics: * MemoryAndroid.NotificationBackground * MemoryAndroid.NotificationForeground * MemoryAndroid.LowMemoryTimeBetween and adds reporting of the following new metrics: * Android.MemoryPressureNotification.Browser * Android.MemoryPressureNotification.ChildService Bug: 834529 Change-Id: If859958ce7adeeb6431f890dea2ffd7a3de8a4f1 Reviewed-on: https://chromium-review.googlesource.com/1013077 Commit-Queue: Dmitry Skiba <dskiba@chromium.org> Reviewed-by: Yaron Friedman <yfriedman@chromium.org> Reviewed-by: Mark Pearson <mpearson@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#553658}(cherry picked from commit 2153a8d73bc18f11905a0bc71308805d56672497) Reviewed-on: https://chromium-review.googlesource.com/1040185 Reviewed-by: Dmitry Skiba <dskiba@chromium.org> Cr-Commit-Position: refs/branch-heads/3396@{#443} Cr-Branched-From: 9ef2aa869bc7bc0c089e255d698cca6e47d6b038-refs/heads/master@{#550428} [modify] https://crrev.com/4fdedcc98c5578307e9cf4024f1b96d8d2b42bb3/base/BUILD.gn [add] https://crrev.com/4fdedcc98c5578307e9cf4024f1b96d8d2b42bb3/base/android/java/src/org/chromium/base/memory/MemoryPressureUma.java [modify] https://crrev.com/4fdedcc98c5578307e9cf4024f1b96d8d2b42bb3/chrome/android/java/src/org/chromium/chrome/browser/init/AsyncInitializationActivity.java [modify] https://crrev.com/4fdedcc98c5578307e9cf4024f1b96d8d2b42bb3/chrome/android/java/src/org/chromium/chrome/browser/init/ChromeBrowserInitializer.java [delete] https://crrev.com/b88963d220694223619130007db1bcc18aa911ea/chrome/android/java/src/org/chromium/chrome/browser/metrics/MemoryUma.java [modify] https://crrev.com/4fdedcc98c5578307e9cf4024f1b96d8d2b42bb3/chrome/android/java_sources.gni [modify] https://crrev.com/4fdedcc98c5578307e9cf4024f1b96d8d2b42bb3/content/public/android/java/src/org/chromium/content/app/ContentChildProcessServiceDelegate.java [modify] https://crrev.com/4fdedcc98c5578307e9cf4024f1b96d8d2b42bb3/tools/metrics/histograms/enums.xml [modify] https://crrev.com/4fdedcc98c5578307e9cf4024f1b96d8d2b42bb3/tools/metrics/histograms/histograms.xml
,
May 30 2018
|
|||||||||||
►
Sign in to add a comment |
|||||||||||
Comment 1 by dskiba@chromium.org
, Apr 19 2018