Background media stream and process importance |
||||||||
Issue descriptionIf an invisible frame is playing media, for the purpose of process priority/importance, it is treated the same way as being visible. I was wondering if it makes sense to 1) separate out the signal. this would mean would no longer show up in metrics such as visible renderer oom 2) drop them from important to moderate binding. in hope of reducing sad tabs, but honestly, not sure if this is the correct move. Also while looking at this, I realized this protection probably doesn't work very well with oopif. If a subframe is has a media stream, then only the subframe is marked visible, not its ancestor frames. This makes sense on desktop, but not android, since ancestor frames are not affected. When an ancestor frame gets killed, the subframe dies along with it.
,
Aug 9
,
Aug 14
I enabled an experiment on canary and dev to disable strong binding on renderer with background media streams.
,
Aug 21
Initial experiments show very positive results on low end OOM rates: Visible renderer OOMs -30% There is a regression on "invisible renderer with moderate bindings with a similar total count compared to the reduction on renderer OOMs. The value of "Empty minidump in foreground app" has not changed much or decreased. This tells that the total number of OOMs have decreased. I would like to merge the cl to Beta to experiment on M69. I will post results of high end devices and comparison across OOPIF later since Canary data is not very useful to show statistical significance yet.
,
Aug 21
,
Aug 21
This bug requires manual review: We are only 13 days from stable. Please contact the milestone owner if you have questions. Owners: amineer@(Android), kariahda@(iOS), cindyb@(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Aug 21
Approved for merge into 69, branch 3497 based on the fact it's a trial.
,
Aug 21
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/760433e2a81c612f6f040476e7b7777f85cd806d commit 760433e2a81c612f6f040476e7b7777f85cd806d Author: Siddhartha <ssid@chromium.org> Date: Tue Aug 21 22:21:26 2018 Android: Add feature to set moderate binding for media renderer This CL adds a flag to control if the renderer with media streams should have strong or moderate binding. BUG=868611 TBR=ssid@chromium.org TBR=creis@chromium.org (cherry picked from commit fc77b8f55d86b3cdb3dd0148d8f8baa0a7521f04) Change-Id: I3263ffcca6076f9d38452c3e60df8fdd90634ea8 Reviewed-on: https://chromium-review.googlesource.com/1162927 Reviewed-by: Bo <boliu@chromium.org> Reviewed-by: Charlie Reis <creis@chromium.org> Commit-Queue: Siddhartha S <ssid@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#581809} Reviewed-on: https://chromium-review.googlesource.com/1183954 Reviewed-by: Siddhartha S <ssid@chromium.org> Cr-Commit-Position: refs/branch-heads/3497@{#748} Cr-Branched-From: 271eaf50594eb818c9295dc78d364aea18c82ea8-refs/heads/master@{#576753} [modify] https://crrev.com/760433e2a81c612f6f040476e7b7777f85cd806d/content/browser/android/content_feature_list.cc [modify] https://crrev.com/760433e2a81c612f6f040476e7b7777f85cd806d/content/browser/child_process_launcher.cc [modify] https://crrev.com/760433e2a81c612f6f040476e7b7777f85cd806d/content/browser/child_process_launcher.h [modify] https://crrev.com/760433e2a81c612f6f040476e7b7777f85cd806d/content/browser/child_process_launcher_helper_android.cc [modify] https://crrev.com/760433e2a81c612f6f040476e7b7777f85cd806d/content/browser/child_process_launcher_helper_linux.cc [modify] https://crrev.com/760433e2a81c612f6f040476e7b7777f85cd806d/content/browser/child_process_launcher_helper_mac.cc [modify] https://crrev.com/760433e2a81c612f6f040476e7b7777f85cd806d/content/browser/child_process_launcher_helper_win.cc [modify] https://crrev.com/760433e2a81c612f6f040476e7b7777f85cd806d/content/browser/renderer_host/render_process_host_impl.cc [modify] https://crrev.com/760433e2a81c612f6f040476e7b7777f85cd806d/content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncherHelperImpl.java [modify] https://crrev.com/760433e2a81c612f6f040476e7b7777f85cd806d/content/public/android/java/src/org/chromium/content/browser/ContentFeatureList.java [modify] https://crrev.com/760433e2a81c612f6f040476e7b7777f85cd806d/content/public/common/content_features.cc [modify] https://crrev.com/760433e2a81c612f6f040476e7b7777f85cd806d/content/public/common/content_features.h
,
Sep 29
ssid@: were there any findings here based on further trials? I'm curious how important this work is for launching site isolation for a subset of "important" sites, where sad tabs are less of a problem, and sad frames still happen but much less frequently than with full site isolation. It would be nice to fix the scenario you mentioned where A(B(C)) and C is playing media, so that we won't kill B, though I'm not sure how often that can happen in practice.
,
Sep 29
So, I observe no changes in any of the metrics due to this experiment. This also proves that setting moderate vs important binding on processes does not impact the memory priority of the process. Android is still equally likely to kill any of the processes. So, we cannot really do anything about this case.. Keeping it open in case we find some new priority that we could assign to processes.
,
Oct 2
Maybe we should add a uma to see how frequently the scenario in #9 happens. ie C playing music is brought down because B got killed by android.
,
Oct 5
Hmm, logging this properly is kind of difficult. First thing comes to mind is count number of times a media playing process is killed because a parent frame got oom-killed. There are a bunch of difference places that calls OnMediaStreamAdded/Removed, and it's not clear to be in that scenario, if they all will maintain the media stream count in RPH, and they don't all look to be associated with frames. Hooking back the OOM signal is pretty complicated on Android. The other problem is we don't have a good way to normalize that count. I think normalizing with page loads is too naive, since most pages don't play media. The simpler histogram to log in RenderProcessHostImpl::OnMediaStreamAdded, log aggregated the frame depth of the process. Idea is if it's not the main frame, then we know it will have a bad time if that tab goes. But that's clearly less useful for making a decision. Maybe we could have a histogram of aggregate frame depth if the process is not visible? That's probably useful enough? Other ideas?
,
Oct 5
c#12: yeah, seems reasonable to start with an UMA to least find out how often media plays in an invisible subframe and how deeply nested that frame is. Could we also check for media in subframes somewhere on the RenderFrameHostImpl::OnRenderProcessGone path()? The ResetForNewProcess() call in there [1] is what destroys all the subframe FrameTreeNodes and potentially their processes, if they aren't used by anything else. So could we potentially walk over the SubtreeNodes() prior to that call and log the UMA if a subframe's process (1) has just one active frame (i.e., the process will die when the frame dies), and (2) has a media stream. That would catch crashes in addition to OOMs, but maybe we don't care that much? [1] https://cs.chromium.org/chromium/src/content/browser/frame_host/render_frame_host_impl.cc?l=2289&rcl=9e26abc0f97228c238b7a4ec82e933e29a68430c
,
Oct 5
Ahh, that's where it is. Yeah could do that. Still don't have a good metric to normalize against though.
,
Oct 8
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/8776db0d1af37cac14ef028a23a5364c2e146eb7 commit 8776db0d1af37cac14ef028a23a5364c2e146eb7 Author: Bo Liu <boliu@chromium.org> Date: Mon Oct 08 20:28:18 2018 Record frame depth of media stream Add a metric to that approximates frame depth of frames with media stream. Bug: 868611 Change-Id: I93dd355f5c2cf6442bd29d9db89e13bb672171a7 Reviewed-on: https://chromium-review.googlesource.com/c/1265094 Reviewed-by: Alex Moshchuk <alexmos@chromium.org> Reviewed-by: Steven Holte <holte@chromium.org> Commit-Queue: Bo <boliu@chromium.org> Cr-Commit-Position: refs/heads/master@{#597667} [modify] https://crrev.com/8776db0d1af37cac14ef028a23a5364c2e146eb7/content/browser/renderer_host/render_process_host_impl.cc [modify] https://crrev.com/8776db0d1af37cac14ef028a23a5364c2e146eb7/content/browser/renderer_host/render_process_host_impl.h [modify] https://crrev.com/8776db0d1af37cac14ef028a23a5364c2e146eb7/tools/metrics/histograms/histograms.xml |
||||||||
►
Sign in to add a comment |
||||||||
Comment 1 by bugdroid1@chromium.org
, Aug 9