Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1214193006:
Delay cleanup of moderate bindings when the app goes to background (Closed)
|
Created:
5 years, 5 months ago by Jaekyun Seok (inactive) Modified:
5 years, 5 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDelay cleanup of moderate bindings when the app goes to background
For now, in the document mode switching tabs always releases all the
moderate bindings because the app goes into the background when launching
the Recents to select a tab.
To relieve this problem, this CL delays cleanup of moderate bindings by 10s.
BUG=485867
Committed: https://crrev.com/3752b44fbb4a8849b245d2946925b00ce83d1541
Cr-Commit-Position: refs/heads/master@{#337928}
Patch Set 1 #
Total comments: 14
Patch Set 2 : Apply Yaron's comments #
Messages
Total messages: 20 (4 generated)
jaekyun@chromium.org changed reviewers: + mariakhomenko@chromium.org, yfriedman@chromium.org
mariakhomenko@ and yfriedman@, please review this CL.
yfriedman@chromium.org changed reviewers: + boliu@chromium.org
https://codereview.chromium.org/1214193006/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java (right): https://codereview.chromium.org/1214193006/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java:36: private static final long MODERATE_BINDING_POOL_CLEARER_DELAY_MILLIS = 10 * 1000; How'd you choose 10 second vs 1 second above for high-end binding drop? Does this mean that after 1 second the one foreground renderer is equally as likely to be discarded as background tabs (because they're all left only with moderate bindings)? Isn't it more likely actually given that we may not have dropped caches and it's therefore using more memory than background tabs? https://codereview.chromium.org/1214193006/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java:50: private final Handler mHandler = new Handler(Looper.getMainLooper()); As this is in content (i.e. use by webview) I think you should use ThreadUtils.getUiThreadHandler(). +bo https://codereview.chromium.org/1214193006/diff/1/content/public/android/java... File content/public/android/javatests/src/org/chromium/content/browser/BindingManagerImplTest.java (right): https://codereview.chromium.org/1214193006/diff/1/content/public/android/java... content/public/android/javatests/src/org/chromium/content/browser/BindingManagerImplTest.java:462: * Verifies that onSentToBackground() drops all the moderate bindings in some delay, and s/in/after https://codereview.chromium.org/1214193006/diff/1/content/public/android/java... content/public/android/javatests/src/org/chromium/content/browser/BindingManagerImplTest.java:502: for (MockChildProcessConnection connection : connections) { you should probably sleep before this to ensure that cancellation happened. Otherwise you aren't really testing the logic since this is still running before the timeout.
I actually think this is the wrong approach. I think we should instead fix the session tracking in document mode. Having it correctly mark the session as ended will also help us with UMA stats for document mode sessions, which are currently a mess.
https://codereview.chromium.org/1214193006/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java (right): https://codereview.chromium.org/1214193006/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java:50: private final Handler mHandler = new Handler(Looper.getMainLooper()); On 2015/07/06 15:50:11, Yaron wrote: > As this is in content (i.e. use by webview) I think you should use > ThreadUtils.getUiThreadHandler(). > > +bo Yep, or just postOnUiThreadDelayed
Message was sent while issue was closed.
On 2015/07/06 18:05:10, boliu wrote: > https://codereview.chromium.org/1214193006/diff/1/content/public/android/java... > File > content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java > (right): > > https://codereview.chromium.org/1214193006/diff/1/content/public/android/java... > content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java:50: > private final Handler mHandler = new Handler(Looper.getMainLooper()); > On 2015/07/06 15:50:11, Yaron wrote: > > As this is in content (i.e. use by webview) I think you should use > > ThreadUtils.getUiThreadHandler(). > > > > +bo > > Yep, or just postOnUiThreadDelayed I closed this CL because the session tracking problem in document mode will be fixed separately.
On 2015/07/06 23:16:53, Jaekyun Seok wrote: > On 2015/07/06 18:05:10, boliu wrote: > > > https://codereview.chromium.org/1214193006/diff/1/content/public/android/java... > > File > > > content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java > > (right): > > > > > https://codereview.chromium.org/1214193006/diff/1/content/public/android/java... > > > content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java:50: > > private final Handler mHandler = new Handler(Looper.getMainLooper()); > > On 2015/07/06 15:50:11, Yaron wrote: > > > As this is in content (i.e. use by webview) I think you should use > > > ThreadUtils.getUiThreadHandler(). > > > > > > +bo > > > > Yep, or just postOnUiThreadDelayed > > I closed this CL because the session tracking problem in document mode will be > fixed separately. I re-opened this CL because Maria agreed to go with this.
jaekyun@chromium.org changed reviewers: + ppi@chromium.org
PTAL. https://codereview.chromium.org/1214193006/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java (right): https://codereview.chromium.org/1214193006/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java:36: private static final long MODERATE_BINDING_POOL_CLEARER_DELAY_MILLIS = 10 * 1000; On 2015/07/06 15:50:11, Yaron wrote: > How'd you choose 10 second vs 1 second above for high-end binding drop? Does > this mean that after 1 second the one foreground renderer is equally as likely > to be discarded as background tabs (because they're all left only with moderate > bindings)? Isn't it more likely actually given that we may not have dropped > caches and it's therefore using more memory than background tabs? I don't know details about DETACH_AS_ACTIVE_HIGH_END_DELAY_MILLIS, but it is being used as a delay when removing strong binding on a high-end device; it isn't related at all with moderate bindings. ppi@, could you please let us why a delay is needed when removing strong binding? https://codereview.chromium.org/1214193006/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java:50: private final Handler mHandler = new Handler(Looper.getMainLooper()); On 2015/07/06 18:05:10, boliu wrote: > On 2015/07/06 15:50:11, Yaron wrote: > > As this is in content (i.e. use by webview) I think you should use > > ThreadUtils.getUiThreadHandler(). > > > > +bo > > Yep, or just postOnUiThreadDelayed ThreadUtils.getUiThreadHandler() isn't accessible, and so instead I will use ThreadUtils.getUiThreadLooper() to get Looper. https://codereview.chromium.org/1214193006/diff/1/content/public/android/java... File content/public/android/javatests/src/org/chromium/content/browser/BindingManagerImplTest.java (right): https://codereview.chromium.org/1214193006/diff/1/content/public/android/java... content/public/android/javatests/src/org/chromium/content/browser/BindingManagerImplTest.java:462: * Verifies that onSentToBackground() drops all the moderate bindings in some delay, and On 2015/07/06 15:50:11, Yaron wrote: > s/in/after Done. https://codereview.chromium.org/1214193006/diff/1/content/public/android/java... content/public/android/javatests/src/org/chromium/content/browser/BindingManagerImplTest.java:502: for (MockChildProcessConnection connection : connections) { On 2015/07/06 15:50:11, Yaron wrote: > you should probably sleep before this to ensure that cancellation happened. > Otherwise you aren't really testing the logic since this is still running before > the timeout. Done.
On 2015/07/07 06:36:06, Jaekyun Seok wrote: > PTAL. > > https://codereview.chromium.org/1214193006/diff/1/content/public/android/java... > File > content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java > (right): > > https://codereview.chromium.org/1214193006/diff/1/content/public/android/java... > content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java:36: > private static final long MODERATE_BINDING_POOL_CLEARER_DELAY_MILLIS = 10 * > 1000; > On 2015/07/06 15:50:11, Yaron wrote: > > How'd you choose 10 second vs 1 second above for high-end binding drop? Does > > this mean that after 1 second the one foreground renderer is equally as likely > > to be discarded as background tabs (because they're all left only with > moderate > > bindings)? Isn't it more likely actually given that we may not have dropped > > caches and it's therefore using more memory than background tabs? > > I don't know details about DETACH_AS_ACTIVE_HIGH_END_DELAY_MILLIS, but it is > being used as a delay when removing strong binding on a high-end device; it > isn't related at all with moderate bindings. > > ppi@, could you please let us why a delay is needed when removing strong > binding? > > https://codereview.chromium.org/1214193006/diff/1/content/public/android/java... > content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java:50: > private final Handler mHandler = new Handler(Looper.getMainLooper()); > On 2015/07/06 18:05:10, boliu wrote: > > On 2015/07/06 15:50:11, Yaron wrote: > > > As this is in content (i.e. use by webview) I think you should use > > > ThreadUtils.getUiThreadHandler(). > > > > > > +bo > > > > Yep, or just postOnUiThreadDelayed > > ThreadUtils.getUiThreadHandler() isn't accessible, and so instead I will use > ThreadUtils.getUiThreadLooper() to get Looper. > > https://codereview.chromium.org/1214193006/diff/1/content/public/android/java... > File > content/public/android/javatests/src/org/chromium/content/browser/BindingManagerImplTest.java > (right): > > https://codereview.chromium.org/1214193006/diff/1/content/public/android/java... > content/public/android/javatests/src/org/chromium/content/browser/BindingManagerImplTest.java:462: > * Verifies that onSentToBackground() drops all the moderate bindings in some > delay, and > On 2015/07/06 15:50:11, Yaron wrote: > > s/in/after > > Done. > > https://codereview.chromium.org/1214193006/diff/1/content/public/android/java... > content/public/android/javatests/src/org/chromium/content/browser/BindingManagerImplTest.java:502: > for (MockChildProcessConnection connection : connections) { > On 2015/07/06 15:50:11, Yaron wrote: > > you should probably sleep before this to ensure that cancellation happened. > > Otherwise you aren't really testing the logic since this is still running > before > > the timeout. > > Done. Sorry for typos. ppi@, could you please let us know why a delay is needed when removing strong binding?
https://codereview.chromium.org/1214193006/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java (right): https://codereview.chromium.org/1214193006/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java:36: private static final long MODERATE_BINDING_POOL_CLEARER_DELAY_MILLIS = 10 * 1000; On 2015/07/07 06:36:06, Jaekyun Seok wrote: > On 2015/07/06 15:50:11, Yaron wrote: > > How'd you choose 10 second vs 1 second above for high-end binding drop? Does > > this mean that after 1 second the one foreground renderer is equally as likely > > to be discarded as background tabs (because they're all left only with > moderate > > bindings)? Isn't it more likely actually given that we may not have dropped > > caches and it's therefore using more memory than background tabs? > > I don't know details about DETACH_AS_ACTIVE_HIGH_END_DELAY_MILLIS, but it is > being used as a delay when removing strong binding on a high-end device; it > isn't related at all with moderate bindings. > I know, but my concern is with the interaction of the two. > ppi@, could you please let us why a delay is needed when removing strong > binding? I think we needed it for two reasons: 1) user mis-clicks and immediately presses back 2) some clicks don't actually lead to a navigation. e.g. you click on a download link, it temporarily navigates, triggers the navigate and then goes back https://codereview.chromium.org/1214193006/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java:50: private final Handler mHandler = new Handler(Looper.getMainLooper()); On 2015/07/07 06:36:06, Jaekyun Seok wrote: > On 2015/07/06 18:05:10, boliu wrote: > > On 2015/07/06 15:50:11, Yaron wrote: > > > As this is in content (i.e. use by webview) I think you should use > > > ThreadUtils.getUiThreadHandler(). > > > > > > +bo > > > > Yep, or just postOnUiThreadDelayed > > ThreadUtils.getUiThreadHandler() isn't accessible, and so instead I will use > ThreadUtils.getUiThreadLooper() to get Looper. But you don't need mHandler. As Bo said, in onSentToBackground you can just use ThreadUtils.postOnUiThreadDelayed
https://codereview.chromium.org/1214193006/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java (right): https://codereview.chromium.org/1214193006/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java:50: private final Handler mHandler = new Handler(Looper.getMainLooper()); On 2015/07/07 17:39:53, Yaron wrote: > On 2015/07/07 06:36:06, Jaekyun Seok wrote: > > On 2015/07/06 18:05:10, boliu wrote: > > > On 2015/07/06 15:50:11, Yaron wrote: > > > > As this is in content (i.e. use by webview) I think you should use > > > > ThreadUtils.getUiThreadHandler(). > > > > > > > > +bo > > > > > > Yep, or just postOnUiThreadDelayed > > > > ThreadUtils.getUiThreadHandler() isn't accessible, and so instead I will use > > ThreadUtils.getUiThreadLooper() to get Looper. > > But you don't need mHandler. As Bo said, in onSentToBackground you can just use > ThreadUtils.postOnUiThreadDelayed I think problem is that postOnUiThreadDelayed doesn't support handler.removeCallbacks. I assume that's important
On 2015/07/07 17:48:34, boliu wrote: > https://codereview.chromium.org/1214193006/diff/1/content/public/android/java... > File > content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java > (right): > > https://codereview.chromium.org/1214193006/diff/1/content/public/android/java... > content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java:50: > private final Handler mHandler = new Handler(Looper.getMainLooper()); > On 2015/07/07 17:39:53, Yaron wrote: > > On 2015/07/07 06:36:06, Jaekyun Seok wrote: > > > On 2015/07/06 18:05:10, boliu wrote: > > > > On 2015/07/06 15:50:11, Yaron wrote: > > > > > As this is in content (i.e. use by webview) I think you should use > > > > > ThreadUtils.getUiThreadHandler(). > > > > > > > > > > +bo > > > > > > > > Yep, or just postOnUiThreadDelayed > > > > > > ThreadUtils.getUiThreadHandler() isn't accessible, and so instead I will use > > > ThreadUtils.getUiThreadLooper() to get Looper. > > > > But you don't need mHandler. As Bo said, in onSentToBackground you can just > use > > ThreadUtils.postOnUiThreadDelayed > > I think problem is that postOnUiThreadDelayed doesn't support > handler.removeCallbacks. I assume that's important oops. thanks
https://codereview.chromium.org/1214193006/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java (right): https://codereview.chromium.org/1214193006/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java:36: private static final long MODERATE_BINDING_POOL_CLEARER_DELAY_MILLIS = 10 * 1000; On 2015/07/07 17:39:53, Yaron wrote: > On 2015/07/07 06:36:06, Jaekyun Seok wrote: > > On 2015/07/06 15:50:11, Yaron wrote: > > > How'd you choose 10 second vs 1 second above for high-end binding drop? Does > > > this mean that after 1 second the one foreground renderer is equally as > likely > > > to be discarded as background tabs (because they're all left only with > > moderate > > > bindings)? Isn't it more likely actually given that we may not have dropped > > > caches and it's therefore using more memory than background tabs? > > > > I don't know details about DETACH_AS_ACTIVE_HIGH_END_DELAY_MILLIS, but it is > > being used as a delay when removing strong binding on a high-end device; it > > isn't related at all with moderate bindings. > > > I know, but my concern is with the interaction of the two. Actually there is no interaction between them because the one foreground renderer keeps its strong binding even when onSentToBackground() is called. That is because it is marked as the last foreground renderer and so its strong binding is kept. The behavior is being tested on BindingManagerImplTest. > > > ppi@, could you please let us why a delay is needed when removing strong > > binding? > > I think we needed it for two reasons: > 1) user mis-clicks and immediately presses back > 2) some clicks don't actually lead to a navigation. e.g. you click on a download > link, it temporarily navigates, triggers the navigate and then goes back https://codereview.chromium.org/1214193006/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java:50: private final Handler mHandler = new Handler(Looper.getMainLooper()); On 2015/07/07 17:48:34, boliu wrote: > On 2015/07/07 17:39:53, Yaron wrote: > > On 2015/07/07 06:36:06, Jaekyun Seok wrote: > > > On 2015/07/06 18:05:10, boliu wrote: > > > > On 2015/07/06 15:50:11, Yaron wrote: > > > > > As this is in content (i.e. use by webview) I think you should use > > > > > ThreadUtils.getUiThreadHandler(). > > > > > > > > > > +bo > > > > > > > > Yep, or just postOnUiThreadDelayed > > > > > > ThreadUtils.getUiThreadHandler() isn't accessible, and so instead I will use > > > ThreadUtils.getUiThreadLooper() to get Looper. > > > > But you don't need mHandler. As Bo said, in onSentToBackground you can just > use > > ThreadUtils.postOnUiThreadDelayed > > I think problem is that postOnUiThreadDelayed doesn't support > handler.removeCallbacks. I assume that's important Right. So I used ThreadUtils.getUiThreadLooper().
On 2015/07/07 22:53:05, Jaekyun Seok wrote: > https://codereview.chromium.org/1214193006/diff/1/content/public/android/java... > File > content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java > (right): > > https://codereview.chromium.org/1214193006/diff/1/content/public/android/java... > content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java:36: > private static final long MODERATE_BINDING_POOL_CLEARER_DELAY_MILLIS = 10 * > 1000; > On 2015/07/07 17:39:53, Yaron wrote: > > On 2015/07/07 06:36:06, Jaekyun Seok wrote: > > > On 2015/07/06 15:50:11, Yaron wrote: > > > > How'd you choose 10 second vs 1 second above for high-end binding drop? > Does > > > > this mean that after 1 second the one foreground renderer is equally as > > likely > > > > to be discarded as background tabs (because they're all left only with > > > moderate > > > > bindings)? Isn't it more likely actually given that we may not have > dropped > > > > caches and it's therefore using more memory than background tabs? > > > > > > I don't know details about DETACH_AS_ACTIVE_HIGH_END_DELAY_MILLIS, but it is > > > being used as a delay when removing strong binding on a high-end device; it > > > isn't related at all with moderate bindings. > > > > > I know, but my concern is with the interaction of the two. > > Actually there is no interaction between them because the one foreground > renderer keeps its strong binding even when onSentToBackground() is called. > > That is because it is marked as the last foreground renderer and so its strong > binding is kept. > > The behavior is being tested on BindingManagerImplTest. > > > > > > ppi@, could you please let us why a delay is needed when removing strong > > > binding? > > > > I think we needed it for two reasons: > > 1) user mis-clicks and immediately presses back > > 2) some clicks don't actually lead to a navigation. e.g. you click on a > download > > link, it temporarily navigates, triggers the navigate and then goes back > > https://codereview.chromium.org/1214193006/diff/1/content/public/android/java... > content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java:50: > private final Handler mHandler = new Handler(Looper.getMainLooper()); > On 2015/07/07 17:48:34, boliu wrote: > > On 2015/07/07 17:39:53, Yaron wrote: > > > On 2015/07/07 06:36:06, Jaekyun Seok wrote: > > > > On 2015/07/06 18:05:10, boliu wrote: > > > > > On 2015/07/06 15:50:11, Yaron wrote: > > > > > > As this is in content (i.e. use by webview) I think you should use > > > > > > ThreadUtils.getUiThreadHandler(). > > > > > > > > > > > > +bo > > > > > > > > > > Yep, or just postOnUiThreadDelayed > > > > > > > > ThreadUtils.getUiThreadHandler() isn't accessible, and so instead I will > use > > > > ThreadUtils.getUiThreadLooper() to get Looper. > > > > > > But you don't need mHandler. As Bo said, in onSentToBackground you can just > > use > > > ThreadUtils.postOnUiThreadDelayed > > > > I think problem is that postOnUiThreadDelayed doesn't support > > handler.removeCallbacks. I assume that's important > > Right. So I used ThreadUtils.getUiThreadLooper(). lgtm
The CQ bit was checked by jaekyun@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1214193006/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/3752b44fbb4a8849b245d2946925b00ce83d1541 Cr-Commit-Position: refs/heads/master@{#337928} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
