Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2363353002:
Document memory coordinator (Closed)
DescriptionDocument memory coordinator
The current headers lack explanations of thread guarantees, lifetime
management, and how to implement MemoryCoordinatorClient. Add these
explanations.
Also, this CL stops exposing ClientList from MemoryCoordinatorClientRegistry.
BUG=617492
Committed: https://crrev.com/0cf58aaf5fd61c0047cbe51d3a484f01607b43ec
Cr-Commit-Position: refs/heads/master@{#421099}
Patch Set 1 #
Total comments: 14
Patch Set 2 : comments #
Total comments: 2
Patch Set 3 : could -> can #
Total comments: 2
Patch Set 4 : comments #
Total comments: 9
Patch Set 5 : comments #
Messages
Total messages: 31 (12 generated)
bashi@chromium.org changed reviewers: + dcheng@chromium.org, rsleevi@chromium.org
dcheng@: could you review as an owner of base/ ?
https://codereview.chromium.org/2363353002/diff/1/base/memory/memory_coordina... File base/memory/memory_coordinator_client.h (right): https://codereview.chromium.org/2363353002/diff/1/base/memory/memory_coordina... base/memory/memory_coordinator_client.h:15: // respond memory state changes. Unlike MemoryPressureListener, this is a Nit: to respond => to respond to https://codereview.chromium.org/2363353002/diff/1/base/memory/memory_coordina... base/memory/memory_coordinator_client.h:17: // are changed. In order to avoid thrashing, state transitions won't occur in a Nit: State transitions are throttled to avoid thrashing; the exact throttling period is platform dependent, but will be at least N seconds (where N is an actual number... maybe there's a concrete lower bound we can give here like 10 or 15 second?) https://codereview.chromium.org/2363353002/diff/1/base/memory/memory_coordina... base/memory/memory_coordinator_client.h:42: // Called when memory state has changed. Any transition could occur except for Is UNKNOWN an initial state? https://codereview.chromium.org/2363353002/diff/1/base/memory/memory_coordina... File base/memory/memory_coordinator_client_registry.h (right): https://codereview.chromium.org/2363353002/diff/1/base/memory/memory_coordina... base/memory/memory_coordinator_client_registry.h:28: // unregistered before they are destructed. Nit: destructed => destroyed https://codereview.chromium.org/2363353002/diff/1/base/memory/memory_coordina... base/memory/memory_coordinator_client_registry.h:39: // Notifies memory state change to clients. Nit: Notify clients of a memory state change. https://codereview.chromium.org/2363353002/diff/1/content/browser/memory/memo... File content/browser/memory/memory_coordinator.h (right): https://codereview.chromium.org/2363353002/diff/1/content/browser/memory/memo... content/browser/memory/memory_coordinator.h:18: // Memory coordinator is a content-aware and stateful replacement for I'm not entirely sure what content-aware means. https://codereview.chromium.org/2363353002/diff/1/content/browser/memory/memo... content/browser/memory/memory_coordinator.h:20: // system consists of several parts and some of them are located in base/ to I think it's OK to leave out the second sentence. Is this mechanism expected to completely replace MemoryPressureMonitor/Listener?
Description was changed from ========== Document memory coordinator The current headers lacks explanations of thread guarantees, lifetime management, and how to implement MemoryCoordinatorClient. Add these explanations. Also, this CL stops exposing ClientList from MemoryCoordinatorClientRegistry. BUG=617492 ========== to ========== Document memory coordinator The current headers lack explanations of thread guarantees, lifetime management, and how to implement MemoryCoordinatorClient. Add these explanations. Also, this CL stops exposing ClientList from MemoryCoordinatorClientRegistry. BUG=617492 ==========
Thank you for review! https://codereview.chromium.org/2363353002/diff/1/base/memory/memory_coordina... File base/memory/memory_coordinator_client.h (right): https://codereview.chromium.org/2363353002/diff/1/base/memory/memory_coordina... base/memory/memory_coordinator_client.h:15: // respond memory state changes. Unlike MemoryPressureListener, this is a On 2016/09/26 04:55:07, dcheng wrote: > Nit: to respond => to respond to Done. https://codereview.chromium.org/2363353002/diff/1/base/memory/memory_coordina... base/memory/memory_coordinator_client.h:17: // are changed. In order to avoid thrashing, state transitions won't occur in a On 2016/09/26 04:55:07, dcheng wrote: > Nit: State transitions are throttled to avoid thrashing; the exact throttling > period is platform dependent, but will be at least N seconds (where N is an > actual number... maybe there's a concrete lower bound we can give here like 10 > or 15 second?) Done. I choose "5-10 seconds" as the design doc which chrisha@ wrote. https://docs.google.com/document/d/1GMSkij4qpbkK6T7MtCX9FcPlv39Q9J2lmW8qY8Wmf... https://codereview.chromium.org/2363353002/diff/1/base/memory/memory_coordina... base/memory/memory_coordinator_client.h:42: // Called when memory state has changed. Any transition could occur except for On 2016/09/26 04:55:07, dcheng wrote: > Is UNKNOWN an initial state? At this point, NORMAL is an initial state. But there is a todo comment in memory_coordinator.cc to have a setup state. Mentioned initial state. https://codereview.chromium.org/2363353002/diff/1/base/memory/memory_coordina... File base/memory/memory_coordinator_client_registry.h (right): https://codereview.chromium.org/2363353002/diff/1/base/memory/memory_coordina... base/memory/memory_coordinator_client_registry.h:28: // unregistered before they are destructed. On 2016/09/26 04:55:07, dcheng wrote: > Nit: destructed => destroyed Done. https://codereview.chromium.org/2363353002/diff/1/base/memory/memory_coordina... base/memory/memory_coordinator_client_registry.h:39: // Notifies memory state change to clients. On 2016/09/26 04:55:07, dcheng wrote: > Nit: Notify clients of a memory state change. Done. https://codereview.chromium.org/2363353002/diff/1/content/browser/memory/memo... File content/browser/memory/memory_coordinator.h (right): https://codereview.chromium.org/2363353002/diff/1/content/browser/memory/memo... content/browser/memory/memory_coordinator.h:18: // Memory coordinator is a content-aware and stateful replacement for On 2016/09/26 04:55:07, dcheng wrote: > I'm not entirely sure what content-aware means. I borrowed this sentence from the design doc. It seems that this overview is not needed. Removed. https://codereview.chromium.org/2363353002/diff/1/content/browser/memory/memo... content/browser/memory/memory_coordinator.h:20: // system consists of several parts and some of them are located in base/ to On 2016/09/26 04:55:07, dcheng wrote: > I think it's OK to leave out the second sentence. Is this mechanism expected to > completely replace MemoryPressureMonitor/Listener? Yes, we are planning to remove MemoryPressureMonitor/Listener.
lgtm https://codereview.chromium.org/2363353002/diff/20001/base/memory/memory_coor... File base/memory/memory_coordinator_client.h (right): https://codereview.chromium.org/2363353002/diff/20001/base/memory/memory_coor... base/memory/memory_coordinator_client.h:35: // changes. An initial state is NORMAL. See MemoryCoordinatorClientRegistry for Nit: An -> The https://codereview.chromium.org/2363353002/diff/20001/base/memory/memory_coor... base/memory/memory_coordinator_client.h:41: // Called when memory state has changed. Any transition could occur except for Nit: could => can
bashi@chromium.org changed reviewers: + chrisha@chromium.org, haraken@chromium.org
(Added chrisha@ and haraken@) Thanks. I'll wait for Ryan's response.
Thanks, LGTM on my side
hajimehoshi@chromium.org changed reviewers: + hajimehoshi@chromium.org
non-owner lgtm https://codereview.chromium.org/2363353002/diff/40001/base/memory/memory_coor... File base/memory/memory_coordinator_client.h (right): https://codereview.chromium.org/2363353002/diff/40001/base/memory/memory_coor... base/memory/memory_coordinator_client.h:16: // stateful mechanism and clients receive notifications only when memory states s/receive/that receive/
https://codereview.chromium.org/2363353002/diff/40001/base/memory/memory_coor... File base/memory/memory_coordinator_client.h (right): https://codereview.chromium.org/2363353002/diff/40001/base/memory/memory_coor... base/memory/memory_coordinator_client.h:16: // stateful mechanism and clients receive notifications only when memory states On 2016/09/26 07:08:13, hajimehoshi wrote: > s/receive/that receive/ Done.
https://codereview.chromium.org/2363353002/diff/60001/base/memory/memory_coor... File base/memory/memory_coordinator_client.h (right): https://codereview.chromium.org/2363353002/diff/60001/base/memory/memory_coor... base/memory/memory_coordinator_client.h:16: // stateful mechanism and clients that receive notifications only when memory clients receive (remove "that") https://codereview.chromium.org/2363353002/diff/60001/base/memory/memory_coor... base/memory/memory_coordinator_client.h:19: // seconds. Mention that clients are expected to make changes in memory usage that persist for the duration of the memory state. https://codereview.chromium.org/2363353002/diff/60001/base/memory/memory_coor... base/memory/memory_coordinator_client.h:29: // Running and interactive but allocation should be throttled. The client should free up any memory that is used as an optimization but that is not necessary for the process to run (eg, caches). https://codereview.chromium.org/2363353002/diff/60001/base/memory/memory_coor... base/memory/memory_coordinator_client.h:31: // Still resident in memory but core processing logic has been suspended. The client should free up any memory that is used as an optimization, or any memory whose contents can be reproduced when transitioning out of the suspended state (eg, parsed resources that can be reloaded from disk).
LGTM w/ Chris' additional suggestions https://codereview.chromium.org/2363353002/diff/60001/base/memory/memory_coor... File base/memory/memory_coordinator_client.h (right): https://codereview.chromium.org/2363353002/diff/60001/base/memory/memory_coor... base/memory/memory_coordinator_client.h:19: // seconds. On 2016/09/26 14:10:45, chrisha (slow) wrote: > Mention that clients are expected to make changes in memory usage that persist > for the duration of the memory state. Interesting. So to confirm - if a memory state is THROTTLED, for example, a component should disable all caching until it receives notification of NORMAL, correct?
On 2016/09/26 15:05:17, Ryan Sleevi (slow) wrote: > LGTM w/ Chris' additional suggestions > > https://codereview.chromium.org/2363353002/diff/60001/base/memory/memory_coor... > File base/memory/memory_coordinator_client.h (right): > > https://codereview.chromium.org/2363353002/diff/60001/base/memory/memory_coor... > base/memory/memory_coordinator_client.h:19: // seconds. > On 2016/09/26 14:10:45, chrisha (slow) wrote: > > Mention that clients are expected to make changes in memory usage that persist > > for the duration of the memory state. > > Interesting. So to confirm - if a memory state is THROTTLED, for example, a > component should disable all caching until it receives notification of NORMAL, > correct? Yup! That's the main difference between MP and MC. In the MP world notifications were stateless, and we'd get thrashing of memory use as clients emptied caches and then immediately filled them back up. MC introduces the notion of persistent states to deal with this.
Thank you for review Ryan and Chris! I'm going to submit this CL. Please let me know if you have more suggestions/comments. I'll address them later. https://codereview.chromium.org/2363353002/diff/60001/base/memory/memory_coor... File base/memory/memory_coordinator_client.h (right): https://codereview.chromium.org/2363353002/diff/60001/base/memory/memory_coor... base/memory/memory_coordinator_client.h:16: // stateful mechanism and clients that receive notifications only when memory On 2016/09/26 14:10:45, chrisha (slow) wrote: > clients receive > > (remove "that") Done. https://codereview.chromium.org/2363353002/diff/60001/base/memory/memory_coor... base/memory/memory_coordinator_client.h:19: // seconds. On 2016/09/26 14:10:45, chrisha (slow) wrote: > Mention that clients are expected to make changes in memory usage that persist > for the duration of the memory state. Done. https://codereview.chromium.org/2363353002/diff/60001/base/memory/memory_coor... base/memory/memory_coordinator_client.h:29: // Running and interactive but allocation should be throttled. On 2016/09/26 14:10:45, chrisha (slow) wrote: > The client should free up any memory that is used as an optimization but that is > not necessary for the process to run (eg, caches). Done. https://codereview.chromium.org/2363353002/diff/60001/base/memory/memory_coor... base/memory/memory_coordinator_client.h:31: // Still resident in memory but core processing logic has been suspended. On 2016/09/26 14:10:45, chrisha (slow) wrote: > The client should free up any memory that is used as an optimization, or any > memory whose contents can be reproduced when transitioning out of the suspended > state (eg, parsed resources that can be reloaded from disk). Done.
The CQ bit was checked by bashi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org, haraken@chromium.org, hajimehoshi@chromium.org, rsleevi@chromium.org Link to the patchset: https://codereview.chromium.org/2363353002/#ps80001 (title: "comments")
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
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by bashi@chromium.org
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
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by bashi@chromium.org
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 ========== Document memory coordinator The current headers lack explanations of thread guarantees, lifetime management, and how to implement MemoryCoordinatorClient. Add these explanations. Also, this CL stops exposing ClientList from MemoryCoordinatorClientRegistry. BUG=617492 ========== to ========== Document memory coordinator The current headers lack explanations of thread guarantees, lifetime management, and how to implement MemoryCoordinatorClient. Add these explanations. Also, this CL stops exposing ClientList from MemoryCoordinatorClientRegistry. BUG=617492 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Document memory coordinator The current headers lack explanations of thread guarantees, lifetime management, and how to implement MemoryCoordinatorClient. Add these explanations. Also, this CL stops exposing ClientList from MemoryCoordinatorClientRegistry. BUG=617492 ========== to ========== Document memory coordinator The current headers lack explanations of thread guarantees, lifetime management, and how to implement MemoryCoordinatorClient. Add these explanations. Also, this CL stops exposing ClientList from MemoryCoordinatorClientRegistry. BUG=617492 Committed: https://crrev.com/0cf58aaf5fd61c0047cbe51d3a484f01607b43ec Cr-Commit-Position: refs/heads/master@{#421099} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/0cf58aaf5fd61c0047cbe51d3a484f01607b43ec Cr-Commit-Position: refs/heads/master@{#421099} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
