Make RendererFrameManager a client of memory coordinator

Make RendererFrameManager a client of memory coordinator

When the memory coordinator is enabled, RendererFrameManager becomes
a client of the memory coordinator instead of installing
MemoryPressureListener.

Design Doc: https://docs.google.com/document/d/1a69mMr7jI7qK0OfKNlrZ350xiXizVMCCe8orGX7K8Uo/edit?ts=57d7968b#

BUG=639700
Committed: https://crrev.com/5ebca45f1d3a2a62bafb887229a29ae0ec5a83bd
Cr-Commit-Position: refs/heads/master@{#427627}

[hajimehoshi, 2016-09-20 07:00:30]

Patchset #1 (id:1) has been deleted

[hajimehoshi, 2016-09-20 07:00:37]

Patchset #1 (id:20001) has been deleted

[hajimehoshi, 2016-09-20 07:14:45]

hajimehoshi@chromium.org changed reviewers:
+ bashi@chromium.org, chrisha@chromium.org, sadrul@chromium.org

[hajimehoshi, 2016-09-20 07:14:46]

PTAL

[hajimehoshi, 2016-09-20 07:15:27]

The CQ bit was checked by hajimehoshi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-20 07:15:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-20 07:28:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-20 07:28:09]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)

[bashi, 2016-09-20 07:56:51]

https://codereview.chromium.org/2358433003/diff/40001/content/browser/rendere...
File content/browser/renderer_host/renderer_frame_manager.cc (right):

https://codereview.chromium.org/2358433003/diff/40001/content/browser/rendere...
content/browser/renderer_host/renderer_frame_manager.cc:99: int percentage =
100;
Remove?

[hajimehoshi, 2016-09-20 08:09:43]

https://codereview.chromium.org/2358433003/diff/40001/content/browser/rendere...
File content/browser/renderer_host/renderer_frame_manager.cc (right):

https://codereview.chromium.org/2358433003/diff/40001/content/browser/rendere...
content/browser/renderer_host/renderer_frame_manager.cc:99: int percentage =
100;
On 2016/09/20 07:56:51, bashi1 (slow til Sep 26) wrote:
> Remove?

Done.

[hajimehoshi, 2016-09-20 08:09:48]

The CQ bit was checked by hajimehoshi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-20 08:10:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-20 09:03:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-20 09:03:54]

Dry run: This issue passed the CQ dry run.

[hajimehoshi, 2016-09-26 11:37:39]

ping

[chrisha, 2016-09-26 14:26:22]

https://codereview.chromium.org/2358433003/diff/60001/content/browser/rendere...
File content/browser/renderer_host/renderer_frame_manager.cc (right):

