Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2933503002:
Add IsCurrentlyLowMemory (Closed)
DescriptionAdd IsCurrentlyLowMemory
IsCurrentlyLowMemory returns Android's MemoryInfo.lowMemory
This will be used as a GC triggering signal for low end devices.
BUG=732664
Review-Url: https://codereview.chromium.org/2933503002
Cr-Commit-Position: refs/heads/master@{#480737}
Committed: https://chromium.googlesource.com/chromium/src/+/ee6d2bccc4bdd982803f7c475f61a54675f4d555
Patch Set 1 #Patch Set 2 : fix #Patch Set 3 : fix #
Total comments: 2
Patch Set 4 : fix #
Total comments: 2
Patch Set 5 : fix #
Total comments: 3
Patch Set 6 : changed to private #
Total comments: 2
Patch Set 7 : added comment #
Messages
Total messages: 37 (13 generated)
keishi@chromium.org changed reviewers: + bashi@chromium.org
I plan to use this as a signal to trigger a page navigation GC. Design doc here: https://docs.google.com/document/d/1vJKPuMbJ5CQiFwrkxvTrB-x25SKF22-ui42oExvwp... bashi@ do you have a better idea on how I should bring this method into WTF?
On 2017/06/09 05:20:40, keishi wrote: > I plan to use this as a signal to trigger a page navigation GC. Design doc here: > https://docs.google.com/document/d/1vJKPuMbJ5CQiFwrkxvTrB-x25SKF22-ui42oExvwp... > > bashi@ do you have a better idea on how I should bring this method into WTF? Maybe just put an alias in platform/heap? I guess you are using it in platform/heap?
On 2017/06/09 05:38:06, bashi wrote: > On 2017/06/09 05:20:40, keishi wrote: > > I plan to use this as a signal to trigger a page navigation GC. Design doc > here: > > > https://docs.google.com/document/d/1vJKPuMbJ5CQiFwrkxvTrB-x25SKF22-ui42oExvwp... > > > > bashi@ do you have a better idea on how I should bring this method into WTF? > > Maybe just put an alias in platform/heap? I guess you are using it in > platform/heap? I will be calling it from V8GCForContextDispose::NotifyContextDisposed which is in bindings/core/v8
On 2017/06/09 05:40:48, keishi wrote: > On 2017/06/09 05:38:06, bashi wrote: > > On 2017/06/09 05:20:40, keishi wrote: > > > I plan to use this as a signal to trigger a page navigation GC. Design doc > > here: > > > > > > https://docs.google.com/document/d/1vJKPuMbJ5CQiFwrkxvTrB-x25SKF22-ui42oExvwp... > > > > > > bashi@ do you have a better idea on how I should bring this method into WTF? > > > > Maybe just put an alias in platform/heap? I guess you are using it in > > platform/heap? > > I will be calling it from V8GCForContextDispose::NotifyContextDisposed which is > in bindings/core/v8 I see. Putting it in platform/MemoryCoordinator.h would be an option. (We may change interfaces of MemoryCoordinator{,Client}. Discussion is ongoing...)
keishi@chromium.org changed reviewers: + haraken@chromium.org
+haraken So I'm wondering how I should be exposing this function to Blink. MemoryCoordinator wasn't related, and it seems like more of a platform feature than WTF, so for this new CL I stuck it into platform/.
On 2017/06/09 07:33:08, keishi wrote: > +haraken > > So I'm wondering how I should be exposing this function to Blink. > MemoryCoordinator wasn't related, and it seems like more of a platform feature > than WTF, so for this new CL I stuck it into platform/. I'd prefer adding it to platform/MemoryCoordinator.h. Although the coordination mechanism is not yet enabled, platform/MemoryCoordinator.h is already working (static methods of platform/MemoryCoordinator.h are already used).
On 2017/06/12 00:51:54, haraken wrote: > On 2017/06/09 07:33:08, keishi wrote: > > +haraken > > > > So I'm wondering how I should be exposing this function to Blink. > > MemoryCoordinator wasn't related, and it seems like more of a platform feature > > than WTF, so for this new CL I stuck it into platform/. > > I'd prefer adding it to platform/MemoryCoordinator.h. Although the coordination > mechanism is not yet enabled, platform/MemoryCoordinator.h is already working > (static methods of platform/MemoryCoordinator.h are already used). I've moved it back into MemoryCoordinator. bashi-san are you OK with this?
On 2017/06/12 05:46:53, keishi wrote: > On 2017/06/12 00:51:54, haraken wrote: > > On 2017/06/09 07:33:08, keishi wrote: > > > +haraken > > > > > > So I'm wondering how I should be exposing this function to Blink. > > > MemoryCoordinator wasn't related, and it seems like more of a platform > feature > > > than WTF, so for this new CL I stuck it into platform/. > > > > I'd prefer adding it to platform/MemoryCoordinator.h. Although the > coordination > > mechanism is not yet enabled, platform/MemoryCoordinator.h is already working > > (static methods of platform/MemoryCoordinator.h are already used). > > I've moved it back into MemoryCoordinator. > > bashi-san are you OK with this? I'm fine with that :)
https://codereview.chromium.org/2933503002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/MemoryCoordinator.h (right): https://codereview.chromium.org/2933503002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/MemoryCoordinator.h:50: static bool HasLowAvailableMemory(); Where is the implementation of this method?
https://codereview.chromium.org/2933503002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/MemoryCoordinator.h (right): https://codereview.chromium.org/2933503002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/MemoryCoordinator.h:50: static bool HasLowAvailableMemory(); On 2017/06/12 06:14:20, haraken wrote: > > Where is the implementation of this method? Sorry. Added.
LGTM
https://codereview.chromium.org/2933503002/diff/60001/base/android/java/src/o... File base/android/java/src/org/chromium/base/SysUtils.java (right): https://codereview.chromium.org/2933503002/diff/60001/base/android/java/src/o... base/android/java/src/org/chromium/base/SysUtils.java:111: public static boolean hasLowAvailableMemory() { isCurrentlyLowMemory() ?
https://codereview.chromium.org/2933503002/diff/60001/base/android/java/src/o... File base/android/java/src/org/chromium/base/SysUtils.java (right): https://codereview.chromium.org/2933503002/diff/60001/base/android/java/src/o... base/android/java/src/org/chromium/base/SysUtils.java:111: public static boolean hasLowAvailableMemory() { On 2017/06/12 08:28:17, bashi wrote: > isCurrentlyLowMemory() ? Changed everything to isCurrentlyLowMemory
Description was changed from ========== Add HasLowAvailableMemory HasLowAvailableMemory returns Android's MemoryInfo.lowMemory This will be used as a GC triggering signal for low end devices. BUG= ========== to ========== Add IsCurrentlyLowMemory IsCurrentlyLowMemory returns Android's MemoryInfo.lowMemory This will be used as a GC triggering signal for low end devices. BUG= ==========
keishi@chromium.org changed reviewers: + yfriedman@chromium.org
+yfriedman Could you PTAL? I plan to use this for an Android Go memory reduction change. Design doc: https://docs.google.com/document/d/1vJKPuMbJ5CQiFwrkxvTrB-x25SKF22-ui42oExvwp...
https://codereview.chromium.org/2933503002/diff/80001/base/android/java/src/o... File base/android/java/src/org/chromium/base/SysUtils.java (right): https://codereview.chromium.org/2933503002/diff/80001/base/android/java/src/o... base/android/java/src/org/chromium/base/SysUtils.java:111: public static boolean isCurrentlyLowMemory() { make private if only used by native https://codereview.chromium.org/2933503002/diff/80001/base/android/java/src/o... base/android/java/src/org/chromium/base/SysUtils.java:116: am.getMemoryInfo(info); Did you consult the javadoc for this? I'm concerned that is it suggests we shouldn't be polling it. Also, we already have MemoryPressureListener which listens to the signals that Android suggests and sends notifications if low memory. You could register as an observer for that and re-use it
https://codereview.chromium.org/2933503002/diff/80001/base/android/java/src/o... File base/android/java/src/org/chromium/base/SysUtils.java (right): https://codereview.chromium.org/2933503002/diff/80001/base/android/java/src/o... base/android/java/src/org/chromium/base/SysUtils.java:116: am.getMemoryInfo(info); On 2017/06/12 18:56:47, Yaron wrote: > Did you consult the javadoc for this? I'm concerned that is it suggests we > shouldn't be polling it. Also, we already have MemoryPressureListener which > listens to the signals that Android suggests and sends notifications if low > memory. You could register as an observer for that and re-use it You're right javadoc says polling isn't recommended. It recommends using onTrimMemory or ActivityManager.getMyMemoryState instead. However, onTrimMemory is not called when memory pressure level goes down. The change I want to make needs the current memory pressure level when a page navigation happens. The current MemoryPressureListener system uses onTrimMemory so it has the same problems. ActivityManager.getMyMemoryState cannot be called from the renderer process because of security. Also we would like to start triggering this memory reduction before memory pressure is high. The MemoryPressureListener will be replaced by MemoryCoordinator's MemoryState in the future. MemoryCoordinator design doc: https://docs.google.com/document/d/1rGeUzhfzIKwmBzUcqUEwxEpLQQRq3PulRuHwFBlVI... When that is implemented I plan to use that instead but it won't be ready for M61, in time for Android Go. Also I plan to call this only once per main frame page load so it won't be called so frequently.
lgtm on my side. Could you file a bug entry for this?
Description was changed from ========== Add IsCurrentlyLowMemory IsCurrentlyLowMemory returns Android's MemoryInfo.lowMemory This will be used as a GC triggering signal for low end devices. BUG= ========== to ========== Add IsCurrentlyLowMemory IsCurrentlyLowMemory returns Android's MemoryInfo.lowMemory This will be used as a GC triggering signal for low end devices. BUG=732664 ==========
On 2017/06/13 04:15:41, bashi wrote: > lgtm on my side. > > Could you file a bug entry for this? Thanks, done.
yaron Ping
Sorry for the delay. Took some time to dig into the details and design docs. I'm a little concerned about methodology and whether this is representative. Let's address those first before moving forward with this. https://codereview.chromium.org/2933503002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/MemoryCoordinator.h (right): https://codereview.chromium.org/2933503002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/MemoryCoordinator.h:48: // Returns true when available memory is low. Please add a comment that this is not cheap and should not be called repeatedly.
https://codereview.chromium.org/2933503002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/MemoryCoordinator.h (right): https://codereview.chromium.org/2933503002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/MemoryCoordinator.h:48: // Returns true when available memory is low. On 2017/06/14 15:45:09, Yaron wrote: > Please add a comment that this is not cheap and should not be called repeatedly. Done.
lgtm
The CQ bit was checked by keishi@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.
The CQ bit was checked by keishi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, bashi@chromium.org Link to the patchset: https://codereview.chromium.org/2933503002/#ps120001 (title: "added comment")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1497933975176660,
"parent_rev": "7eecfd7d05677a84c2d35b7562483822362654d5", "commit_rev":
"ee6d2bccc4bdd982803f7c475f61a54675f4d555"}
Message was sent while issue was closed.
Description was changed from ========== Add IsCurrentlyLowMemory IsCurrentlyLowMemory returns Android's MemoryInfo.lowMemory This will be used as a GC triggering signal for low end devices. BUG=732664 ========== to ========== Add IsCurrentlyLowMemory IsCurrentlyLowMemory returns Android's MemoryInfo.lowMemory This will be used as a GC triggering signal for low end devices. BUG=732664 Review-Url: https://codereview.chromium.org/2933503002 Cr-Commit-Position: refs/heads/master@{#480737} Committed: https://chromium.googlesource.com/chromium/src/+/ee6d2bccc4bdd982803f7c475f61... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/ee6d2bccc4bdd982803f7c475f61... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
