Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2094583002:
Add MemoryCoordinator (Closed)
|
Created:
4 years, 6 months ago by bashi Modified:
4 years, 5 months ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, nasko+codewatch_chromium.org, creis+watch_chromium.org, qsr+mojo_chromium.org, droger+watchlist_chromium.org, viettrungluu+watch_chromium.org, blundell+watchlist_chromium.org, sdefresne+watchlist_chromium.org, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, dglazkov+blink, blink-reviews, darin-cc_chromium.org, mkwst+moarreviews-renderer_chromium.org, blink-reviews-api_chromium.org, darin (slow to review), ben+mojo_chromium.org, kinuko+watch Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd MemoryCoordinator
This CL adds MemoryCoordinator mojo interface and
its implementation. When --enable-features=MemoryCoordinator
is specified the browser and renderers create their
MemoryCoordinators. MemoryCoordinatorImpl (which lives in
the browser process) creates a mojo message pipe when a
renderer is launched.
This CL doesn't add actual handling of memory events, which
will be implemented in follow-up CLs.
BUG=617492
Committed: https://crrev.com/066dcbbad57a70ba5380a4a8cdd0572d89bdc279
Cr-Commit-Position: refs/heads/master@{#405402}
Patch Set 1 #Patch Set 2 : gyp #
Total comments: 15
Patch Set 3 : comment #Patch Set 4 : Use feature_list #
Total comments: 10
Patch Set 5 : comments #Patch Set 6 : Changed a way to add child to memory coordinator #Patch Set 7 : gn #
Total comments: 6
Patch Set 8 : comment #
Total comments: 10
Patch Set 9 : Create handle per child #
Total comments: 12
Patch Set 10 : comment #Patch Set 11 : Remove state checking #
Total comments: 2
Patch Set 12 : comment #Patch Set 13 : rebase #Messages
Total messages: 66 (14 generated)
Description was changed from ========== Activate memory coordinator when --enable-memory-coordinator is specified Description to be written later. BUG=617492 ========== to ========== Use memory coordinator when --enable-memory-coordinator is specified This CL adds --enable-memory-coordinator flag which: - makes the browser connects to a ChildMemoryCoordinator when a renderer is launched - disables memory pressure listener on renderers This CL also adds a blink API for setting memory allocation mode. A ChildMemoryCoordinator in a renderer calls the API based on memory events it receives. The API is empty now and it will be implemented in follow-up CLs. BUG=617492 ==========
bashi@chromium.org changed reviewers: + chrisha@chromium.org, haraken@chromium.org
PTAL If this looks ok I'll ask OWNERS reviews.
ping?
https://codereview.chromium.org/2094583002/diff/20001/components/memory_coord... File components/memory_coordinator/browser/memory_state_notifier.cc (right): https://codereview.chromium.org/2094583002/diff/20001/components/memory_coord... components/memory_coordinator/browser/memory_state_notifier.cc:29: int id = GetNextId(); Would you help me understand why you need an id system? (i.e., why it's not enough to just keep the list of child coordinators) https://codereview.chromium.org/2094583002/diff/20001/components/memory_coord... File components/memory_coordinator/child/blink_memory_coordinator_client.cc (right): https://codereview.chromium.org/2094583002/diff/20001/components/memory_coord... components/memory_coordinator/child/blink_memory_coordinator_client.cc:24: blink::WebMemoryCoordinator::setAllocationMode( As commented in the other place, I want to make the Blink API consistent with MemoryCoordinatorClient's API. So I'd prefer just redirecting the OnMemoryStateChanged API to Blink instead of translating it to setAllocationMode or something. https://codereview.chromium.org/2094583002/diff/20001/components/memory_coord... File components/memory_coordinator/child/child_memory_coordinator_impl.cc (right): https://codereview.chromium.org/2094583002/diff/20001/components/memory_coord... components/memory_coordinator/child/child_memory_coordinator_impl.cc:15: new ChildMemoryCoordinatorImpl(std::move(request), clients); Nit: new ChildMemoryCoordinatorImpl(std::move(request), (new ClientList) https://codereview.chromium.org/2094583002/diff/20001/components/memory_coord... components/memory_coordinator/child/child_memory_coordinator_impl.cc:23: blink_client_(new BlinkMemoryCoordinatorClient) { Do we need the member variable |blink_client_|? https://codereview.chromium.org/2094583002/diff/20001/components/memory_coord... File components/memory_coordinator/public/interfaces/child_memory_coordinator.mojom (right): https://codereview.chromium.org/2094583002/diff/20001/components/memory_coord... components/memory_coordinator/public/interfaces/child_memory_coordinator.mojom:24: GetLastState() => (MemoryState state); GetLastState => GetCurrentState ? https://codereview.chromium.org/2094583002/diff/20001/third_party/WebKit/publ... File third_party/WebKit/public/web/WebMemoryCoordinator.h (right): https://codereview.chromium.org/2094583002/diff/20001/third_party/WebKit/publ... third_party/WebKit/public/web/WebMemoryCoordinator.h:24: BLINK_EXPORT static void setAllocationMode(WebMemoryAllocationMode); Would it make more sense to introduce onMemoryStateChanged() API instead of setAllocationMode? I want the Blink API to be more consistent with the Chromium-side memory-coordinator API.
https://codereview.chromium.org/2094583002/diff/20001/components/memory_coord... File components/memory_coordinator/browser/memory_state_notifier.cc (right): https://codereview.chromium.org/2094583002/diff/20001/components/memory_coord... components/memory_coordinator/browser/memory_state_notifier.cc:29: int id = GetNextId(); On 2016/06/27 02:17:07, haraken wrote: > > Would you help me understand why you need an id system? (i.e., why it's not > enough to just keep the list of child coordinators) ChildMemoryCoordinatorPtr is an alias for InterfacePtr and it doesn't support copy and compare, so - we can't use std::set - when we use std::vector we need to pass the pointer of the InterfacePtr to RemoveChild() and RemoveChild() needs to iterate the list to remove it from the list. The same can be said to other list-like containers I think using IDs is simple. https://codereview.chromium.org/2094583002/diff/20001/components/memory_coord... File components/memory_coordinator/child/blink_memory_coordinator_client.cc (right): https://codereview.chromium.org/2094583002/diff/20001/components/memory_coord... components/memory_coordinator/child/blink_memory_coordinator_client.cc:24: blink::WebMemoryCoordinator::setAllocationMode( On 2016/06/27 02:17:07, haraken wrote: > > As commented in the other place, I want to make the Blink API consistent with > MemoryCoordinatorClient's API. So I'd prefer just redirecting the > OnMemoryStateChanged API to Blink instead of translating it to setAllocationMode > or something. > > That requires duplicate enums in public/web which I don't want to introduce. We'll end up translating the enum to actual actions and I think this is an appropriate boundary. https://codereview.chromium.org/2094583002/diff/20001/components/memory_coord... File components/memory_coordinator/child/child_memory_coordinator_impl.cc (right): https://codereview.chromium.org/2094583002/diff/20001/components/memory_coord... components/memory_coordinator/child/child_memory_coordinator_impl.cc:15: new ChildMemoryCoordinatorImpl(std::move(request), clients); On 2016/06/27 02:17:07, haraken wrote: > > Nit: new ChildMemoryCoordinatorImpl(std::move(request), (new ClientList) Done. https://codereview.chromium.org/2094583002/diff/20001/components/memory_coord... components/memory_coordinator/child/child_memory_coordinator_impl.cc:23: blink_client_(new BlinkMemoryCoordinatorClient) { On 2016/06/27 02:17:07, haraken wrote: > > Do we need the member variable |blink_client_|? Someone needs to have the ownership of BlinkMemoryCoordinatorClient because ClientList doesn't take the ownership. I think ChildMemoryCoordinatorImpl is a good place. https://codereview.chromium.org/2094583002/diff/20001/components/memory_coord... File components/memory_coordinator/public/interfaces/child_memory_coordinator.mojom (right): https://codereview.chromium.org/2094583002/diff/20001/components/memory_coord... components/memory_coordinator/public/interfaces/child_memory_coordinator.mojom:24: GetLastState() => (MemoryState state); On 2016/06/27 02:17:07, haraken wrote: > > GetLastState => GetCurrentState ? Done. https://codereview.chromium.org/2094583002/diff/20001/content/public/common/c... File content/public/common/content_switches.cc (right): https://codereview.chromium.org/2094583002/diff/20001/content/public/common/c... content/public/common/content_switches.cc:410: const char kEnableMemoryCoordinator[] = "enable-memory-coordinator"; I'll switch to use base::FeatureList
https://codereview.chromium.org/2094583002/diff/20001/components/memory_coord... File components/memory_coordinator/child/blink_memory_coordinator_client.cc (right): https://codereview.chromium.org/2094583002/diff/20001/components/memory_coord... components/memory_coordinator/child/blink_memory_coordinator_client.cc:24: blink::WebMemoryCoordinator::setAllocationMode( On 2016/06/27 03:49:29, bashi1 wrote: > On 2016/06/27 02:17:07, haraken wrote: > > > > As commented in the other place, I want to make the Blink API consistent with > > MemoryCoordinatorClient's API. So I'd prefer just redirecting the > > OnMemoryStateChanged API to Blink instead of translating it to > setAllocationMode > > or something. > > > > > > That requires duplicate enums in public/web which I don't want to introduce. > We'll end up translating the enum to actual actions and I think this is an > appropriate boundary. But you're already introducing WebMemoryAllocationMode::Normal etc? I don't think we can avoid duplicating the enum. A couple of questions: - Are you going to add a purging mode to setAllocationMode? - Are you going to add setAllocationMode to all clients (cc, skia, v8 etc)? - What's a difference between "onMemoryStateChanged" and "setAllocationMode"?
https://codereview.chromium.org/2094583002/diff/20001/components/memory_coord... File components/memory_coordinator/child/blink_memory_coordinator_client.cc (right): https://codereview.chromium.org/2094583002/diff/20001/components/memory_coord... components/memory_coordinator/child/blink_memory_coordinator_client.cc:24: blink::WebMemoryCoordinator::setAllocationMode( On 2016/06/27 04:48:33, haraken wrote: > On 2016/06/27 03:49:29, bashi1 wrote: > > On 2016/06/27 02:17:07, haraken wrote: > > > > > > As commented in the other place, I want to make the Blink API consistent > with > > > MemoryCoordinatorClient's API. So I'd prefer just redirecting the > > > OnMemoryStateChanged API to Blink instead of translating it to > > setAllocationMode > > > or something. > > > > > > > > > > That requires duplicate enums in public/web which I don't want to introduce. > > We'll end up translating the enum to actual actions and I think this is an > > appropriate boundary. > > But you're already introducing WebMemoryAllocationMode::Normal etc? I don't > think we can avoid duplicating the enum. It's not going to be the same as mojom::MemoryState. I don't plan to add Purge enum. Instead, I'll add purge() (or similar) API in WebMemoryCoordinator. > > A couple of questions: > > - Are you going to add a purging mode to setAllocationMode? No, as described above. > > - Are you going to add setAllocationMode to all clients (cc, skia, v8 etc)? No. I may do similar things but if ethese components already provide APIs for purging/throttling I'll just use it. > > - What's a difference between "onMemoryStateChanged" and "setAllocationMode"? The former is notification, the later is action. At some point we need to have swich or if statements to convert notification to action, and I think mc -> blink boundary would be a good place to do that.
On 2016/06/27 06:36:46, bashi1 wrote: > https://codereview.chromium.org/2094583002/diff/20001/components/memory_coord... > File components/memory_coordinator/child/blink_memory_coordinator_client.cc > (right): > > https://codereview.chromium.org/2094583002/diff/20001/components/memory_coord... > components/memory_coordinator/child/blink_memory_coordinator_client.cc:24: > blink::WebMemoryCoordinator::setAllocationMode( > On 2016/06/27 04:48:33, haraken wrote: > > On 2016/06/27 03:49:29, bashi1 wrote: > > > On 2016/06/27 02:17:07, haraken wrote: > > > > > > > > As commented in the other place, I want to make the Blink API consistent > > with > > > > MemoryCoordinatorClient's API. So I'd prefer just redirecting the > > > > OnMemoryStateChanged API to Blink instead of translating it to > > > setAllocationMode > > > > or something. > > > > > > > > > > > > > > That requires duplicate enums in public/web which I don't want to introduce. > > > We'll end up translating the enum to actual actions and I think this is an > > > appropriate boundary. > > > > But you're already introducing WebMemoryAllocationMode::Normal etc? I don't > > think we can avoid duplicating the enum. > It's not going to be the same as mojom::MemoryState. I don't plan to add Purge > enum. Instead, I'll add purge() (or similar) API in WebMemoryCoordinator. > > > > A couple of questions: > > > > - Are you going to add a purging mode to setAllocationMode? > No, as described above. > > > > - Are you going to add setAllocationMode to all clients (cc, skia, v8 etc)? > No. I may do similar things but if ethese components already provide APIs for > purging/throttling I'll just use it. > > > > - What's a difference between "onMemoryStateChanged" and "setAllocationMode"? > The former is notification, the later is action. > > At some point we need to have swich or if statements to convert notification to > action, and I think mc -> blink boundary would be a good place to do that. I was assuming that we would do the conversion in each allocator component. If we do that, will it mess up many allocators? If it will, I'm okay with doing the conversion from the notification to the actions, but then I want to make the APIs for the actions consistent throughout all the allocators (i.e., add setAllocationMode to cc, skia, v8 etc). My main point is that I want to make allocator-facing APIs consistent.
On 2016/06/27 06:43:15, haraken wrote: > On 2016/06/27 06:36:46, bashi1 wrote: > > > https://codereview.chromium.org/2094583002/diff/20001/components/memory_coord... > > File components/memory_coordinator/child/blink_memory_coordinator_client.cc > > (right): > > > > > https://codereview.chromium.org/2094583002/diff/20001/components/memory_coord... > > components/memory_coordinator/child/blink_memory_coordinator_client.cc:24: > > blink::WebMemoryCoordinator::setAllocationMode( > > On 2016/06/27 04:48:33, haraken wrote: > > > On 2016/06/27 03:49:29, bashi1 wrote: > > > > On 2016/06/27 02:17:07, haraken wrote: > > > > > > > > > > As commented in the other place, I want to make the Blink API consistent > > > with > > > > > MemoryCoordinatorClient's API. So I'd prefer just redirecting the > > > > > OnMemoryStateChanged API to Blink instead of translating it to > > > > setAllocationMode > > > > > or something. > > > > > > > > > > > > > > > > > > That requires duplicate enums in public/web which I don't want to > introduce. > > > > We'll end up translating the enum to actual actions and I think this is an > > > > appropriate boundary. > > > > > > But you're already introducing WebMemoryAllocationMode::Normal etc? I don't > > > think we can avoid duplicating the enum. > > It's not going to be the same as mojom::MemoryState. I don't plan to add Purge > > enum. Instead, I'll add purge() (or similar) API in WebMemoryCoordinator. > > > > > > A couple of questions: > > > > > > - Are you going to add a purging mode to setAllocationMode? > > No, as described above. > > > > > > - Are you going to add setAllocationMode to all clients (cc, skia, v8 etc)? > > No. I may do similar things but if ethese components already provide APIs for > > purging/throttling I'll just use it. > > > > > > - What's a difference between "onMemoryStateChanged" and > "setAllocationMode"? > > The former is notification, the later is action. > > > > At some point we need to have swich or if statements to convert notification > to > > action, and I think mc -> blink boundary would be a good place to do that. > > I was assuming that we would do the conversion in each allocator component. If > we do that, will it mess up many allocators? > > If it will, I'm okay with doing the conversion from the notification to the > actions, but then I want to make the APIs for the actions consistent throughout > all the allocators (i.e., add setAllocationMode to cc, skia, v8 etc). > > My main point is that I want to make allocator-facing APIs consistent. Allocator is ambiguous in this context. If allocator == components, we already have MemoryCoordinatorClient for consistency. What the actual meaning of allocator? And what's the benefit of having the consistent APIs even we already have MemoryCoordinatorClient?
On 2016/06/27 06:50:55, bashi1 wrote: > On 2016/06/27 06:43:15, haraken wrote: > > On 2016/06/27 06:36:46, bashi1 wrote: > > > > > > https://codereview.chromium.org/2094583002/diff/20001/components/memory_coord... > > > File components/memory_coordinator/child/blink_memory_coordinator_client.cc > > > (right): > > > > > > > > > https://codereview.chromium.org/2094583002/diff/20001/components/memory_coord... > > > components/memory_coordinator/child/blink_memory_coordinator_client.cc:24: > > > blink::WebMemoryCoordinator::setAllocationMode( > > > On 2016/06/27 04:48:33, haraken wrote: > > > > On 2016/06/27 03:49:29, bashi1 wrote: > > > > > On 2016/06/27 02:17:07, haraken wrote: > > > > > > > > > > > > As commented in the other place, I want to make the Blink API > consistent > > > > with > > > > > > MemoryCoordinatorClient's API. So I'd prefer just redirecting the > > > > > > OnMemoryStateChanged API to Blink instead of translating it to > > > > > setAllocationMode > > > > > > or something. > > > > > > > > > > > > > > > > > > > > > > That requires duplicate enums in public/web which I don't want to > > introduce. > > > > > We'll end up translating the enum to actual actions and I think this is > an > > > > > appropriate boundary. > > > > > > > > But you're already introducing WebMemoryAllocationMode::Normal etc? I > don't > > > > think we can avoid duplicating the enum. > > > It's not going to be the same as mojom::MemoryState. I don't plan to add > Purge > > > enum. Instead, I'll add purge() (or similar) API in WebMemoryCoordinator. > > > > > > > > A couple of questions: > > > > > > > > - Are you going to add a purging mode to setAllocationMode? > > > No, as described above. > > > > > > > > - Are you going to add setAllocationMode to all clients (cc, skia, v8 > etc)? > > > No. I may do similar things but if ethese components already provide APIs > for > > > purging/throttling I'll just use it. > > > > > > > > - What's a difference between "onMemoryStateChanged" and > > "setAllocationMode"? > > > The former is notification, the later is action. > > > > > > At some point we need to have swich or if statements to convert notification > > to > > > action, and I think mc -> blink boundary would be a good place to do that. > > > > I was assuming that we would do the conversion in each allocator component. If > > we do that, will it mess up many allocators? > > > > If it will, I'm okay with doing the conversion from the notification to the > > actions, but then I want to make the APIs for the actions consistent > throughout > > all the allocators (i.e., add setAllocationMode to cc, skia, v8 etc). > > > > My main point is that I want to make allocator-facing APIs consistent. > > Allocator is ambiguous in this context. If allocator == components, we already > have MemoryCoordinatorClient for consistency. What the actual meaning of > allocator? And what's the benefit of having the consistent APIs even we already > have MemoryCoordinatorClient? I meant "a component that implements logic to throttle/unthrottle/suspend its memory" by allocator. It includes PartitionAlloc, FontCache etc. I'd ask the question in the other way. What's the benefit of introducing different APIs for those allocators? As far as I understand, one of the goals of MemoryCoordinator is that the API is actionable. If the API is just a notification and not actionable, I'd say that it's a bug of the API. (i.e., my preference is to design the API so that everything can just use the API.)
On 2016/06/27 07:52:11, haraken wrote: > On 2016/06/27 06:50:55, bashi1 wrote: > > On 2016/06/27 06:43:15, haraken wrote: > > > On 2016/06/27 06:36:46, bashi1 wrote: > > > > > > > > > > https://codereview.chromium.org/2094583002/diff/20001/components/memory_coord... > > > > File > components/memory_coordinator/child/blink_memory_coordinator_client.cc > > > > (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2094583002/diff/20001/components/memory_coord... > > > > components/memory_coordinator/child/blink_memory_coordinator_client.cc:24: > > > > blink::WebMemoryCoordinator::setAllocationMode( > > > > On 2016/06/27 04:48:33, haraken wrote: > > > > > On 2016/06/27 03:49:29, bashi1 wrote: > > > > > > On 2016/06/27 02:17:07, haraken wrote: > > > > > > > > > > > > > > As commented in the other place, I want to make the Blink API > > consistent > > > > > with > > > > > > > MemoryCoordinatorClient's API. So I'd prefer just redirecting the > > > > > > > OnMemoryStateChanged API to Blink instead of translating it to > > > > > > setAllocationMode > > > > > > > or something. > > > > > > > > > > > > > > > > > > > > > > > > > > That requires duplicate enums in public/web which I don't want to > > > introduce. > > > > > > We'll end up translating the enum to actual actions and I think this > is > > an > > > > > > appropriate boundary. > > > > > > > > > > But you're already introducing WebMemoryAllocationMode::Normal etc? I > > don't > > > > > think we can avoid duplicating the enum. > > > > It's not going to be the same as mojom::MemoryState. I don't plan to add > > Purge > > > > enum. Instead, I'll add purge() (or similar) API in WebMemoryCoordinator. > > > > > > > > > > A couple of questions: > > > > > > > > > > - Are you going to add a purging mode to setAllocationMode? > > > > No, as described above. > > > > > > > > > > - Are you going to add setAllocationMode to all clients (cc, skia, v8 > > etc)? > > > > No. I may do similar things but if ethese components already provide APIs > > for > > > > purging/throttling I'll just use it. > > > > > > > > > > - What's a difference between "onMemoryStateChanged" and > > > "setAllocationMode"? > > > > The former is notification, the later is action. > > > > > > > > At some point we need to have swich or if statements to convert > notification > > > to > > > > action, and I think mc -> blink boundary would be a good place to do that. > > > > > > I was assuming that we would do the conversion in each allocator component. > If > > > we do that, will it mess up many allocators? > > > > > > If it will, I'm okay with doing the conversion from the notification to the > > > actions, but then I want to make the APIs for the actions consistent > > throughout > > > all the allocators (i.e., add setAllocationMode to cc, skia, v8 etc). > > > > > > My main point is that I want to make allocator-facing APIs consistent. > > > > Allocator is ambiguous in this context. If allocator == components, we already > > have MemoryCoordinatorClient for consistency. What the actual meaning of > > allocator? And what's the benefit of having the consistent APIs even we > already > > have MemoryCoordinatorClient? > > I meant "a component that implements logic to throttle/unthrottle/suspend its > memory" by allocator. It includes PartitionAlloc, FontCache etc. I'm afraid that this would mess up code base. We'll add a lot of clients which only have one line (e.g. invalidate() for FontCacheMemoryCoordiantorClient) for purge/throttle/whatever if we take this approach. I'm thinking one client for Blink, specifically core/dom/MemoryCoordinator. > > I'd ask the question in the other way. What's the benefit of introducing > different APIs for those allocators? As far as I understand, one of the goals of > MemoryCoordinator is that the API is actionable. If the API is just a > notification and not actionable, I'd say that it's a bug of the API. (i.e., my > preference is to design the API so that everything can just use the API.) No redundant abstraction. I don't think that ChildMemoryCoordinator need such granularity (awareness of FontCache, PartitionAlloc). CMC can just ask BlinkMemoryCoordinator to purge/throttle/whatever defined in MemoryCoordinatorClient. That's the memory coordinator API in my mind. Could you elaborate pros of your approach?
On 2016/06/27 08:04:22, bashi1 wrote: > On 2016/06/27 07:52:11, haraken wrote: > > On 2016/06/27 06:50:55, bashi1 wrote: > > > On 2016/06/27 06:43:15, haraken wrote: > > > > On 2016/06/27 06:36:46, bashi1 wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/2094583002/diff/20001/components/memory_coord... > > > > > File > > components/memory_coordinator/child/blink_memory_coordinator_client.cc > > > > > (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2094583002/diff/20001/components/memory_coord... > > > > > > components/memory_coordinator/child/blink_memory_coordinator_client.cc:24: > > > > > blink::WebMemoryCoordinator::setAllocationMode( > > > > > On 2016/06/27 04:48:33, haraken wrote: > > > > > > On 2016/06/27 03:49:29, bashi1 wrote: > > > > > > > On 2016/06/27 02:17:07, haraken wrote: > > > > > > > > > > > > > > > > As commented in the other place, I want to make the Blink API > > > consistent > > > > > > with > > > > > > > > MemoryCoordinatorClient's API. So I'd prefer just redirecting the > > > > > > > > OnMemoryStateChanged API to Blink instead of translating it to > > > > > > > setAllocationMode > > > > > > > > or something. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > That requires duplicate enums in public/web which I don't want to > > > > introduce. > > > > > > > We'll end up translating the enum to actual actions and I think this > > is > > > an > > > > > > > appropriate boundary. > > > > > > > > > > > > But you're already introducing WebMemoryAllocationMode::Normal etc? I > > > don't > > > > > > think we can avoid duplicating the enum. > > > > > It's not going to be the same as mojom::MemoryState. I don't plan to add > > > Purge > > > > > enum. Instead, I'll add purge() (or similar) API in > WebMemoryCoordinator. > > > > > > > > > > > > A couple of questions: > > > > > > > > > > > > - Are you going to add a purging mode to setAllocationMode? > > > > > No, as described above. > > > > > > > > > > > > - Are you going to add setAllocationMode to all clients (cc, skia, v8 > > > etc)? > > > > > No. I may do similar things but if ethese components already provide > APIs > > > for > > > > > purging/throttling I'll just use it. > > > > > > > > > > > > - What's a difference between "onMemoryStateChanged" and > > > > "setAllocationMode"? > > > > > The former is notification, the later is action. > > > > > > > > > > At some point we need to have swich or if statements to convert > > notification > > > > to > > > > > action, and I think mc -> blink boundary would be a good place to do > that. > > > > > > > > I was assuming that we would do the conversion in each allocator > component. > > If > > > > we do that, will it mess up many allocators? > > > > > > > > If it will, I'm okay with doing the conversion from the notification to > the > > > > actions, but then I want to make the APIs for the actions consistent > > > throughout > > > > all the allocators (i.e., add setAllocationMode to cc, skia, v8 etc). > > > > > > > > My main point is that I want to make allocator-facing APIs consistent. > > > > > > Allocator is ambiguous in this context. If allocator == components, we > already > > > have MemoryCoordinatorClient for consistency. What the actual meaning of > > > allocator? And what's the benefit of having the consistent APIs even we > > already > > > have MemoryCoordinatorClient? > > > > I meant "a component that implements logic to throttle/unthrottle/suspend its > > memory" by allocator. It includes PartitionAlloc, FontCache etc. > I'm afraid that this would mess up code base. We'll add a lot of clients which > only have one line (e.g. invalidate() for FontCacheMemoryCoordiantorClient) for > purge/throttle/whatever if we take this approach. I'm thinking one client for > Blink, specifically core/dom/MemoryCoordinator. > > > > I'd ask the question in the other way. What's the benefit of introducing > > different APIs for those allocators? As far as I understand, one of the goals > of > > MemoryCoordinator is that the API is actionable. If the API is just a > > notification and not actionable, I'd say that it's a bug of the API. (i.e., my > > preference is to design the API so that everything can just use the API.) > No redundant abstraction. I don't think that ChildMemoryCoordinator need such > granularity (awareness of FontCache, PartitionAlloc). CMC can just ask > BlinkMemoryCoordinator to purge/throttle/whatever defined in > MemoryCoordinatorClient. That's the memory coordinator API in my mind. Could you > elaborate pros of your approach? For consistency. It would be easier to understand if all allocators provide methods for purge/throttle/whatever than they provide random methods for controlling their memory. If you really want to split the notification API from the action API, I won't object to the idea. But even in that case, I want to make the action API consistent.
On 2016/06/27 08:11:21, haraken wrote: > On 2016/06/27 08:04:22, bashi1 wrote: > > On 2016/06/27 07:52:11, haraken wrote: > > > On 2016/06/27 06:50:55, bashi1 wrote: > > > > On 2016/06/27 06:43:15, haraken wrote: > > > > > On 2016/06/27 06:36:46, bashi1 wrote: > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2094583002/diff/20001/components/memory_coord... > > > > > > File > > > components/memory_coordinator/child/blink_memory_coordinator_client.cc > > > > > > (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2094583002/diff/20001/components/memory_coord... > > > > > > > > components/memory_coordinator/child/blink_memory_coordinator_client.cc:24: > > > > > > blink::WebMemoryCoordinator::setAllocationMode( > > > > > > On 2016/06/27 04:48:33, haraken wrote: > > > > > > > On 2016/06/27 03:49:29, bashi1 wrote: > > > > > > > > On 2016/06/27 02:17:07, haraken wrote: > > > > > > > > > > > > > > > > > > As commented in the other place, I want to make the Blink API > > > > consistent > > > > > > > with > > > > > > > > > MemoryCoordinatorClient's API. So I'd prefer just redirecting > the > > > > > > > > > OnMemoryStateChanged API to Blink instead of translating it to > > > > > > > > setAllocationMode > > > > > > > > > or something. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > That requires duplicate enums in public/web which I don't want to > > > > > introduce. > > > > > > > > We'll end up translating the enum to actual actions and I think > this > > > is > > > > an > > > > > > > > appropriate boundary. > > > > > > > > > > > > > > But you're already introducing WebMemoryAllocationMode::Normal etc? > I > > > > don't > > > > > > > think we can avoid duplicating the enum. > > > > > > It's not going to be the same as mojom::MemoryState. I don't plan to > add > > > > Purge > > > > > > enum. Instead, I'll add purge() (or similar) API in > > WebMemoryCoordinator. > > > > > > > > > > > > > > A couple of questions: > > > > > > > > > > > > > > - Are you going to add a purging mode to setAllocationMode? > > > > > > No, as described above. > > > > > > > > > > > > > > - Are you going to add setAllocationMode to all clients (cc, skia, > v8 > > > > etc)? > > > > > > No. I may do similar things but if ethese components already provide > > APIs > > > > for > > > > > > purging/throttling I'll just use it. > > > > > > > > > > > > > > - What's a difference between "onMemoryStateChanged" and > > > > > "setAllocationMode"? > > > > > > The former is notification, the later is action. > > > > > > > > > > > > At some point we need to have swich or if statements to convert > > > notification > > > > > to > > > > > > action, and I think mc -> blink boundary would be a good place to do > > that. > > > > > > > > > > I was assuming that we would do the conversion in each allocator > > component. > > > If > > > > > we do that, will it mess up many allocators? > > > > > > > > > > If it will, I'm okay with doing the conversion from the notification to > > the > > > > > actions, but then I want to make the APIs for the actions consistent > > > > throughout > > > > > all the allocators (i.e., add setAllocationMode to cc, skia, v8 etc). > > > > > > > > > > My main point is that I want to make allocator-facing APIs consistent. > > > > > > > > Allocator is ambiguous in this context. If allocator == components, we > > already > > > > have MemoryCoordinatorClient for consistency. What the actual meaning of > > > > allocator? And what's the benefit of having the consistent APIs even we > > > already > > > > have MemoryCoordinatorClient? > > > > > > I meant "a component that implements logic to throttle/unthrottle/suspend > its > > > memory" by allocator. It includes PartitionAlloc, FontCache etc. > > I'm afraid that this would mess up code base. We'll add a lot of clients which > > only have one line (e.g. invalidate() for FontCacheMemoryCoordiantorClient) > for > > purge/throttle/whatever if we take this approach. I'm thinking one client for > > Blink, specifically core/dom/MemoryCoordinator. > > > > > > I'd ask the question in the other way. What's the benefit of introducing > > > different APIs for those allocators? As far as I understand, one of the > goals > > of > > > MemoryCoordinator is that the API is actionable. If the API is just a > > > notification and not actionable, I'd say that it's a bug of the API. (i.e., > my > > > preference is to design the API so that everything can just use the API.) > > No redundant abstraction. I don't think that ChildMemoryCoordinator need such > > granularity (awareness of FontCache, PartitionAlloc). CMC can just ask > > BlinkMemoryCoordinator to purge/throttle/whatever defined in > > MemoryCoordinatorClient. That's the memory coordinator API in my mind. Could > you > > elaborate pros of your approach? > > For consistency. It would be easier to understand if all allocators provide > methods for purge/throttle/whatever than they provide random methods for > controlling their memory. My proposal doesn't introduce any inconsistency. Let me clarify what kind of clients we will have in renderers (in my proposal): v8, cc, skia, blink These will have the same API. I guess that in your mind, client would be: v8, cc, skia, partition_alloc, oilpan, font_cache, memory_cache, ... This doesn't seem to have the same levels of abstraction, and will introduce unnecessary abstraction (Adding {FontCache,MemoryCache,Oilpan,PartitionAlloc}MemoryCoordinator instead of one BlinkMemoryCoordinator) > > If you really want to split the notification API from the action API, I won't > object to the idea. But even in that case, I want to make the action API > consistent.
On 2016/06/27 08:20:02, bashi1 wrote: > On 2016/06/27 08:11:21, haraken wrote: > > On 2016/06/27 08:04:22, bashi1 wrote: > > > On 2016/06/27 07:52:11, haraken wrote: > > > > On 2016/06/27 06:50:55, bashi1 wrote: > > > > > On 2016/06/27 06:43:15, haraken wrote: > > > > > > On 2016/06/27 06:36:46, bashi1 wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2094583002/diff/20001/components/memory_coord... > > > > > > > File > > > > components/memory_coordinator/child/blink_memory_coordinator_client.cc > > > > > > > (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2094583002/diff/20001/components/memory_coord... > > > > > > > > > > components/memory_coordinator/child/blink_memory_coordinator_client.cc:24: > > > > > > > blink::WebMemoryCoordinator::setAllocationMode( > > > > > > > On 2016/06/27 04:48:33, haraken wrote: > > > > > > > > On 2016/06/27 03:49:29, bashi1 wrote: > > > > > > > > > On 2016/06/27 02:17:07, haraken wrote: > > > > > > > > > > > > > > > > > > > > As commented in the other place, I want to make the Blink API > > > > > consistent > > > > > > > > with > > > > > > > > > > MemoryCoordinatorClient's API. So I'd prefer just redirecting > > the > > > > > > > > > > OnMemoryStateChanged API to Blink instead of translating it to > > > > > > > > > setAllocationMode > > > > > > > > > > or something. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > That requires duplicate enums in public/web which I don't want > to > > > > > > introduce. > > > > > > > > > We'll end up translating the enum to actual actions and I think > > this > > > > is > > > > > an > > > > > > > > > appropriate boundary. > > > > > > > > > > > > > > > > But you're already introducing WebMemoryAllocationMode::Normal > etc? > > I > > > > > don't > > > > > > > > think we can avoid duplicating the enum. > > > > > > > It's not going to be the same as mojom::MemoryState. I don't plan to > > add > > > > > Purge > > > > > > > enum. Instead, I'll add purge() (or similar) API in > > > WebMemoryCoordinator. > > > > > > > > > > > > > > > > A couple of questions: > > > > > > > > > > > > > > > > - Are you going to add a purging mode to setAllocationMode? > > > > > > > No, as described above. > > > > > > > > > > > > > > > > - Are you going to add setAllocationMode to all clients (cc, skia, > > v8 > > > > > etc)? > > > > > > > No. I may do similar things but if ethese components already provide > > > APIs > > > > > for > > > > > > > purging/throttling I'll just use it. > > > > > > > > > > > > > > > > - What's a difference between "onMemoryStateChanged" and > > > > > > "setAllocationMode"? > > > > > > > The former is notification, the later is action. > > > > > > > > > > > > > > At some point we need to have swich or if statements to convert > > > > notification > > > > > > to > > > > > > > action, and I think mc -> blink boundary would be a good place to do > > > that. > > > > > > > > > > > > I was assuming that we would do the conversion in each allocator > > > component. > > > > If > > > > > > we do that, will it mess up many allocators? > > > > > > > > > > > > If it will, I'm okay with doing the conversion from the notification > to > > > the > > > > > > actions, but then I want to make the APIs for the actions consistent > > > > > throughout > > > > > > all the allocators (i.e., add setAllocationMode to cc, skia, v8 etc). > > > > > > > > > > > > My main point is that I want to make allocator-facing APIs consistent. > > > > > > > > > > Allocator is ambiguous in this context. If allocator == components, we > > > already > > > > > have MemoryCoordinatorClient for consistency. What the actual meaning of > > > > > allocator? And what's the benefit of having the consistent APIs even we > > > > already > > > > > have MemoryCoordinatorClient? > > > > > > > > I meant "a component that implements logic to throttle/unthrottle/suspend > > its > > > > memory" by allocator. It includes PartitionAlloc, FontCache etc. > > > I'm afraid that this would mess up code base. We'll add a lot of clients > which > > > only have one line (e.g. invalidate() for FontCacheMemoryCoordiantorClient) > > for > > > purge/throttle/whatever if we take this approach. I'm thinking one client > for > > > Blink, specifically core/dom/MemoryCoordinator. > > > > > > > > I'd ask the question in the other way. What's the benefit of introducing > > > > different APIs for those allocators? As far as I understand, one of the > > goals > > > of > > > > MemoryCoordinator is that the API is actionable. If the API is just a > > > > notification and not actionable, I'd say that it's a bug of the API. > (i.e., > > my > > > > preference is to design the API so that everything can just use the API.) > > > No redundant abstraction. I don't think that ChildMemoryCoordinator need > such > > > granularity (awareness of FontCache, PartitionAlloc). CMC can just ask > > > BlinkMemoryCoordinator to purge/throttle/whatever defined in > > > MemoryCoordinatorClient. That's the memory coordinator API in my mind. Could > > you > > > elaborate pros of your approach? > > > > For consistency. It would be easier to understand if all allocators provide > > methods for purge/throttle/whatever than they provide random methods for > > controlling their memory. > My proposal doesn't introduce any inconsistency. Let me clarify what kind of > clients we will have in renderers (in my proposal): > > v8, cc, skia, blink > > These will have the same API. I guess that in your mind, client would be: > > v8, cc, skia, partition_alloc, oilpan, font_cache, memory_cache, ... > > This doesn't seem to have the same levels of abstraction, and will introduce > unnecessary abstraction (Adding > {FontCache,MemoryCache,Oilpan,PartitionAlloc}MemoryCoordinator instead of one > BlinkMemoryCoordinator) I'm not saying that we should add the MC abstraction to all the clients. I just want to keep the API shape as consistent as possible. What's a problem of consistently using the onMemoryStateChanged() API for all the clients (which don't need to inherit from MC)? If you think that the onMemoryStateChanged() API is not convenient, I'd say we should revise the API.
On 2016/06/27 08:27:15, haraken wrote: > On 2016/06/27 08:20:02, bashi1 wrote: > > On 2016/06/27 08:11:21, haraken wrote: > > > On 2016/06/27 08:04:22, bashi1 wrote: > > > > On 2016/06/27 07:52:11, haraken wrote: > > > > > On 2016/06/27 06:50:55, bashi1 wrote: > > > > > > On 2016/06/27 06:43:15, haraken wrote: > > > > > > > On 2016/06/27 06:36:46, bashi1 wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2094583002/diff/20001/components/memory_coord... > > > > > > > > File > > > > > components/memory_coordinator/child/blink_memory_coordinator_client.cc > > > > > > > > (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2094583002/diff/20001/components/memory_coord... > > > > > > > > > > > > components/memory_coordinator/child/blink_memory_coordinator_client.cc:24: > > > > > > > > blink::WebMemoryCoordinator::setAllocationMode( > > > > > > > > On 2016/06/27 04:48:33, haraken wrote: > > > > > > > > > On 2016/06/27 03:49:29, bashi1 wrote: > > > > > > > > > > On 2016/06/27 02:17:07, haraken wrote: > > > > > > > > > > > > > > > > > > > > > > As commented in the other place, I want to make the Blink > API > > > > > > consistent > > > > > > > > > with > > > > > > > > > > > MemoryCoordinatorClient's API. So I'd prefer just > redirecting > > > the > > > > > > > > > > > OnMemoryStateChanged API to Blink instead of translating it > to > > > > > > > > > > setAllocationMode > > > > > > > > > > > or something. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > That requires duplicate enums in public/web which I don't want > > to > > > > > > > introduce. > > > > > > > > > > We'll end up translating the enum to actual actions and I > think > > > this > > > > > is > > > > > > an > > > > > > > > > > appropriate boundary. > > > > > > > > > > > > > > > > > > But you're already introducing WebMemoryAllocationMode::Normal > > etc? > > > I > > > > > > don't > > > > > > > > > think we can avoid duplicating the enum. > > > > > > > > It's not going to be the same as mojom::MemoryState. I don't plan > to > > > add > > > > > > Purge > > > > > > > > enum. Instead, I'll add purge() (or similar) API in > > > > WebMemoryCoordinator. > > > > > > > > > > > > > > > > > > A couple of questions: > > > > > > > > > > > > > > > > > > - Are you going to add a purging mode to setAllocationMode? > > > > > > > > No, as described above. > > > > > > > > > > > > > > > > > > - Are you going to add setAllocationMode to all clients (cc, > skia, > > > v8 > > > > > > etc)? > > > > > > > > No. I may do similar things but if ethese components already > provide > > > > APIs > > > > > > for > > > > > > > > purging/throttling I'll just use it. > > > > > > > > > > > > > > > > > > - What's a difference between "onMemoryStateChanged" and > > > > > > > "setAllocationMode"? > > > > > > > > The former is notification, the later is action. > > > > > > > > > > > > > > > > At some point we need to have swich or if statements to convert > > > > > notification > > > > > > > to > > > > > > > > action, and I think mc -> blink boundary would be a good place to > do > > > > that. > > > > > > > > > > > > > > I was assuming that we would do the conversion in each allocator > > > > component. > > > > > If > > > > > > > we do that, will it mess up many allocators? > > > > > > > > > > > > > > If it will, I'm okay with doing the conversion from the notification > > to > > > > the > > > > > > > actions, but then I want to make the APIs for the actions consistent > > > > > > throughout > > > > > > > all the allocators (i.e., add setAllocationMode to cc, skia, v8 > etc). > > > > > > > > > > > > > > My main point is that I want to make allocator-facing APIs > consistent. > > > > > > > > > > > > Allocator is ambiguous in this context. If allocator == components, we > > > > already > > > > > > have MemoryCoordinatorClient for consistency. What the actual meaning > of > > > > > > allocator? And what's the benefit of having the consistent APIs even > we > > > > > already > > > > > > have MemoryCoordinatorClient? > > > > > > > > > > I meant "a component that implements logic to > throttle/unthrottle/suspend > > > its > > > > > memory" by allocator. It includes PartitionAlloc, FontCache etc. > > > > I'm afraid that this would mess up code base. We'll add a lot of clients > > which > > > > only have one line (e.g. invalidate() for > FontCacheMemoryCoordiantorClient) > > > for > > > > purge/throttle/whatever if we take this approach. I'm thinking one client > > for > > > > Blink, specifically core/dom/MemoryCoordinator. > > > > > > > > > > I'd ask the question in the other way. What's the benefit of introducing > > > > > different APIs for those allocators? As far as I understand, one of the > > > goals > > > > of > > > > > MemoryCoordinator is that the API is actionable. If the API is just a > > > > > notification and not actionable, I'd say that it's a bug of the API. > > (i.e., > > > my > > > > > preference is to design the API so that everything can just use the > API.) > > > > No redundant abstraction. I don't think that ChildMemoryCoordinator need > > such > > > > granularity (awareness of FontCache, PartitionAlloc). CMC can just ask > > > > BlinkMemoryCoordinator to purge/throttle/whatever defined in > > > > MemoryCoordinatorClient. That's the memory coordinator API in my mind. > Could > > > you > > > > elaborate pros of your approach? > > > > > > For consistency. It would be easier to understand if all allocators provide > > > methods for purge/throttle/whatever than they provide random methods for > > > controlling their memory. > > My proposal doesn't introduce any inconsistency. Let me clarify what kind of > > clients we will have in renderers (in my proposal): > > > > v8, cc, skia, blink > > > > These will have the same API. I guess that in your mind, client would be: > > > > v8, cc, skia, partition_alloc, oilpan, font_cache, memory_cache, ... > > > > This doesn't seem to have the same levels of abstraction, and will introduce > > unnecessary abstraction (Adding > > {FontCache,MemoryCache,Oilpan,PartitionAlloc}MemoryCoordinator instead of one > > BlinkMemoryCoordinator) > > I'm not saying that we should add the MC abstraction to all the clients. I just > want to keep the API shape as consistent as possible. What's a problem of > consistently using the onMemoryStateChanged() API for all the clients (which > don't need to inherit from MC)? > > If you think that the onMemoryStateChanged() API is not convenient, I'd say we > should revise the API. Discussed offline. I'll drop blink related code from this CL. For BlinkMemoryCoordinator I'll create a separate CL when the design is stabilized.
Dropped BlinkMemoryCoordinator. chrisha@: any chance to take a look? https://codereview.chromium.org/2094583002/diff/20001/content/public/common/c... File content/public/common/content_switches.cc (right): https://codereview.chromium.org/2094583002/diff/20001/content/public/common/c... content/public/common/content_switches.cc:410: const char kEnableMemoryCoordinator[] = "enable-memory-coordinator"; On 2016/06/27 03:49:29, bashi1 wrote: > I'll switch to use base::FeatureList Done.
non-owner LGTM https://codereview.chromium.org/2094583002/diff/60001/components/memory_coord... File components/memory_coordinator/browser/memory_state_notifier.cc (right): https://codereview.chromium.org/2094583002/diff/60001/components/memory_coord... components/memory_coordinator/browser/memory_state_notifier.cc:31: &MemoryStateNotifier::RemoveChild, base::Unretained(this), id)); I'm not sure if the ID system is the best way to implement this. I'll defer the review to Chromium's experts. https://codereview.chromium.org/2094583002/diff/60001/components/memory_coord... File components/memory_coordinator/common/memory_coordinator_features.cc (right): https://codereview.chromium.org/2094583002/diff/60001/components/memory_coord... components/memory_coordinator/common/memory_coordinator_features.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. 2016
I don't think the MemoryStateNotifier is what you want or need. It grabs memory pressure notifications from the browser process and sends them all to all of the child processes. Right now messages are forwarded via logic in TabManager, and send using code in the various MemoryMessageFilter classes. For now you're interested in having messages arrive in a child memory coordinator. The easiest way to do that is to not modify anything at all in the browser. Then, using your Notifier callback mechanism, intercept memory pressure notifications coming in to renderer processes. Then, translate these to MemoryCoordinator "pressure levels", and forward them to MemoryCoordinatorClients in via the ChildProcessMemoryCoordinator. You shouldn't need to change any logic in the browser initially. We can then in parallel lift the PriorityTracker out of the TabManager, create the main browser MemoryCoordinator, create our own parallel messaging mechanism in the various MemoryMessageFilter classes (necessary so we can enable/disable/override this functionality). Then we can remove the MemoryNotifier mechanism entirely. Do you have a detailed roadmap I can look at? Would be easiest to coordinate on something like that initially. https://codereview.chromium.org/2094583002/diff/60001/components/memory_coord... File components/memory_coordinator/browser/memory_state_notifier.cc (right): https://codereview.chromium.org/2094583002/diff/60001/components/memory_coord... components/memory_coordinator/browser/memory_state_notifier.cc:31: &MemoryStateNotifier::RemoveChild, base::Unretained(this), id)); On 2016/06/27 11:16:00, haraken wrote: > > I'm not sure if the ID system is the best way to implement this. I'll defer the > review to Chromium's experts. I don't see any problem with this, but you could also just use a std::set and pass around the raw pointer to the child? https://codereview.chromium.org/2094583002/diff/60001/components/memory_coord... File components/memory_coordinator/browser/memory_state_notifier.h (right): https://codereview.chromium.org/2094583002/diff/60001/components/memory_coord... components/memory_coordinator/browser/memory_state_notifier.h:38: void RemoveChild(int id); Documentation for these functions would be helpful for somebody reading the code. (ie. RemoveChild is used as an on-error callback to remove a child when the connection dies.) https://codereview.chromium.org/2094583002/diff/60001/components/memory_coord... File components/memory_coordinator/child/child_memory_coordinator_impl.cc (right): https://codereview.chromium.org/2094583002/diff/60001/components/memory_coord... components/memory_coordinator/child/child_memory_coordinator_impl.cc:20: // TODO(bashi): Add clients (e.g. skia). Wouldn't it make more sense for clients to add themselves? ie. via a mechanism in MemoryCoordinatorClient constructor? (Something more akin to MemoryPressureListener's mechanism?) https://codereview.chromium.org/2094583002/diff/60001/components/memory_coord... components/memory_coordinator/child/child_memory_coordinator_impl.cc:31: void ChildMemoryCoordinatorImpl::GetCurrentState( Do we need this call? The state is centrally imposed from the coordinator in the browser. It shouldn't need to query the individual child process for its memory state.
On 2016/06/27 20:35:30, chrisha (slow) wrote: > I don't think the MemoryStateNotifier is what you want or need. > > It grabs memory pressure notifications from the browser process and sends them > all to all of the child processes. Right now messages are forwarded via logic in > TabManager, and send using code in the various MemoryMessageFilter classes. > > For now you're interested in having messages arrive in a child memory > coordinator. The easiest way to do that is to not modify anything at all in the > browser. Then, using your Notifier callback mechanism, intercept memory pressure > notifications coming in to renderer processes. Then, translate these to > MemoryCoordinator "pressure levels", and forward them to > MemoryCoordinatorClients in via the ChildProcessMemoryCoordinator. > > You shouldn't need to change any logic in the browser initially. We can then in > parallel lift the PriorityTracker out of the TabManager, create the main browser > MemoryCoordinator, create our own parallel messaging mechanism in the various > MemoryMessageFilter classes (necessary so we can enable/disable/override this > functionality). Then we can remove the MemoryNotifier mechanism entirely. Hmm, what I'm trying to do in V0 are: - MemoryMessage IPC is replaced with Mojo - All memory coordinator messages are routed to the browser process - browser -> browser - browser -> renderer - renderer -> browser -> renderer - Migrate MemoryPressureListener instances to MemoryCoordinatorClient - This includes browser side - Remove MemoryPressureListener It seems you are suggesting the last two things in V0. I'm not sure only doing the last two things is worth doing separately because it won't provide any structual differences. One of the key goal of memory coordinator is to have a consistent notification mechanism and I think it includes how to communicate between browser and renderers. > > Do you have a detailed roadmap I can look at? Would be easiest to coordinate on > something like that initially. The doc I sent previously partially described a roadmap (I've been revising it). https://docs.google.com/document/d/1rGeUzhfzIKwmBzUcqUEwxEpLQQRq3PulRuHwFBlVI... This doc isn't enough to answer your questions. I'll add more explanations in the doc. https://codereview.chromium.org/2094583002/diff/60001/components/memory_coord... File components/memory_coordinator/browser/memory_state_notifier.cc (right): https://codereview.chromium.org/2094583002/diff/60001/components/memory_coord... components/memory_coordinator/browser/memory_state_notifier.cc:31: &MemoryStateNotifier::RemoveChild, base::Unretained(this), id)); On 2016/06/27 20:35:30, chrisha (slow) wrote: > On 2016/06/27 11:16:00, haraken wrote: > > > > I'm not sure if the ID system is the best way to implement this. I'll defer > the > > review to Chromium's experts. > > I don't see any problem with this, but you could also just use a std::set and > pass around the raw pointer to the child? Yes. I'd just prefer using IDs but raw pointers should work. https://codereview.chromium.org/2094583002/diff/60001/components/memory_coord... File components/memory_coordinator/child/child_memory_coordinator_impl.cc (right): https://codereview.chromium.org/2094583002/diff/60001/components/memory_coord... components/memory_coordinator/child/child_memory_coordinator_impl.cc:20: // TODO(bashi): Add clients (e.g. skia). On 2016/06/27 20:35:30, chrisha (slow) wrote: > Wouldn't it make more sense for clients to add themselves? ie. via a mechanism > in MemoryCoordinatorClient constructor? (Something more akin to > MemoryPressureListener's mechanism?) It was my first attempt. MemoryPressureListener uses a singleton to add/remove listeners. On the other hand ChildMemoryCoordinator is a mojo interface impl and I couldn't figure how to access it without InterfaceRequest. https://codereview.chromium.org/2094583002/diff/60001/components/memory_coord... components/memory_coordinator/child/child_memory_coordinator_impl.cc:31: void ChildMemoryCoordinatorImpl::GetCurrentState( On 2016/06/27 20:35:30, chrisha (slow) wrote: > Do we need this call? The state is centrally imposed from the coordinator in the > browser. It shouldn't need to query the individual child process for its memory > state. This is for testing. Without it it's hard to make CL smaller. We may want to rename it as GetCurrentStateForTesting(). https://codereview.chromium.org/2094583002/diff/60001/components/memory_coord... File components/memory_coordinator/common/memory_coordinator_features.cc (right): https://codereview.chromium.org/2094583002/diff/60001/components/memory_coord... components/memory_coordinator/common/memory_coordinator_features.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2016/06/27 11:16:00, haraken wrote: > > 2016 Acknowledged.
Description was changed from ========== Use memory coordinator when --enable-memory-coordinator is specified This CL adds --enable-memory-coordinator flag which: - makes the browser connects to a ChildMemoryCoordinator when a renderer is launched - disables memory pressure listener on renderers This CL also adds a blink API for setting memory allocation mode. A ChildMemoryCoordinator in a renderer calls the API based on memory events it receives. The API is empty now and it will be implemented in follow-up CLs. BUG=617492 ========== to ========== Add MemoryCoordinator feature flag This CL adds MemoryCoordinator feature flag which: - makes the browser connects to a ChildMemoryCoordinator when a renderer is launched - disables memory pressure listener on renderers A ChildMemoryCoordinator in a renderer calls the API based on memory events it receives. The API is empty now and it will be implemented in follow-up CLs. BUG=617492 ==========
On 2016/06/27 22:40:23, bashi1 wrote: > The doc I sent previously partially described a roadmap (I've been revising it). > https://docs.google.com/document/d/1rGeUzhfzIKwmBzUcqUEwxEpLQQRq3PulRuHwFBlVI... > > This doc isn't enough to answer your questions. I'll add more explanations in > the doc. Updated.
My problem with this CL is the dispatch unconditionally from browser to all child processes. The point is for the browser to decide (inside of Memory Coordinator) which levels to apply to which renderers, in response to global memory pressure; not to blanket all renderers with the same notification. For now that logic is in TabManager [1], which should be reworked to use a mojo notification if memory coordinator is enabled (just need to replace the callback). [1] https://cs.chromium.org/chromium/src/chrome/browser/memory/tab_manager.cc?q=t...
On 2016/06/29 21:15:31, chrisha (slow) wrote: > My problem with this CL is the dispatch unconditionally from browser to all > child processes. The point is for the browser to decide (inside of Memory > Coordinator) which levels to apply to which renderers, in response to global > memory pressure; not to blanket all renderers with the same notification. > > For now that logic is in TabManager [1], which should be reworked to use a mojo > notification if memory coordinator is enabled (just need to replace the > callback). > > [1] > https://cs.chromium.org/chromium/src/chrome/browser/memory/tab_manager.cc?q=t... Hmm, I thought that it's to be done by tab manager -> priority tracker work. What in my mind was just creating basic notification mechanism for memory coordinator. I'll take closer look at tab manager then.
It will be done by the tab manager refactor, but I'd be uncomfortable turning things on this way in the meantime; it would be very heavy handed. If this is not intended to be enabled for users at all, but only internally on the way to launching the feature, then I'm fine with the notify-all-children, provided a giant warning comment with a TODO. Otherwise, I'd prefer piggybacking in the slightly broken but much more conservative existing notification mechanism. On Wed, Jun 29, 2016, 20:09 <bashi@chromium.org> wrote: > On 2016/06/29 21:15:31, chrisha (slow) wrote: > > My problem with this CL is the dispatch unconditionally from browser to > all > > child processes. The point is for the browser to decide (inside of Memory > > Coordinator) which levels to apply to which renderers, in response to > global > > memory pressure; not to blanket all renderers with the same notification. > > > > For now that logic is in TabManager [1], which should be reworked to use > a > mojo > > notification if memory coordinator is enabled (just need to replace the > > callback). > > > > [1] > > > > https://cs.chromium.org/chromium/src/chrome/browser/memory/tab_manager.cc?q=t... > > Hmm, I thought that it's to be done by tab manager -> priority tracker > work. > What in my mind was just creating basic notification mechanism for memory > coordinator. > > I'll take closer look at tab manager then. > > https://codereview.chromium.org/2094583002/ > -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
It will be done by the tab manager refactor, but I'd be uncomfortable turning things on this way in the meantime; it would be very heavy handed. If this is not intended to be enabled for users at all, but only internally on the way to launching the feature, then I'm fine with the notify-all-children, provided a giant warning comment with a TODO. Otherwise, I'd prefer piggybacking in the slightly broken but much more conservative existing notification mechanism. On Wed, Jun 29, 2016, 20:09 <bashi@chromium.org> wrote: > On 2016/06/29 21:15:31, chrisha (slow) wrote: > > My problem with this CL is the dispatch unconditionally from browser to > all > > child processes. The point is for the browser to decide (inside of Memory > > Coordinator) which levels to apply to which renderers, in response to > global > > memory pressure; not to blanket all renderers with the same notification. > > > > For now that logic is in TabManager [1], which should be reworked to use > a > mojo > > notification if memory coordinator is enabled (just need to replace the > > callback). > > > > [1] > > > > https://cs.chromium.org/chromium/src/chrome/browser/memory/tab_manager.cc?q=t... > > Hmm, I thought that it's to be done by tab manager -> priority tracker > work. > What in my mind was just creating basic notification mechanism for memory > coordinator. > > I'll take closer look at tab manager then. > > https://codereview.chromium.org/2094583002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2016/06/30 03:17:15, chrisha (slow) wrote: > It will be done by the tab manager refactor, but I'd be uncomfortable > turning things on this way in the meantime; it would be very heavy handed. > If this is not intended to be enabled for users at all, but only internally > on the way to launching the feature, then I'm fine with the > notify-all-children, provided a giant warning comment with a TODO. > > Otherwise, I'd prefer piggybacking in the slightly broken but much more > conservative existing notification mechanism. I haven't took a look at tab manager yet (as I was ooo yesterday) but let me give it a try. I'm revising this CL with some structural changes and it would take couple of hours. > > On Wed, Jun 29, 2016, 20:09 <mailto:bashi@chromium.org> wrote: > > > On 2016/06/29 21:15:31, chrisha (slow) wrote: > > > My problem with this CL is the dispatch unconditionally from browser to > > all > > > child processes. The point is for the browser to decide (inside of Memory > > > Coordinator) which levels to apply to which renderers, in response to > > global > > > memory pressure; not to blanket all renderers with the same notification. > > > > > > For now that logic is in TabManager [1], which should be reworked to use > > a > > mojo > > > notification if memory coordinator is enabled (just need to replace the > > > callback). > > > > > > [1] > > > > > > > > https://cs.chromium.org/chromium/src/chrome/browser/memory/tab_manager.cc?q=t... > > > > Hmm, I thought that it's to be done by tab manager -> priority tracker > > work. > > What in my mind was just creating basic notification mechanism for memory > > coordinator. > > > > I'll take closer look at tab manager then. > > > > https://codereview.chromium.org/2094583002/ > > > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
Description was changed from ========== Add MemoryCoordinator feature flag This CL adds MemoryCoordinator feature flag which: - makes the browser connects to a ChildMemoryCoordinator when a renderer is launched - disables memory pressure listener on renderers A ChildMemoryCoordinator in a renderer calls the API based on memory events it receives. The API is empty now and it will be implemented in follow-up CLs. BUG=617492 ========== to ========== Add MemoryCoordinator feature flag This CL adds MemoryCoordinator feature flag which creates MemoryCoordinatorImpl in the browser process. When a renderer is launched the renderer creates ChildMemoryCoordinator and connects it to the MemoryCoordinator. This CL doesn't add actual handling of memory events, which will be implemented in follow-up CLs. BUG=617492 ==========
Revised. Chris, WDYT? I made some structural changes: - Added MemoryCoordinator mojo interface to align changes in the V0 design doc. - ChildMemoryCoordinator is owned by RenderThreadImpl instead of self-deleting by StrongBinding. This enables us to add clients from anywhere in content/ by RenderThreadImpl::current()->memory_coordinator()->RegisterClient(). As for dispatching signals to renderers, this CL doesn't actually replace MemoryPressureListener so I removed notifier. The browser tests added in this CL just checks whether MC <-> CMC communication works. I'll create a separate CL for tab manager and memory coordinator integration once this is settled.
Description was changed from ========== Add MemoryCoordinator feature flag This CL adds MemoryCoordinator feature flag which creates MemoryCoordinatorImpl in the browser process. When a renderer is launched the renderer creates ChildMemoryCoordinator and connects it to the MemoryCoordinator. This CL doesn't add actual handling of memory events, which will be implemented in follow-up CLs. BUG=617492 ========== to ========== Add MemoryCoordinator This CL adds MemoryCoordinator mojo interface and its implementation. When --enable-features=MemoryCoordinator is specified the browser and renderers create their MemoryCoordinators. MemoryCoordinatorImpl (which lives in the browser process) creates a mojo message pipe when a renderer is launched. This CL doesn't add actual handling of memory events, which will be implemented in follow-up CLs. BUG=617492 ==========
chrisha@: friendly ping :)
A few more questions... https://codereview.chromium.org/2094583002/diff/120001/components/memory_coor... File components/memory_coordinator/child/child_memory_coordinator_impl.h (right): https://codereview.chromium.org/2094583002/diff/120001/components/memory_coor... components/memory_coordinator/child/child_memory_coordinator_impl.h:28: // Reigsters/unregisters a client. Does not take ownership of client. Registers* https://codereview.chromium.org/2094583002/diff/120001/content/browser/memory... File content/browser/memory/memory_coordinator_impl_browsertest.cc (right): https://codereview.chromium.org/2094583002/diff/120001/content/browser/memory... content/browser/memory/memory_coordinator_impl_browsertest.cc:26: [&state](memory_coordinator::mojom::ChildMemoryCoordinator* child) { Capturing state by ref here, and by value below? Be consistent? https://codereview.chromium.org/2094583002/diff/120001/content/browser/memory... content/browser/memory/memory_coordinator_impl_browsertest.cc:51: child->GetCurrentState( GetCurrentState is only used for testing, no? Maybe better off named GetCurrentStateForTesting? Also, why the need to pass in a callback? Can't it be a simple getter function?
https://codereview.chromium.org/2094583002/diff/120001/components/memory_coor... File components/memory_coordinator/child/child_memory_coordinator_impl.h (right): https://codereview.chromium.org/2094583002/diff/120001/components/memory_coor... components/memory_coordinator/child/child_memory_coordinator_impl.h:28: // Reigsters/unregisters a client. Does not take ownership of client. On 2016/07/07 02:53:43, chrisha (slow) wrote: > Registers* Done. https://codereview.chromium.org/2094583002/diff/120001/content/browser/memory... File content/browser/memory/memory_coordinator_impl_browsertest.cc (right): https://codereview.chromium.org/2094583002/diff/120001/content/browser/memory... content/browser/memory/memory_coordinator_impl_browsertest.cc:26: [&state](memory_coordinator::mojom::ChildMemoryCoordinator* child) { On 2016/07/07 02:53:44, chrisha (slow) wrote: > Capturing state by ref here, and by value below? Be consistent? (seems forgot to remove &) Changed to by value. https://codereview.chromium.org/2094583002/diff/120001/content/browser/memory... content/browser/memory/memory_coordinator_impl_browsertest.cc:51: child->GetCurrentState( On 2016/07/07 02:53:44, chrisha (slow) wrote: > GetCurrentState is only used for testing, no? Maybe better off named > GetCurrentStateForTesting? Renamed. > > Also, why the need to pass in a callback? Can't it be a simple getter function? This is a mojo method and a mojo method takes a callback when it returns something. Return value is passed to a callback.
Ah, forgive my lack of familiarity with Mojo IPC. lgtm!
bashi@chromium.org changed reviewers: + dcheng@chromium.org, jam@chromium.org
dcheng@: Could you do IPC security review? (comoponents/memory_coordinator/public/interfaces/memory_coordinator.mojom. This is an empty interface but I'll add methods in follow-up CLs) jam@: Could you review for content/ changes and services/shell dependency?
LGTM on my side. https://codereview.chromium.org/2094583002/diff/140001/components/memory_coor... File components/memory_coordinator/browser/memory_coordinator_impl.cc (right): https://codereview.chromium.org/2094583002/diff/140001/components/memory_coor... components/memory_coordinator/browser/memory_coordinator_impl.cc:17: children_.AddPtr(std::move(child)); Don't we need to remove the pointer when the renderer is gone? https://codereview.chromium.org/2094583002/diff/140001/components/memory_coor... File components/memory_coordinator/browser/memory_coordinator_impl.h (right): https://codereview.chromium.org/2094583002/diff/140001/components/memory_coor... components/memory_coordinator/browser/memory_coordinator_impl.h:27: void IterateChildren(FunctionType function) { Is this only for testing? Or is it going to be used in other code in the future?
https://codereview.chromium.org/2094583002/diff/140001/components/memory_coor... File components/memory_coordinator/browser/memory_coordinator_impl.cc (right): https://codereview.chromium.org/2094583002/diff/140001/components/memory_coor... components/memory_coordinator/browser/memory_coordinator_impl.cc:17: children_.AddPtr(std::move(child)); On 2016/07/08 02:40:25, haraken wrote: > > Don't we need to remove the pointer when the renderer is gone? No. InterfacePtrSet removes it when an error occurred on the connection. https://codereview.chromium.org/2094583002/diff/140001/components/memory_coor... File components/memory_coordinator/browser/memory_coordinator_impl.h (right): https://codereview.chromium.org/2094583002/diff/140001/components/memory_coor... components/memory_coordinator/browser/memory_coordinator_impl.h:27: void IterateChildren(FunctionType function) { On 2016/07/08 02:40:26, haraken wrote: > > Is this only for testing? Or is it going to be used in other code in the future? > Yes. I don't have plans to use it outside tests.
https://codereview.chromium.org/2094583002/diff/140001/components/memory_coor... File components/memory_coordinator/browser/memory_coordinator_impl.h (right): https://codereview.chromium.org/2094583002/diff/140001/components/memory_coor... components/memory_coordinator/browser/memory_coordinator_impl.h:27: void IterateChildren(FunctionType function) { On 2016/07/08 02:44:49, bashi1 wrote: > On 2016/07/08 02:40:26, haraken wrote: > > > > Is this only for testing? Or is it going to be used in other code in the > future? > > > > Yes. I don't have plans to use it outside tests. Then IterateChildrenForTesting?
https://codereview.chromium.org/2094583002/diff/140001/components/memory_coor... File components/memory_coordinator/browser/memory_coordinator_impl.h (right): https://codereview.chromium.org/2094583002/diff/140001/components/memory_coor... components/memory_coordinator/browser/memory_coordinator_impl.h:27: void IterateChildren(FunctionType function) { On 2016/07/08 02:48:54, haraken wrote: > On 2016/07/08 02:44:49, bashi1 wrote: > > On 2016/07/08 02:40:26, haraken wrote: > > > > > > Is this only for testing? Or is it going to be used in other code in the > > future? > > > > > > > Yes. I don't have plans to use it outside tests. > > Then IterateChildrenForTesting? Will do.
https://codereview.chromium.org/2094583002/diff/140001/components/memory_coor... File components/memory_coordinator/child/child_memory_coordinator_impl.cc (right): https://codereview.chromium.org/2094583002/diff/140001/components/memory_coor... components/memory_coordinator/child/child_memory_coordinator_impl.cc:18: binding_.Bind(std::move(request)); I think it is somewhat unusual that we have a persistent object (scoped to lifetime of RenderThreadImpl) that only accepts one binding request. Logically, this is OK, since the browser process should only ever connect once, but it feels a bit weird. I am going to ask chromium-mojo about best practices for this. https://codereview.chromium.org/2094583002/diff/140001/content/renderer/rende... File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/2094583002/diff/140001/content/renderer/rende... content/renderer/render_thread_impl.cc:823: base::Unretained(memory_coordinator_.get()))); Does it make sense to just have memory_coordinator_ be a base::Owned() of the callback?
https://codereview.chromium.org/2094583002/diff/140001/components/memory_coor... File components/memory_coordinator/child/child_memory_coordinator_impl.cc (right): https://codereview.chromium.org/2094583002/diff/140001/components/memory_coor... components/memory_coordinator/child/child_memory_coordinator_impl.cc:18: binding_.Bind(std::move(request)); On 2016/07/08 07:10:35, dcheng wrote: > I think it is somewhat unusual that we have a persistent object (scoped to > lifetime of RenderThreadImpl) that only accepts one binding request. Logically, > this is OK, since the browser process should only ever connect once, but it > feels a bit weird. I am going to ask chromium-mojo about best practices for > this. Yeah.. Actually, in PS#5 I didn't use this pattern (I guess PS#5 is a usual way). The reason I changed the pattern is that the browser needs to have RenderProcessHost::GetID() -> ChildMemoryCoordinatorPtr mappings so that the central MemoryCoordinator can use the logic in TabManager (which uses RenderProcessHost and old IPC) to dispatch memory pressure notification to renderers. I intentionally dropped the mappings from this CL to make this CL simple, but the next CL would add the mappings. I'm happy to change this if there is a workaround. https://codereview.chromium.org/2094583002/diff/140001/content/renderer/rende... File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/2094583002/diff/140001/content/renderer/rende... content/renderer/render_thread_impl.cc:823: base::Unretained(memory_coordinator_.get()))); On 2016/07/08 07:10:35, dcheng wrote: > Does it make sense to just have memory_coordinator_ be a base::Owned() of the > callback? I didn't know base::Owned(). Thanks. Probably we can use it (may depend on how we connect the browser and renderers).
lgtm
btw do you want this to work for other process types as well, i.e. not just renderers? can be done in a followup, but I was wondering what your plan is.
On 2016/07/09 02:36:13, jam wrote: > btw do you want this to work for other process types as well, i.e. not just > renderers? can be done in a followup, but I was wondering what your plan is. Currently I don't have a concrete plan to work for other process types but other processes types are our future targets (probably V1 or later).
PS#9 changed the way to bind mojo interfaces and its implements. It stops using a mojo interface for MemoryCoordinator. Instead, PS#9 adds MemoryCoordinatorHandle mojo interface which will be created per renderer by the browser. MemoryCoordinator manages ID -> handle mappings. This way we don't need to register ChildMemoryCoordinator as an exposed interface in renderers. dcheng@, does this make sense to you? https://codereview.chromium.org/2094583002/diff/160001/components/memory_coor... File components/memory_coordinator/public/interfaces/memory_coordinator.mojom (right): https://codereview.chromium.org/2094583002/diff/160001/components/memory_coor... components/memory_coordinator/public/interfaces/memory_coordinator.mojom:11: interface MemoryCoordinatorHandle { Maybe "handle" isn't a good naming for this interface. I couldn't come up with better name.
https://codereview.chromium.org/2094583002/diff/160001/components/memory_coor... File components/memory_coordinator/browser/memory_coordinator.h (right): https://codereview.chromium.org/2094583002/diff/160001/components/memory_coor... components/memory_coordinator/browser/memory_coordinator.h:23: void CreateHandle(int id, mojom::MemoryCoordinatorHandleRequest request); Nit: please call this rph_id or something so it's clearer what kind of ID it is. https://codereview.chromium.org/2094583002/diff/160001/components/memory_coor... components/memory_coordinator/browser/memory_coordinator.h:27: void IterateChildrenForTesting(IterateCallback callback); Nit: pass callbacks by const ref. But rather than having this, I think we should just friend a test helper that can access the state: it will make the tests very straightforward, since all the test code will be inline. https://codereview.chromium.org/2094583002/diff/160001/components/memory_coor... components/memory_coordinator/browser/memory_coordinator.h:32: std::map<int, std::unique_ptr<MemoryCoordinatorHandleImpl>> children_; And please add a comment here =) https://codereview.chromium.org/2094583002/diff/160001/components/memory_coor... File components/memory_coordinator/public/interfaces/child_memory_coordinator.mojom (right): https://codereview.chromium.org/2094583002/diff/160001/components/memory_coor... components/memory_coordinator/public/interfaces/child_memory_coordinator.mojom:24: GetCurrentStateForTesting() => (MemoryState state); I think we shouldn't expose testing interfaces in the mojom if there are other ways. This would be akin to adding an IPC just for testing purposes: it's another IPC to audit for security and isn't something we want to encourage. It looks like all the tests that use this can just use the impl directly instead: I think doing that would make the tests simpler as well (since we don't need to wait for all the callbacks to complete).
Thank you for review! https://codereview.chromium.org/2094583002/diff/160001/components/memory_coor... File components/memory_coordinator/browser/memory_coordinator.h (right): https://codereview.chromium.org/2094583002/diff/160001/components/memory_coor... components/memory_coordinator/browser/memory_coordinator.h:23: void CreateHandle(int id, mojom::MemoryCoordinatorHandleRequest request); On 2016/07/12 05:20:28, dcheng wrote: > Nit: please call this rph_id or something so it's clearer what kind of ID it is. Done. https://codereview.chromium.org/2094583002/diff/160001/components/memory_coor... components/memory_coordinator/browser/memory_coordinator.h:27: void IterateChildrenForTesting(IterateCallback callback); On 2016/07/12 05:20:28, dcheng wrote: > Nit: pass callbacks by const ref. But rather than having this, I think we should > just friend a test helper that can access the state: it will make the tests very > straightforward, since all the test code will be inline. Hmm, can we make the test helper a friend of this class? The test lives in content/ but this lives in components/memory_coordinator. Dependency is content/ -> components/memory_coordinator. https://codereview.chromium.org/2094583002/diff/160001/components/memory_coor... components/memory_coordinator/browser/memory_coordinator.h:32: std::map<int, std::unique_ptr<MemoryCoordinatorHandleImpl>> children_; On 2016/07/12 05:20:28, dcheng wrote: > And please add a comment here =) Done. https://codereview.chromium.org/2094583002/diff/160001/components/memory_coor... File components/memory_coordinator/public/interfaces/child_memory_coordinator.mojom (right): https://codereview.chromium.org/2094583002/diff/160001/components/memory_coor... components/memory_coordinator/public/interfaces/child_memory_coordinator.mojom:24: GetCurrentStateForTesting() => (MemoryState state); On 2016/07/12 05:20:28, dcheng wrote: > I think we shouldn't expose testing interfaces in the mojom if there are other > ways. This would be akin to adding an IPC just for testing purposes: it's > another IPC to audit for security and isn't something we want to encourage. > > It looks like all the tests that use this can just use the impl directly > instead: I think doing that would make the tests simpler as well (since we don't > need to wait for all the callbacks to complete). Maybe I'm wrong but I think we need an IPC to get information from impl class because an instance of the impl class is in renderer and the test added in this CL is a browsertest. Is there a way to access impl instances which live in a renderer from a browsertest?
https://codereview.chromium.org/2094583002/diff/160001/components/memory_coor... File components/memory_coordinator/public/interfaces/child_memory_coordinator.mojom (right): https://codereview.chromium.org/2094583002/diff/160001/components/memory_coor... components/memory_coordinator/public/interfaces/child_memory_coordinator.mojom:24: GetCurrentStateForTesting() => (MemoryState state); On 2016/07/12 05:51:46, bashi1 wrote: > On 2016/07/12 05:20:28, dcheng wrote: > > I think we shouldn't expose testing interfaces in the mojom if there are other > > ways. This would be akin to adding an IPC just for testing purposes: it's > > another IPC to audit for security and isn't something we want to encourage. > > > > It looks like all the tests that use this can just use the impl directly > > instead: I think doing that would make the tests simpler as well (since we > don't > > need to wait for all the callbacks to complete). > > Maybe I'm wrong but I think we need an IPC to get information from impl class > because an instance of the impl class is in renderer and the test added in this > CL is a browsertest. Is there a way to access impl instances which live in a > renderer from a browsertest? Ah, I missed that it's a browser test. I think we still don't want to expose test only IPCs if we can avoid it... as far as I know of, we only have 3 in legacy IPC and it'd be good not to add another. Hmm...
https://codereview.chromium.org/2094583002/diff/160001/components/memory_coor... File components/memory_coordinator/public/interfaces/child_memory_coordinator.mojom (right): https://codereview.chromium.org/2094583002/diff/160001/components/memory_coor... components/memory_coordinator/public/interfaces/child_memory_coordinator.mojom:24: GetCurrentStateForTesting() => (MemoryState state); On 2016/07/12 06:10:53, dcheng wrote: > On 2016/07/12 05:51:46, bashi1 wrote: > > On 2016/07/12 05:20:28, dcheng wrote: > > > I think we shouldn't expose testing interfaces in the mojom if there are > other > > > ways. This would be akin to adding an IPC just for testing purposes: it's > > > another IPC to audit for security and isn't something we want to encourage. > > > > > > It looks like all the tests that use this can just use the impl directly > > > instead: I think doing that would make the tests simpler as well (since we > > don't > > > need to wait for all the callbacks to complete). > > > > Maybe I'm wrong but I think we need an IPC to get information from impl class > > because an instance of the impl class is in renderer and the test added in > this > > CL is a browsertest. Is there a way to access impl instances which live in a > > renderer from a browsertest? > > Ah, I missed that it's a browser test. I think we still don't want to expose > test only IPCs if we can avoid it... as far as I know of, we only have 3 in > legacy IPC and it'd be good not to add another. Hmm... I see. Let me think about a workaround. Probably we just check if there is a mapping in the browser to test renderers create their ChildMemoryCoordinator.
https://codereview.chromium.org/2094583002/diff/160001/components/memory_coor... File components/memory_coordinator/public/interfaces/child_memory_coordinator.mojom (right): https://codereview.chromium.org/2094583002/diff/160001/components/memory_coor... components/memory_coordinator/public/interfaces/child_memory_coordinator.mojom:24: GetCurrentStateForTesting() => (MemoryState state); On 2016/07/12 06:19:24, bashi1 wrote: > On 2016/07/12 06:10:53, dcheng wrote: > > On 2016/07/12 05:51:46, bashi1 wrote: > > > On 2016/07/12 05:20:28, dcheng wrote: > > > > I think we shouldn't expose testing interfaces in the mojom if there are > > other > > > > ways. This would be akin to adding an IPC just for testing purposes: it's > > > > another IPC to audit for security and isn't something we want to > encourage. > > > > > > > > It looks like all the tests that use this can just use the impl directly > > > > instead: I think doing that would make the tests simpler as well (since we > > > don't > > > > need to wait for all the callbacks to complete). > > > > > > Maybe I'm wrong but I think we need an IPC to get information from impl > class > > > because an instance of the impl class is in renderer and the test added in > > this > > > CL is a browsertest. Is there a way to access impl instances which live in a > > > renderer from a browsertest? > > > > Ah, I missed that it's a browser test. I think we still don't want to expose > > test only IPCs if we can avoid it... as far as I know of, we only have 3 in > > legacy IPC and it'd be good not to add another. Hmm... > > I see. Let me think about a workaround. Probably we just check if there is a > mapping in the browser to test renderers create their ChildMemoryCoordinator. Done. dcheng@, what do you think about PS#11?
LGTM. I'm not wholly satisfied in terms of clarify of lifetimes here, but that's probably a bigger question that needs to be tackled outside the scope of this CL. https://codereview.chromium.org/2094583002/diff/200001/content/browser/render... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/2094583002/diff/200001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:454: int id, Nit: render_process_id
> I'm not wholly satisfied in terms of clarify of lifetimes here, but that's > probably a bigger question that needs to be tackled outside the scope of this > CL. Added a TODO in RenderThreadImpl. chrisha@, jam@, haraken@: The latest PS has non-trivial changes since you reviewed. I'll land this CL tomorrow (or maybe day after tomorrow). Please respond if you have concerns. Thanks! https://codereview.chromium.org/2094583002/diff/200001/content/browser/render... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/2094583002/diff/200001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:454: int id, On 2016/07/12 07:44:46, dcheng wrote: > Nit: render_process_id Done.
The CQ bit was checked by bashi@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 bashi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, chrisha@chromium.org, jam@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2094583002/#ps240001 (title: "rebase")
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 ========== Add MemoryCoordinator This CL adds MemoryCoordinator mojo interface and its implementation. When --enable-features=MemoryCoordinator is specified the browser and renderers create their MemoryCoordinators. MemoryCoordinatorImpl (which lives in the browser process) creates a mojo message pipe when a renderer is launched. This CL doesn't add actual handling of memory events, which will be implemented in follow-up CLs. BUG=617492 ========== to ========== Add MemoryCoordinator This CL adds MemoryCoordinator mojo interface and its implementation. When --enable-features=MemoryCoordinator is specified the browser and renderers create their MemoryCoordinators. MemoryCoordinatorImpl (which lives in the browser process) creates a mojo message pipe when a renderer is launched. This CL doesn't add actual handling of memory events, which will be implemented in follow-up CLs. BUG=617492 ==========
Message was sent while issue was closed.
Committed patchset #13 (id:240001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== Add MemoryCoordinator This CL adds MemoryCoordinator mojo interface and its implementation. When --enable-features=MemoryCoordinator is specified the browser and renderers create their MemoryCoordinators. MemoryCoordinatorImpl (which lives in the browser process) creates a mojo message pipe when a renderer is launched. This CL doesn't add actual handling of memory events, which will be implemented in follow-up CLs. BUG=617492 ========== to ========== Add MemoryCoordinator This CL adds MemoryCoordinator mojo interface and its implementation. When --enable-features=MemoryCoordinator is specified the browser and renderers create their MemoryCoordinators. MemoryCoordinatorImpl (which lives in the browser process) creates a mojo message pipe when a renderer is launched. This CL doesn't add actual handling of memory events, which will be implemented in follow-up CLs. BUG=617492 Committed: https://crrev.com/066dcbbad57a70ba5380a4a8cdd0572d89bdc279 Cr-Commit-Position: refs/heads/master@{#405402} ==========
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/066dcbbad57a70ba5380a4a8cdd0572d89bdc279 Cr-Commit-Position: refs/heads/master@{#405402}
Message was sent while issue was closed.
A revert of this CL (patchset #13 id:240001) has been created in https://codereview.chromium.org/2143353004/ by bashi@chromium.org. The reason for reverting is: Broke mac build. |