https://codereview.chromium.org/2358433003/diff/60001/content/browser/rendere...
content/browser/renderer_host/renderer_frame_manager.cc:79: switch
(memory_state_) {
Presumably the thing that assigned the memory pressure (the
ChildMemoryCoordinatorImpl) knows the memory pressure that has been given to
this MCC. Worth adding an API to query for the current state, rather than having
each MCC cache it individually?

https://codereview.chromium.org/2358433003/diff/60001/content/browser/rendere...
content/browser/renderer_host/renderer_frame_manager.cc:180: // have such value,
let's change the argument accroding to the value.
according*

[sadrul, 2016-09-26 20:04:20]

I have a couple of nit-y suggestions, but I don't think I am a good owner for
this. Can you have one of the content owners to review/approve instead (I
suspect some of them were involved in signing off on the design doc)?

https://codereview.chromium.org/2358433003/diff/60001/content/browser/rendere...
File content/browser/renderer_host/renderer_frame_manager.cc (right):

https://codereview.chromium.org/2358433003/diff/60001/content/browser/rendere...
content/browser/renderer_host/renderer_frame_manager.cc:118:
base::MemoryCoordinatorClientRegistry::GetInstance()->Register(this);
nit: Can the MemoryCoordinatorClient parts be moved into a separate object as
well? which, like MemoryPressureListener, takes a callback to run in response to
OnMemoryStateChange()? That way, RendererFrameManager itself doesn't have to
become one, and instead delegates that work to one of
MemoryCoordinatorClient/MemoryPressureListener (depending on which feature is
enabled)

https://codereview.chromium.org/2358433003/diff/60001/content/browser/rendere...
File content/browser/renderer_host/renderer_frame_manager.h (right):

https://codereview.chromium.org/2358433003/diff/60001/content/browser/rendere...
content/browser/renderer_host/renderer_frame_manager.h:68: void PurgeMemory(int
percentage);
Non override before overrides.

[hajimehoshi, 2016-09-28 07:31:10]

Thank you!

https://codereview.chromium.org/2358433003/diff/60001/content/browser/rendere...
File content/browser/renderer_host/renderer_frame_manager.cc (right):

https://codereview.chromium.org/2358433003/diff/60001/content/browser/rendere...
content/browser/renderer_host/renderer_frame_manager.cc:79: switch
(memory_state_) {
On 2016/09/26 14:26:22, chrisha (slow) wrote:
> Presumably the thing that assigned the memory pressure (the
> ChildMemoryCoordinatorImpl) knows the memory pressure that has been given to
> this MCC. Worth adding an API to query for the current state, rather than
having
> each MCC cache it individually?

1. To which class should I add a new API? Sorry but I didn't understand.
2. Would this work even after we eliminate MemoryPressure(Listener)?

https://codereview.chromium.org/2358433003/diff/60001/content/browser/rendere...
content/browser/renderer_host/renderer_frame_manager.cc:118:
base::MemoryCoordinatorClientRegistry::GetInstance()->Register(this);
On 2016/09/26 20:04:20, sadrul wrote:
> nit: Can the MemoryCoordinatorClient parts be moved into a separate object as
> well? which, like MemoryPressureListener, takes a callback to run in response
to
> OnMemoryStateChange()? That way, RendererFrameManager itself doesn't have to
> become one, and instead delegates that work to one of
> MemoryCoordinatorClient/MemoryPressureListener (depending on which feature is
> enabled)

Hmm, as for interfaces vs callbacks, we are discussing.

https://codereview.chromium.org/2358433003/diff/60001/content/browser/rendere...
content/browser/renderer_host/renderer_frame_manager.cc:180: // have such value,
let's change the argument accroding to the value.
On 2016/09/26 14:26:22, chrisha (slow) wrote:
> according*

Done.

https://codereview.chromium.org/2358433003/diff/60001/content/browser/rendere...
File content/browser/renderer_host/renderer_frame_manager.h (right):

https://codereview.chromium.org/2358433003/diff/60001/content/browser/rendere...
content/browser/renderer_host/renderer_frame_manager.h:68: void PurgeMemory(int
percentage);
On 2016/09/26 20:04:20, sadrul wrote:
> Non override before overrides.

Done.

[hajimehoshi, 2016-09-28 07:40:32]

hajimehoshi@chromium.org changed reviewers:
+ haraken@chromium.org

[hajimehoshi, 2016-09-28 07:40:32]

+haraken

[haraken, 2016-09-28 08:56:18]

Currently we only have only one API, so using callback function would make more
sense than using interface.

However, the plan is to add a couple of more APIs to MemoryCoordinatorClient.
For example, we're planning to add APIs to let the client report the size of
current memory, the estimated size of memory the client can purge, the estimated
size of memory the client can the throttle etc (see the design doc here:
https://docs.google.com/document/d/1dkUXXmpJk7xBUeQM-olBpTHJ2MXamDgY_kjNrl9JX...).
In that world, I think using interface would make more sense.

Does it makes sense?

[hajimehoshi, 2016-09-29 07:29:05]

The CQ bit was checked by hajimehoshi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-29 07:29:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-29 08:24:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-29 08:24:36]

Dry run: This issue passed the CQ dry run.

[hajimehoshi, 2016-10-11 07:12:55]

On 2016/09/28 08:56:18, haraken wrote:
> Currently we only have only one API, so using callback function would make
more
> sense than using interface.
> 
> However, the plan is to add a couple of more APIs to MemoryCoordinatorClient.
> For example, we're planning to add APIs to let the client report the size of
> current memory, the estimated size of memory the client can purge, the
estimated
> size of memory the client can the throttle etc (see the design doc here:
>
https://docs.google.com/document/d/1dkUXXmpJk7xBUeQM-olBpTHJ2MXamDgY_kjNrl9JX...).
> In that world, I think using interface would make more sense.
> 
> Does it makes sense?

ping sadrul

[sadrul, 2016-10-11 16:28:08]

On 2016/10/11 07:12:55, hajimehoshi wrote:
> On 2016/09/28 08:56:18, haraken wrote:
> > Currently we only have only one API, so using callback function would make
> more
> > sense than using interface.
> > 
> > However, the plan is to add a couple of more APIs to
MemoryCoordinatorClient.
> > For example, we're planning to add APIs to let the client report the size of
> > current memory, the estimated size of memory the client can purge, the
> estimated
> > size of memory the client can the throttle etc (see the design doc here:
> >
>
https://docs.google.com/document/d/1dkUXXmpJk7xBUeQM-olBpTHJ2MXamDgY_kjNrl9JX...).
> > In that world, I think using interface would make more sense.
> > 
> > Does it makes sense?
> 
> ping sadrul

My earlier comment was about RendererFrameManager owning an instance of a
base::MemoryCoordinatorClient (like it owns an instance of a
base::MemoryPressureListener), instead of being a MemoryCoordinatorClient
itself. Whether that MemoryCoordinatorClient uses callbacks or delegate
interfaces to talk back to RendererFrameManager is less of a concern (but it
sounds like there will be more communication between the two, so a delegate
interface sounds like the right choice).

Having said that, like I mentioned before, I am not a good owner reviewer for
this. I recommend someone from content owners who has already reviewed/approved
the MemoryCoordinatorClient design.

[haraken, 2016-10-12 00:18:49]

On 2016/10/11 16:28:08, sadrul wrote:
> On 2016/10/11 07:12:55, hajimehoshi wrote:
> > On 2016/09/28 08:56:18, haraken wrote:
> > > Currently we only have only one API, so using callback function would make
> > more
> > > sense than using interface.
> > > 
> > > However, the plan is to add a couple of more APIs to
> MemoryCoordinatorClient.
> > > For example, we're planning to add APIs to let the client report the size
of
> > > current memory, the estimated size of memory the client can purge, the
> > estimated
> > > size of memory the client can the throttle etc (see the design doc here:
> > >
> >
>
https://docs.google.com/document/d/1dkUXXmpJk7xBUeQM-olBpTHJ2MXamDgY_kjNrl9JX...).
> > > In that world, I think using interface would make more sense.
> > > 
> > > Does it makes sense?
> > 
> > ping sadrul
> 
> My earlier comment was about RendererFrameManager owning an instance of a
> base::MemoryCoordinatorClient (like it owns an instance of a
> base::MemoryPressureListener), instead of being a MemoryCoordinatorClient
> itself. Whether that MemoryCoordinatorClient uses callbacks or delegate
> interfaces to talk back to RendererFrameManager is less of a concern (but it
> sounds like there will be more communication between the two, so a delegate
> interface sounds like the right choice).

So this means that you're happy with the shape of PS5, right?

> 
> Having said that, like I mentioned before, I am not a good owner reviewer for
> this. I recommend someone from content owners who has already
reviewed/approved
> the MemoryCoordinatorClient design.

In any case, as I commented in https://codereview.chromium.org/2370753002/,
let's remove memory_state_ before landing this CL.

[sadrul, 2016-10-12 00:21:34]

On 2016/10/12 00:18:49, haraken wrote:
> On 2016/10/11 16:28:08, sadrul wrote:
> > On 2016/10/11 07:12:55, hajimehoshi wrote:
> > > On 2016/09/28 08:56:18, haraken wrote:
> > > > Currently we only have only one API, so using callback function would
make
> > > more
> > > > sense than using interface.
> > > > 
> > > > However, the plan is to add a couple of more APIs to
> > MemoryCoordinatorClient.
> > > > For example, we're planning to add APIs to let the client report the
size
> of
> > > > current memory, the estimated size of memory the client can purge, the
> > > estimated
> > > > size of memory the client can the throttle etc (see the design doc here:
> > > >
> > >
> >
>
https://docs.google.com/document/d/1dkUXXmpJk7xBUeQM-olBpTHJ2MXamDgY_kjNrl9JX...).
> > > > In that world, I think using interface would make more sense.
> > > > 
> > > > Does it makes sense?
> > > 
> > > ping sadrul
> > 
> > My earlier comment was about RendererFrameManager owning an instance of a
> > base::MemoryCoordinatorClient (like it owns an instance of a
> > base::MemoryPressureListener), instead of being a MemoryCoordinatorClient
> > itself. Whether that MemoryCoordinatorClient uses callbacks or delegate
> > interfaces to talk back to RendererFrameManager is less of a concern (but it
> > sounds like there will be more communication between the two, so a delegate
> > interface sounds like the right choice).
> 
> So this means that you're happy with the shape of PS5, right?

No? In PS5, RendererFrameManager is-a MemoryCoordinatorClient. I am suggesting
this be changed to RendererFrameManager has-a MemoryCoordinatorClient (like
RendererFrameManager currently has-a MemoryPressureListener).

> 
> > 
> > Having said that, like I mentioned before, I am not a good owner reviewer
for
> > this. I recommend someone from content owners who has already
> reviewed/approved
> > the MemoryCoordinatorClient design.
> 
> In any case, as I commented in https://codereview.chromium.org/2370753002/,
> let's remove memory_state_ before landing this CL.

[haraken, 2016-10-12 00:27:31]

On 2016/10/12 00:21:34, sadrul wrote:
> On 2016/10/12 00:18:49, haraken wrote:
> > On 2016/10/11 16:28:08, sadrul wrote:
> > > On 2016/10/11 07:12:55, hajimehoshi wrote:
> > > > On 2016/09/28 08:56:18, haraken wrote:
> > > > > Currently we only have only one API, so using callback function would
> make
> > > > more
> > > > > sense than using interface.
> > > > > 
> > > > > However, the plan is to add a couple of more APIs to
> > > MemoryCoordinatorClient.
> > > > > For example, we're planning to add APIs to let the client report the
> size
> > of
> > > > > current memory, the estimated size of memory the client can purge, the
> > > > estimated
> > > > > size of memory the client can the throttle etc (see the design doc
here:
> > > > >
> > > >
> > >
> >
>
https://docs.google.com/document/d/1dkUXXmpJk7xBUeQM-olBpTHJ2MXamDgY_kjNrl9JX...).
> > > > > In that world, I think using interface would make more sense.
> > > > > 
> > > > > Does it makes sense?
> > > > 
> > > > ping sadrul
> > > 
> > > My earlier comment was about RendererFrameManager owning an instance of a
> > > base::MemoryCoordinatorClient (like it owns an instance of a
> > > base::MemoryPressureListener), instead of being a MemoryCoordinatorClient
> > > itself. Whether that MemoryCoordinatorClient uses callbacks or delegate
> > > interfaces to talk back to RendererFrameManager is less of a concern (but
it
> > > sounds like there will be more communication between the two, so a
delegate
> > > interface sounds like the right choice).
> > 
> > So this means that you're happy with the shape of PS5, right?
> 
> No? In PS5, RendererFrameManager is-a MemoryCoordinatorClient. I am suggesting
> this be changed to RendererFrameManager has-a MemoryCoordinatorClient (like
> RendererFrameManager currently has-a MemoryPressureListener).

Ah, got it. What's your point of making RendererFrameManager has-a
MemoryCoordinatorClient?

We have used the is-a pattern in other clients. It seems a bit strange too me
that a client owns MemoryCoordinatorClient instead of being
MemoryCoordinatorClient but I may still be missing your point :)

> 
> > 
> > > 
> > > Having said that, like I mentioned before, I am not a good owner reviewer
> for
> > > this. I recommend someone from content owners who has already
> > reviewed/approved
> > > the MemoryCoordinatorClient design.
> > 
> > In any case, as I commented in https://codereview.chromium.org/2370753002/,
> > let's remove memory_state_ before landing this CL.

[sadrul, 2016-10-12 00:33:28]

On 2016/10/12 00:27:31, haraken wrote:
> On 2016/10/12 00:21:34, sadrul wrote:
> > On 2016/10/12 00:18:49, haraken wrote:
> > > On 2016/10/11 16:28:08, sadrul wrote:
> > > > On 2016/10/11 07:12:55, hajimehoshi wrote:
> > > > > On 2016/09/28 08:56:18, haraken wrote:
> > > > > > Currently we only have only one API, so using callback function
would
> > make
> > > > > more
> > > > > > sense than using interface.
> > > > > > 
> > > > > > However, the plan is to add a couple of more APIs to
> > > > MemoryCoordinatorClient.
> > > > > > For example, we're planning to add APIs to let the client report the
> > size
> > > of
> > > > > > current memory, the estimated size of memory the client can purge,
the
> > > > > estimated
> > > > > > size of memory the client can the throttle etc (see the design doc
> here:
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://docs.google.com/document/d/1dkUXXmpJk7xBUeQM-olBpTHJ2MXamDgY_kjNrl9JX...).
> > > > > > In that world, I think using interface would make more sense.
> > > > > > 
> > > > > > Does it makes sense?
> > > > > 
> > > > > ping sadrul
> > > > 
> > > > My earlier comment was about RendererFrameManager owning an instance of
a
> > > > base::MemoryCoordinatorClient (like it owns an instance of a
> > > > base::MemoryPressureListener), instead of being a
MemoryCoordinatorClient
> > > > itself. Whether that MemoryCoordinatorClient uses callbacks or delegate
> > > > interfaces to talk back to RendererFrameManager is less of a concern
(but
> it
> > > > sounds like there will be more communication between the two, so a
> delegate
> > > > interface sounds like the right choice).
> > > 
> > > So this means that you're happy with the shape of PS5, right?
> > 
> > No? In PS5, RendererFrameManager is-a MemoryCoordinatorClient. I am
suggesting
> > this be changed to RendererFrameManager has-a MemoryCoordinatorClient (like
> > RendererFrameManager currently has-a MemoryPressureListener).
> 
> Ah, got it. What's your point of making RendererFrameManager has-a
> MemoryCoordinatorClient?
> 
> We have used the is-a pattern in other clients. It seems a bit strange too me
> that a client owns MemoryCoordinatorClient instead of being
> MemoryCoordinatorClient but I may still be missing your point :)

Yep. We do use is-a pattern in a lot of places. We also use has-a pattern in a
lot of places :) When possible, I think has-a usually leads to cleaner code
('composition over inheritance'). In this specific case, since the existing
mechanism uses has-a, I was suggesting the new mechanism does the same.

But really, I understand that this is a subjective matter, and as I have
mentioned, I am not a good owner-review for this change. So I wouldn't block
this CL on this.

[hajimehoshi, 2016-10-25 08:38:55]

hajimehoshi@chromium.org changed reviewers:
+ avi@chromium.org

[hajimehoshi, 2016-10-25 08:38:55]

+avi PTAL

[haraken, 2016-10-25 09:33:36]

LGTM

[Avi (use Gerrit), 2016-10-25 15:14:36]

LGTM

Looking forward to the switchover when we can drop the duplicate code.

[hajimehoshi, 2016-10-26 06:38:58]

The CQ bit was checked by hajimehoshi@chromium.org

[commit-bot: I haz the power, 2016-10-26 06:39:16]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-26 07:30:45]

Committed patchset #6 (id:140001)

[commit-bot: I haz the power, 2016-10-26 07:32:47]

Description was changed from

==========
Make RendererFrameManager a client of memory coordinator

When the memory coordinator is enabled, RendererFrameManager becomes
a client of the memory coordinator instead of installing
MemoryPressureListener.

Design Doc:
https://docs.google.com/document/d/1a69mMr7jI7qK0OfKNlrZ350xiXizVMCCe8orGX7K8...

BUG=639700
==========

to

==========
Make RendererFrameManager a client of memory coordinator

When the memory coordinator is enabled, RendererFrameManager becomes
a client of the memory coordinator instead of installing
MemoryPressureListener.

Design Doc:
https://docs.google.com/document/d/1a69mMr7jI7qK0OfKNlrZ350xiXizVMCCe8orGX7K8...

BUG=639700

Committed: https://crrev.com/5ebca45f1d3a2a62bafb887229a29ae0ec5a83bd
Cr-Commit-Position: refs/heads/master@{#427627}
==========

[commit-bot: I haz the power, 2016-10-26 07:32:48]

Patchset 6 (id:??) landed as
https://crrev.com/5ebca45f1d3a2a62bafb887229a29ae0ec5a83bd
Cr-Commit-Position: refs/heads/master@{#427627}