Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1303203002:
Add MemoryPurgeController (Closed)
|
Created:
5 years, 4 months ago by bashi Modified:
5 years, 4 months ago CC:
blink-reviews, blink-reviews-rendering, eae+blinkwatch, jchaffraix+rendering, leviw+renderwatch, pdr+renderingwatchlist_chromium.org, szager+layoutwatch_chromium.org, zoltan1 Base URL:
svn://svn.chromium.org/blink/trunk Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionAdd MemoryPurgeController
No client so far. Follow-up CLs will add actual clients.
Design doc:
https://docs.google.com/document/d/1TbtkhXpjw_8lftLwELuEPEgcyWfXuVLuqW2KYJZeaBA/edit?pli=1
Non trivial changes from the design doc:
- MemoryPurgeController owns its clients.
- Page owns MemoryPurgeController (because core/ and modules/ will want
to access MemoryPurgeController to register clients)
I chose Page as the owner of the controller because it has the same
lifetime of WebView.
BUG=520496
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=201116
Patch Set 1 #
Total comments: 4
Patch Set 2 : Ready for review #Patch Set 3 : export #
Total comments: 4
Patch Set 4 : #
Total comments: 13
Patch Set 5 : #Patch Set 6 : #
Messages
Total messages: 28 (4 generated)
bashi@chromium.org changed reviewers: + hajimehoshi@chromium.org, haraken@chromium.org
I'm trying to implement MemoryPurgeController. Does this make sense?
haraken@chromium.org changed reviewers: + dcheng@chromium.org, tkent@chromium.org
+tkent-san and dcheng for design review. > - Page owns MemoryPurgeController (because core/ and modules/ will want > to access MemoryPurgeController to register clients) Sounds good to me but I want to have tkent-san confirm this. > - MemoryPurgeController owns its clients. When I was writing the design doc, I thought that a LocalFrame would be a better owner of the clients because of out-of-process iframes. However, I agree that we can worry about the issue later. Dcheng? Nit: I'd rename MemoryPurgeListener to MemoryPurgeClient. It seems that Blink prefers "Clients".
On 2015/08/21 at 12:29:24, haraken wrote: > +tkent-san and dcheng for design review. > > > - Page owns MemoryPurgeController (because core/ and modules/ will want > > to access MemoryPurgeController to register clients) > > Sounds good to me but I want to have tkent-san confirm this. > > > - MemoryPurgeController owns its clients. > > When I was writing the design doc, I thought that a LocalFrame would be a better owner of the clients because of out-of-process iframes. However, I agree that we can worry about the issue later. Dcheng? > > Nit: I'd rename MemoryPurgeListener to MemoryPurgeClient. It seems that Blink prefers "Clients". I think this is fine: it probably makes more sense to discard memory on a per-tab level rather than per-frame anyway. I don't see any actual ownership in the current patch though: it appears to just be raw pointers in the listener hash set?
https://codereview.chromium.org/1303203002/diff/1/Source/platform/MemoryPurge... File Source/platform/MemoryPurgeController.h (right): https://codereview.chromium.org/1303203002/diff/1/Source/platform/MemoryPurge... Source/platform/MemoryPurgeController.h:14: Inactive, Suggestion: we should prefer enum class for both of these (Inactive is a pretty generic name) https://codereview.chromium.org/1303203002/diff/1/Source/platform/MemoryPurge... Source/platform/MemoryPurgeController.h:18: enum DeviceKind { Ditto: NotSpecified is pretty generic.
On 2015/08/21 15:04:14, dcheng wrote: > On 2015/08/21 at 12:29:24, haraken wrote: > > +tkent-san and dcheng for design review. > > > > > - Page owns MemoryPurgeController (because core/ and modules/ will want > > > to access MemoryPurgeController to register clients) > > > > Sounds good to me but I want to have tkent-san confirm this. > > > > > - MemoryPurgeController owns its clients. > > > > When I was writing the design doc, I thought that a LocalFrame would be a > better owner of the clients because of out-of-process iframes. However, I agree > that we can worry about the issue later. Dcheng? > > > > Nit: I'd rename MemoryPurgeListener to MemoryPurgeClient. It seems that Blink > prefers "Clients". > > I think this is fine: it probably makes more sense to discard memory on a > per-tab level rather than per-frame anyway. > > I don't see any actual ownership in the current patch though: it appears to just > be raw pointers in the listener hash set? Ah, right; there is no ownership. The hash set is going to be a HeapHashSet of WeakMembers. So our question was just who should hold the hash set.
On 2015/08/21 at 15:13:34, haraken wrote: > On 2015/08/21 15:04:14, dcheng wrote: > > On 2015/08/21 at 12:29:24, haraken wrote: > > > +tkent-san and dcheng for design review. > > > > > > > - Page owns MemoryPurgeController (because core/ and modules/ will want > > > > to access MemoryPurgeController to register clients) > > > > > > Sounds good to me but I want to have tkent-san confirm this. > > > > > > > - MemoryPurgeController owns its clients. > > > > > > When I was writing the design doc, I thought that a LocalFrame would be a > > better owner of the clients because of out-of-process iframes. However, I agree > > that we can worry about the issue later. Dcheng? > > > > > > Nit: I'd rename MemoryPurgeListener to MemoryPurgeClient. It seems that Blink > > prefers "Clients". > > > > I think this is fine: it probably makes more sense to discard memory on a > > per-tab level rather than per-frame anyway. > > > > I don't see any actual ownership in the current patch though: it appears to just > > be raw pointers in the listener hash set? > > Ah, right; there is no ownership. The hash set is going to be a HeapHashSet of WeakMembers. So our question was just who should hold the hash set. Yep, that seems fine with me.
On 2015/08/21 12:29:24, haraken wrote: > +tkent-san and dcheng for design review. > > > - Page owns MemoryPurgeController (because core/ and modules/ will want > > to access MemoryPurgeController to register clients) > > Sounds good to me but I want to have tkent-san confirm this. I's ok for me with it if dcheng accepts it :)
Thanks all for feedbacks! PS#3 is ready for review. Could you take another look? https://codereview.chromium.org/1303203002/diff/1/Source/platform/MemoryPurge... File Source/platform/MemoryPurgeController.h (right): https://codereview.chromium.org/1303203002/diff/1/Source/platform/MemoryPurge... Source/platform/MemoryPurgeController.h:14: Inactive, On 2015/08/21 15:04:19, dcheng wrote: > Suggestion: we should prefer enum class for both of these (Inactive is a pretty > generic name) Done. https://codereview.chromium.org/1303203002/diff/1/Source/platform/MemoryPurge... Source/platform/MemoryPurgeController.h:18: enum DeviceKind { On 2015/08/21 15:04:19, dcheng wrote: > Ditto: NotSpecified is pretty generic. Done.
https://codereview.chromium.org/1303203002/diff/40001/Source/platform/MemoryP... File Source/platform/MemoryPurgeController.h (right): https://codereview.chromium.org/1303203002/diff/40001/Source/platform/MemoryP... Source/platform/MemoryPurgeController.h:45: void registerClient(MemoryPurgeClient* client) { m_clients.add(client); } Do you allow null client? https://codereview.chromium.org/1303203002/diff/40001/Source/platform/MemoryP... Source/platform/MemoryPurgeController.h:46: void unregisterClient(MemoryPurgeClient* client) { m_clients.remove(client); } Do you allow to call unregisterClient with not-registered client?
https://codereview.chromium.org/1303203002/diff/40001/Source/platform/MemoryP... File Source/platform/MemoryPurgeController.h (right): https://codereview.chromium.org/1303203002/diff/40001/Source/platform/MemoryP... Source/platform/MemoryPurgeController.h:45: void registerClient(MemoryPurgeClient* client) { m_clients.add(client); } On 2015/08/25 03:09:31, tkent wrote: > Do you allow null client? Added ASSERT(). https://codereview.chromium.org/1303203002/diff/40001/Source/platform/MemoryP... Source/platform/MemoryPurgeController.h:46: void unregisterClient(MemoryPurgeClient* client) { m_clients.remove(client); } On 2015/08/25 03:09:31, tkent wrote: > Do you allow to call unregisterClient with not-registered client? > Added ASSERT().
https://codereview.chromium.org/1303203002/diff/60001/Source/platform/MemoryP... File Source/platform/MemoryPurgeController.h (right): https://codereview.chromium.org/1303203002/diff/60001/Source/platform/MemoryP... Source/platform/MemoryPurgeController.h:45: void registerClient(MemoryPurgeClient* client) Is it ok to call registerClient/unregisterClient from non-main threads? https://codereview.chromium.org/1303203002/diff/60001/Source/platform/MemoryP... Source/platform/MemoryPurgeController.h:51: void unregisterClient(MemoryPurgeClient* client) How to call unregisterClient() for an object which is already GarbageCollected?
LGTM on my side. https://codereview.chromium.org/1303203002/diff/60001/Source/platform/MemoryP... File Source/platform/MemoryPurgeController.cpp (right): https://codereview.chromium.org/1303203002/diff/60001/Source/platform/MemoryP... Source/platform/MemoryPurgeController.cpp:24: purgeMemory(MemoryPurgeMode::InactiveTab); This method will be actually dispatched. I'd remove pageBecameVisible() and pageBecameHidden() from this CL. We need more investigation to figure out what the MemoryPurgeController should do. https://codereview.chromium.org/1303203002/diff/60001/Source/platform/MemoryP... File Source/platform/MemoryPurgeController.h (right): https://codereview.chromium.org/1303203002/diff/60001/Source/platform/MemoryP... Source/platform/MemoryPurgeController.h:39: // Since we want to controll memory per tab, MemoryPurgeController is owned by control https://codereview.chromium.org/1303203002/diff/60001/Source/platform/MemoryP... Source/platform/MemoryPurgeController.h:47: ASSERT(client); Add ASSERT(!m_clients.contains(client)).
https://codereview.chromium.org/1303203002/diff/60001/Source/platform/MemoryP... File Source/platform/MemoryPurgeController.h (right): https://codereview.chromium.org/1303203002/diff/60001/Source/platform/MemoryP... Source/platform/MemoryPurgeController.h:51: void unregisterClient(MemoryPurgeClient* client) On 2015/08/25 03:45:59, tkent wrote: > How to call unregisterClient() for an object which is already GarbageCollected? This will be used only when we want to explicitly unregister the client from the controller.
https://codereview.chromium.org/1303203002/diff/60001/Source/platform/MemoryP... File Source/platform/MemoryPurgeController.h (right): https://codereview.chromium.org/1303203002/diff/60001/Source/platform/MemoryP... Source/platform/MemoryPurgeController.h:51: void unregisterClient(MemoryPurgeClient* client) On 2015/08/25 03:47:03, haraken wrote: > On 2015/08/25 03:45:59, tkent wrote: > > How to call unregisterClient() for an object which is already > GarbageCollected? > > This will be used only when we want to explicitly unregister the client from the > controller. > m_clients consists of raw pointers now (!ENABLE(OILPAN)). So, if a GarbageCollected object implements MemoryPurgeClient, it needs to unregister itself when it's destructed.
https://codereview.chromium.org/1303203002/diff/60001/Source/platform/MemoryP... File Source/platform/MemoryPurgeController.h (right): https://codereview.chromium.org/1303203002/diff/60001/Source/platform/MemoryP... Source/platform/MemoryPurgeController.h:51: void unregisterClient(MemoryPurgeClient* client) On 2015/08/25 03:50:19, tkent wrote: > On 2015/08/25 03:47:03, haraken wrote: > > On 2015/08/25 03:45:59, tkent wrote: > > > How to call unregisterClient() for an object which is already > > GarbageCollected? > > > > This will be used only when we want to explicitly unregister the client from > the > > controller. > > > > m_clients consists of raw pointers now (!ENABLE(OILPAN)). So, if a > GarbageCollected object implements MemoryPurgeClient, it needs to unregister > itself when it's destructed. Yes, I'm assuming the following use case. class X : public RefCountedWillBeGarbageCollected<X>, public MemoryPurgeClient { ~X() { #if !ENABLE(OILPAN) unregisterClient(this); #endif } void someDisposal() { unregisterClient(this); } };
https://codereview.chromium.org/1303203002/diff/60001/Source/platform/MemoryP... File Source/platform/MemoryPurgeController.cpp (right): https://codereview.chromium.org/1303203002/diff/60001/Source/platform/MemoryP... Source/platform/MemoryPurgeController.cpp:24: purgeMemory(MemoryPurgeMode::InactiveTab); On 2015/08/25 03:46:08, haraken wrote: > > This method will be actually dispatched. I'd remove pageBecameVisible() and > pageBecameHidden() from this CL. We need more investigation to figure out what > the MemoryPurgeController should do. Removed. https://codereview.chromium.org/1303203002/diff/60001/Source/platform/MemoryP... File Source/platform/MemoryPurgeController.h (right): https://codereview.chromium.org/1303203002/diff/60001/Source/platform/MemoryP... Source/platform/MemoryPurgeController.h:39: // Since we want to controll memory per tab, MemoryPurgeController is owned by On 2015/08/25 03:46:08, haraken wrote: > > control Done. https://codereview.chromium.org/1303203002/diff/60001/Source/platform/MemoryP... Source/platform/MemoryPurgeController.h:45: void registerClient(MemoryPurgeClient* client) On 2015/08/25 03:45:59, tkent wrote: > Is it ok to call registerClient/unregisterClient from non-main threads? Added ASSERT(isMainThead()) https://codereview.chromium.org/1303203002/diff/60001/Source/platform/MemoryP... Source/platform/MemoryPurgeController.h:47: ASSERT(client); On 2015/08/25 03:46:08, haraken wrote: > > Add ASSERT(!m_clients.contains(client)). Done.
https://codereview.chromium.org/1303203002/diff/60001/Source/platform/MemoryP... File Source/platform/MemoryPurgeController.h (right): https://codereview.chromium.org/1303203002/diff/60001/Source/platform/MemoryP... Source/platform/MemoryPurgeController.h:51: void unregisterClient(MemoryPurgeClient* client) On 2015/08/25 03:52:35, haraken wrote: > On 2015/08/25 03:50:19, tkent wrote: > > On 2015/08/25 03:47:03, haraken wrote: > > > On 2015/08/25 03:45:59, tkent wrote: > > > > How to call unregisterClient() for an object which is already > > > GarbageCollected? > > > > > > This will be used only when we want to explicitly unregister the client from > > the > > > controller. > > > > > > > m_clients consists of raw pointers now (!ENABLE(OILPAN)). So, if a > > GarbageCollected object implements MemoryPurgeClient, it needs to unregister > > itself when it's destructed. > > Yes, I'm assuming the following use case. > > class X : public RefCountedWillBeGarbageCollected<X>, public MemoryPurgeClient { > ~X() { > #if !ENABLE(OILPAN) > unregisterClient(this); > #endif > } > > void someDisposal() { > unregisterClient(this); > } > }; I'm asking about GarbageCollected(Finalized) cases, not WillBe.
On 2015/08/25 04:58:08, tkent wrote: > https://codereview.chromium.org/1303203002/diff/60001/Source/platform/MemoryP... > File Source/platform/MemoryPurgeController.h (right): > > https://codereview.chromium.org/1303203002/diff/60001/Source/platform/MemoryP... > Source/platform/MemoryPurgeController.h:51: void > unregisterClient(MemoryPurgeClient* client) > On 2015/08/25 03:52:35, haraken wrote: > > On 2015/08/25 03:50:19, tkent wrote: > > > On 2015/08/25 03:47:03, haraken wrote: > > > > On 2015/08/25 03:45:59, tkent wrote: > > > > > How to call unregisterClient() for an object which is already > > > > GarbageCollected? > > > > > > > > This will be used only when we want to explicitly unregister the client > from > > > the > > > > controller. > > > > > > > > > > m_clients consists of raw pointers now (!ENABLE(OILPAN)). So, if a > > > GarbageCollected object implements MemoryPurgeClient, it needs to unregister > > > itself when it's destructed. > > > > Yes, I'm assuming the following use case. > > > > class X : public RefCountedWillBeGarbageCollected<X>, public MemoryPurgeClient > { > > ~X() { > > #if !ENABLE(OILPAN) > > unregisterClient(this); > > #endif > > } > > > > void someDisposal() { > > unregisterClient(this); > > } > > }; > > I'm asking about GarbageCollected(Finalized) cases, not WillBe. We're assuming that classes that need to inherit from MemoryPurgeClient (e.g., Document, Resource, Frame etc) will have WillBe, and we drop the WillBes at the same time.
On 2015/08/25 05:05:12, haraken wrote: > We're assuming that classes that need to inherit from MemoryPurgeClient (e.g., > Document, Resource, Frame etc) will have WillBe, and we drop the WillBes at the > same time. ok, got it. MemoryPurgeController.h should mention this restriction.
On 2015/08/25 05:07:21, tkent wrote: > On 2015/08/25 05:05:12, haraken wrote: > > We're assuming that classes that need to inherit from MemoryPurgeClient (e.g., > > Document, Resource, Frame etc) will have WillBe, and we drop the WillBes at > the > > same time. > > ok, got it. MemoryPurgeController.h should mention this restriction. Done.
lgtm
Thanks!
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 Link to the patchset: https://codereview.chromium.org/1303203002/#ps100001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1303203002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1303203002/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://src.chromium.org/viewvc/blink?view=rev&revision=201116 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
