Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2678323002:
Add documentation for MemoryCoordinator-related primitives
DescriptionAdd documentation for MemoryCoordinator-related primitives
It's not clear when we should use MemoryCoordinator, MemoryPressureListener
and isLowEndDevice. This CL adds documentation about it.
BUG=
Patch Set 1 #
Total comments: 6
Patch Set 2 : temp #
Total comments: 5
Messages
Total messages: 9 (2 generated)
haraken@chromium.org changed reviewers: + bashi@chromium.org, rsleevi@chromium.org
Ryan: PTAL
Thanks for adding this! I'm not sure it's worth doing but does it make sense to have one .md file in base/memory which provides an overview of the current status of MemoryPressureListener/MemoryCoordinator/IsLowEndDevice() and future plans?
On 2017/02/07 08:08:54, bashi wrote: > Thanks for adding this! > > I'm not sure it's worth doing but does it make sense to have one .md file in > base/memory which provides an overview of the current status of > MemoryPressureListener/MemoryCoordinator/IsLowEndDevice() > and future plans? I'll do whatever Ryan prefers :)
Thank you very much for taking this on. I think I may have miscommunicated the goals/concerns that I was trying to raise on the thread and in person, so I've tried to expand them here. I realize I end up repeating some things, but that is not intended as disrespect - just showing the different ways in which the concerns can manifest. Another way of phrasing my goal was in understanding, at a high level, what are the patterns and code to "be a good memory citizen". This provides the overview of the ways in which you can (MemoryPressureListener, isLowEndDevice, MemoryCoordinatorClient), and then tries to show how to select the right tool for the right job. This seems like it would best fit within either //docs or //base/memory/docs, and then you could refer to that documentation in all three of these classes to show how they fit together, and what the best practices are. Please consider my remarks about expanding the class comments moreso as something for those docs (rather than where they currently are), but I was trying to expand on the pieces of information that are 'missing' or 'unclear' https://codereview.chromium.org/2678323002/diff/1/base/memory/memory_pressure... File base/memory/memory_pressure_listener.h (right): https://codereview.chromium.org/2678323002/diff/1/base/memory/memory_pressure... base/memory/memory_pressure_listener.h:48: // TODO(bashi): Deprecate MemoryPressureListener. Note that the "we" here is ambiguous (is it team? Chromium? Google?) The thread at https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/NH-S6KCkr2M has more discussion about the challenges with that Perhaps this could be rephrased more assertively, and give clearer guidance as to what the recommended 'alternative' pattern is // NOTE: The MemoryPressureListener is in the process of being replaced with the MemoryCoordinator. // For new code, prefer to implement the MemoryCoordinatorClient interface specified in // //base/memory/memory_coordinator_client.h // // The state MEMORY_PRESSURE_LEVEL_NONE corresponds to MemoryState::NORMAL and the state // MEMORY_PRESSURE_LEVEL_MODERATE corresponds to MemoryState::THROTTLED. The state // MEMORY_PRESSURE_LEVEL_CRITICAL is indicated via MemoryCoordinatorClient::OnPurgeMemory() 1) I'm not sure if that's a correct mapping; if it isn't, it might be an opportunity to improve the documentation 2) When I search the codebase for "MemoryCoordinator", I found multiple results - but the MemoryCoordinatorClient was not the first of them It might be better to put some of this broader roadmap in //docs (or in a //base/memory/docs folder) and reference that here, since you can then more freely expand upon the relationship between the classes, their equivalences, and their known gaps. https://codereview.chromium.org/2678323002/diff/1/base/sys_info.h File base/sys_info.h (right): https://codereview.chromium.org/2678323002/diff/1/base/sys_info.h#newcode149 base/sys_info.h:149: // See a comment in MemoryCoordinator for how to use it. So, I think this is a 'bad' comment, because it couples //base/sys_info to //base/memory implementation details. Also, see my other remark about trying to figure out what "in MemoryCoordinator" means This is part of why I suggest a //base/memory/docs sort of guidance. https://codereview.chromium.org/2678323002/diff/1/content/public/browser/memo... File content/public/browser/memory_coordinator.h (right): https://codereview.chromium.org/2678323002/diff/1/content/public/browser/memo... content/public/browser/memory_coordinator.h:19: // Currently we have three ways to control memory components: Similar to my earlier remarks, the "we" here is ambiguous - is it "the owners of //content/public/browser"? The owners of Chromium? When looking through this CL, I was surprised to see that (effectively) it's a coupling between //base/memory and //content/public/browser - which seems like a layering violation that we're defining an interface in //base whose behaviour and relationship can only be explained in terms of //content/public/browser I know there are plans to move things around to better accomodate that, and I would suggest that while it may not be a critical blocker relative to your other tasks, for your colleagues such as myself, having things either logically make sense or having a clearly documented roadmap (e.g. in source via Markdown) is something to be prioritized over any feature work. That is - if folks are to use //base/memory/memory_coordinator_client.h, then we should prioritize moving the memory coordinator infrastructure to base. This is especially important and critical for a project like //net, which does not depend on //content/public, and for applications such as Cronet, which similarly do not depend on this. Being able to understand the interfaces and their relationship is an important part of code health. I would suggest that because the other two classes in //base are effectively saying "see //content/public" - it is a layering violation, the same as if base #include'd "//content/public". The way to resolve this is like I mentioned - using an explicit document that provides guidance. I think especially useful for the transition is if you could consider something like how https://google.github.io/styleguide/cppguide.html is structured with respect to examples; that is, when there are N ways of doing something, it provides examples of the 'syntactically valid but not acceptable/desirable' form, and then provides an example of showing the 'right' form. We use that same approach in https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md when discussion conditionals.
Description was changed from ========== Add documentation for MemoryCoordinator-related primitives It's not clear when we should use MemoryCoordinator, MemoryPressureListener and isLowEndDevice. This CL adds documentation about it. BUG= ========== to ========== Add documentation for MemoryCoordinator-related primitives It's not clear when we should use MemoryCoordinator, MemoryPressureListener and isLowEndDevice. This CL adds documentation about it. BUG= ==========
PTAL https://codereview.chromium.org/2678323002/diff/1/base/memory/memory_pressure... File base/memory/memory_pressure_listener.h (right): https://codereview.chromium.org/2678323002/diff/1/base/memory/memory_pressure... base/memory/memory_pressure_listener.h:48: // TODO(bashi): Deprecate MemoryPressureListener. On 2017/02/08 15:02:04, Ryan Sleevi wrote: > Note that the "we" here is ambiguous (is it team? Chromium? Google?) The thread > at > https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/NH-S6KCkr2M > has more discussion about the challenges with that > > Perhaps this could be rephrased more assertively, and give clearer guidance as > to what the recommended 'alternative' pattern is > > // NOTE: The MemoryPressureListener is in the process of being replaced with the > MemoryCoordinator. > // For new code, prefer to implement the MemoryCoordinatorClient interface > specified in > // //base/memory/memory_coordinator_client.h > // > // The state MEMORY_PRESSURE_LEVEL_NONE corresponds to MemoryState::NORMAL and > the state > // MEMORY_PRESSURE_LEVEL_MODERATE corresponds to MemoryState::THROTTLED. The > state > // MEMORY_PRESSURE_LEVEL_CRITICAL is indicated via > MemoryCoordinatorClient::OnPurgeMemory() > > 1) I'm not sure if that's a correct mapping; if it isn't, it might be an > opportunity to improve the documentation > 2) When I search the codebase for "MemoryCoordinator", I found multiple results > - but the MemoryCoordinatorClient was not the first of them > > It might be better to put some of this broader roadmap in //docs (or in a > //base/memory/docs folder) and reference that here, since you can then more > freely expand upon the relationship between the classes, their equivalences, and > their known gaps. - Removed 'we'. - Regarding the mapping, we'll map LEVEL_NONE to MemoryState::NORMAL and LEVEL_MODERATE and LEVEL_CRITICAL to MemoryState::THROTTLE. However, this is still under experiments. https://codereview.chromium.org/2678323002/diff/1/base/sys_info.h File base/sys_info.h (right): https://codereview.chromium.org/2678323002/diff/1/base/sys_info.h#newcode149 base/sys_info.h:149: // See a comment in MemoryCoordinator for how to use it. On 2017/02/08 15:02:04, Ryan Sleevi wrote: > So, I think this is a 'bad' comment, because it couples //base/sys_info to > //base/memory implementation details. Also, see my other remark about trying to > figure out what "in MemoryCoordinator" means > > This is part of why I suggest a //base/memory/docs sort of guidance. Done. https://codereview.chromium.org/2678323002/diff/1/content/public/browser/memo... File content/public/browser/memory_coordinator.h (right): https://codereview.chromium.org/2678323002/diff/1/content/public/browser/memo... content/public/browser/memory_coordinator.h:19: // Currently we have three ways to control memory components: On 2017/02/08 15:02:04, Ryan Sleevi wrote: > Similar to my earlier remarks, the "we" here is ambiguous - is it "the owners of > //content/public/browser"? The owners of Chromium? > > When looking through this CL, I was surprised to see that (effectively) it's a > coupling between //base/memory and //content/public/browser - which seems like a > layering violation that we're defining an interface in //base whose behaviour > and relationship can only be explained in terms of //content/public/browser > > I know there are plans to move things around to better accomodate that, and I > would suggest that while it may not be a critical blocker relative to your other > tasks, for your colleagues such as myself, having things either logically make > sense or having a clearly documented roadmap (e.g. in source via Markdown) is > something to be prioritized over any feature work. That is - if folks are to use > //base/memory/memory_coordinator_client.h, then we should prioritize moving the > memory coordinator infrastructure to base. > > This is especially important and critical for a project like //net, which does > not depend on //content/public, and for applications such as Cronet, which > similarly do not depend on this. Being able to understand the interfaces and > their relationship is an important part of code health. > > I would suggest that because the other two classes in //base are effectively > saying "see //content/public" - it is a layering violation, the same as if base > #include'd "//content/public". The way to resolve this is like I mentioned - > using an explicit document that provides guidance. > > I think especially useful for the transition is if you could consider something > like how https://google.github.io/styleguide/cppguide.html is structured with > respect to examples; that is, when there are N ways of doing something, it > provides examples of the 'syntactically valid but not acceptable/desirable' > form, and then provides an example of showing the 'right' form. We use that same > approach in > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md > when discussion conditionals. Yeah, the fact that we don't have MemoryCoordinator in //base is making things complicated. And I think it should be fixed. As you mentioned, this causes a practical problem when we want to use MemoryCoordinator in //net.
Sorry, I somehow missed your feedback in my email queue. Please feel free to ping anytime you're not getting timely feedback. I've taken another stab, which offers a lot more suggestions and tries to explain some of the concrete objections. Please feel free to reword any of these suggestions as you see appropriate, but do please pay attention to the motivation behind each set of suggested changes. Thank you for your continued patience in working on this. https://codereview.chromium.org/2678323002/diff/20001/base/memory/memory_coor... File base/memory/memory_coordinator.md (right): https://codereview.chromium.org/2678323002/diff/20001/base/memory/memory_coor... base/memory/memory_coordinator.md:4: have the following two notions: Suggestion: "memory-intensive" leaves it subjective to the reader as to what is or is not in scope of this. To ensure Chromium projects make the best use of available system memory, developers should strive to ensure their code and components address the following two scenarios: 1. How to deal with past allocations when the system is under memory pressure. Components are expected to reduce as much previously allocated memory as possible by listening for and processing notifications that signal such pressure. For example, a component that has an internal cache may choose to clear the cache as a way to reduce memory. This is a _reactive_ way to reduce the memory footprint. 2. How to deal with future allocations when the system is under memory pressure. Components are expected to reduce or throttle future memory allocations for as long as the memory pressure exists, which for memory constrained devices and applications, may be indefinitely. For example, a component that has an internal cache may limit the cache size (or disable caching, if appropriate) for the duration of the memory pressure event. This is the _proactive_ way to reduce the memory footprint. The changes I made are trying to avoid subjective/interpretative statements, which will hopefully provide clearer guidance. This is eliminating "memory-intensive" or "high". I also tried to offer symmetry for the examples, and Markdown-cleaned them up a little to use hanging indents :) https://codereview.chromium.org/2678323002/diff/20001/base/memory/memory_coor... base/memory/memory_coordinator.md:14: # Current status # Current Status ## Reactive Reduction To properly address reactive reduction in memory, classes should make use of the following classes: * [base::MemoryPressureListener](memory_pressure_listener.h) allows a component to subscribe a Callback to be notified as the available system memory changes. Components are expected to respond to the `MEMORY_PRESSURE_LEVEL_MODERATE` and `MEMORY_PRESSURE_LEVEL_CRITICAL` notifications and take steps to free memory, as described in the class documentation. * [base::MemoryCoordinatorClient](memory_coordinator_client.h) allows components to implement an interface that receives notifications when the available system memory changes. Components are expected to implement this interface by ensuring that the `OnPurgeMemory()` method takes steps to free memory, as described in the class documentation. Note: `base::MemoryPressureListener` is in the process of being deprecated, while the `base::MemoryCoordinatorClient` is still being developed behind a flag. Until `base::MemoryCoordinatorClient` becomes the default, components should implement both methods of receiving reactive notifications. ## Proactive Reduction To properly address proactive reduction in memory, classes should make use of the following classes: * [base::SysInfo::IsLowEndDevice()](../sys_info.h) indicates whether the device is operating in a constrained memory environment. For non-Android devices, this is 512MB of RAM or less. For Android devices, this is a compile-time flag dependent on the device being targeted. Components should make use of this information to significantly reduce or eliminate allocations that are not core to the functionality of the component, and should strive to free available memory as quickly as reasonable. For example, a cache size might be 128 items under normal conditions, but may be only 16 items (or disabled) for devices that are memory constrained. * [base::MemoryCoordinatorClient](memory_coordinator_client.h) provides the `OnMemoryStateChange()` notification that provides dynamic notification about memory pressure. Components that make use of this notification should make use of the `THROTTLED` and `SUSPENDED` notifications to adjust future memory allocations and growth. Note: `MemoryCoordinatorClient` and `IsLowEndDevice` are complementary, and as such, components should implement both strategies of managing memory allocations. My highlights of the change here: - Follow the format of the introduction - Make use of Markdown to the most we can - isLowEndDevice does *NOT* appear to be a compile time flag, on Android and !Android, judging by both sys_info.cc and sys_info_android.cc - Remove the discussion about 'MemoryCoordinator', since as it relates to developing, it appears to be MemoryCoordinator*Client* https://codereview.chromium.org/2678323002/diff/20001/base/memory/memory_coor... File base/memory/memory_coordinator_client.h (right): https://codereview.chromium.org/2678323002/diff/20001/base/memory/memory_coor... base/memory/memory_coordinator_client.h:28: // See //base/memory/memory_coordinator.md for the replacement plan. I don't think memory_coordinator.md is really "the replacement plan", but rather the "How to use them" That is, it gives no information about how the replacement will look other than there is a plan :) https://codereview.chromium.org/2678323002/diff/20001/base/memory/memory_pres... File base/memory/memory_pressure_listener.h (right): https://codereview.chromium.org/2678323002/diff/20001/base/memory/memory_pres... base/memory/memory_pressure_listener.h:48: // for more details. Suggestion: Update line 5 with something like: // This file contains a set of APIs for receiving notifications of // memory pressure events, where supported by the underlying platform. // Developers should strive to reduce non-essential memory when // receiving these notifications, when possible. // // For further details, see //base/memory/memory_coordinator.md for // further details Then update this with: // NOTE: MemoryPressureListener is in the process of being deprecated. See //base/memory/memory_coordinator.md for current best practices to respond to memory pressure events. https://codereview.chromium.org/2678323002/diff/20001/content/public/browser/... File content/public/browser/memory_coordinator.h (right): https://codereview.chromium.org/2678323002/diff/20001/content/public/browser/... content/public/browser/memory_coordinator.h:17: // See comments on MemoryCoordinatorClient for how to use it. I think this comment (line 17) should be deleted entirely. The MemoryCoordinatorClient doesn't describe how to use the MemoryCoordinator, it describes how to use the MCC. For example, a logical question is whether or not I need to explicitly initialize the MemoryCoordinator (e.g. call MemoryCoorindator::GetInstance()) to start notifications being processed, whether that's implicit by the MemoryCoordinatorClient, or something else. Does that make sense? I'm not sure what's trying to be said here, but I don't think this comment (past or present) does what it says.
Poke? |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
