Add MemoryCoordinatorClient

Add MemoryCoordinatorClient

Also move MemoryCoordinator from core/dom to platform/. This is a preparation for experimenting purge and suspend [1,2].

[1] https://docs.google.com/document/d/1EgLimgxWK5DGhptnNVbEGSvVn6Q609ZJaBkLjEPRJvI/edit
[2] https://codereview.chromium.org/2130683002/

Committed: https://crrev.com/75ce9c3e2c15f2df215184026162dc505df37e4b
Cr-Commit-Position: refs/heads/master@{#412171}

[bashi, 2016-08-09 08:01:37]

bashi@chromium.org changed reviewers:
+ haraken@chromium.org, tasak@google.com

[bashi, 2016-08-09 08:01:38]

Sakamoto-san,

Does this fit your purpose? I intentionally separate onMemoryPressure() from
purgeMemory() so that you can implement purgeMemory() independently.

[bashi, 2016-08-11 22:52:44]

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

[commit-bot: I haz the power, 2016-08-11 22:54:04]

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

[bashi, 2016-08-11 22:58:02]

Description was changed from

==========
WIP: Add MemoryCoordinatorClient

Also move it from core/dom to platform
==========

to

==========
Add MemoryCoordinatorClient

Also move MemoryCoordinator from core/dom to platform/. This is a preparation
for experimenting purge and suspend [1,2].

[1]
https://docs.google.com/document/d/1EgLimgxWK5DGhptnNVbEGSvVn6Q609ZJaBkLjEPRJ...
[2] https://codereview.chromium.org/2130683002/
==========

[bashi, 2016-08-11 23:00:36]

bashi@chromium.org changed reviewers:
+ esprehn@chromium.org

[bashi, 2016-08-11 23:00:37]

haraken@, esprehn@,

Could you review this? tasak@ seems OK with this approach.

[commit-bot: I haz the power, 2016-08-11 23:28:03]

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

[commit-bot: I haz the power, 2016-08-11 23:28:03]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[haraken, 2016-08-12 02:55:01]

LGTM

https://codereview.chromium.org/2230433002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/MemoryCache.cpp (right):

https://codereview.chromium.org/2230433002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/MemoryCache.cpp:123:
MemoryCoordinator::instance().unregisterClient(this);

We don't need to call this since the client is automatically removed by Oilpan's
weak processing.

[tasak, 2016-08-15 08:07:58]

lgtm

[tasak, 2016-08-15 08:12:24]

https://codereview.chromium.org/2230433002/diff/20001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/WebMemoryCoordinator.h (right):

https://codereview.chromium.org/2230433002/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebMemoryCoordinator.h:21: BLINK_EXPORT
static void purgeMemory();
BLINK_PLATFORM_EXPORT?
Because:
e:\b\c\b\win\src\third_party\webkit\source\platform\exported\webmemorycoordinator.cpp(17):
warning C4273: 'blink::WebMemoryCoordinator::purgeMemory': inconsistent dll
linkage
e:\b\c\b\win\src\third_party\webkit\public\platform\webmemorycoordinator.h(21):
note: see previous definition of 'purgeMemory'

[esprehn, 2016-08-15 15:41:07]

lgtm

https://codereview.chromium.org/2230433002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/MemoryCache.cpp (right):

https://codereview.chromium.org/2230433002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/MemoryCache.cpp:743: if
(ProcessHeap::isLowEndDevice())
Hmm, should we do this in the constructor then? Calling this method and having
it be a no-op seems strange.

[bashi, 2016-08-16 00:09:07]

Thanks for reviews!

https://codereview.chromium.org/2230433002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/MemoryCache.cpp (right):

https://codereview.chromium.org/2230433002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/MemoryCache.cpp:123:
MemoryCoordinator::instance().unregisterClient(this);
On 2016/08/12 02:55:00, haraken wrote:
> 
> We don't need to call this since the client is automatically removed by
Oilpan's
> weak processing.

Removed.

Does it mean we should have null checks in MemoryCoordinator when it iterates
over |m_clients|?

https://codereview.chromium.org/2230433002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/MemoryCache.cpp:743: if
(ProcessHeap::isLowEndDevice())
On 2016/08/15 15:41:07, esprehn wrote:
> Hmm, should we do this in the constructor then? Calling this method and having
> it be a no-op seems strange.

Done.

tasak@: you might want to move back this condition when you implement
purgeMemory() interface in MemoryCache.

https://codereview.chromium.org/2230433002/diff/20001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/WebMemoryCoordinator.h (right):

https://codereview.chromium.org/2230433002/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebMemoryCoordinator.h:21: BLINK_EXPORT
static void purgeMemory();
On 2016/08/15 08:12:24, tasak wrote:
> BLINK_PLATFORM_EXPORT?
> Because:
>
e:\b\c\b\win\src\third_party\webkit\source\platform\exported\webmemorycoordinator.cpp(17):
> warning C4273: 'blink::WebMemoryCoordinator::purgeMemory': inconsistent dll
> linkage
>
e:\b\c\b\win\src\third_party\webkit\public\platform\webmemorycoordinator.h(21):
> note: see previous definition of 'purgeMemory'

Done.

[haraken, 2016-08-16 02:37:04]

https://codereview.chromium.org/2230433002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/MemoryCache.cpp (right):

https://codereview.chromium.org/2230433002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/MemoryCache.cpp:123:
MemoryCoordinator::instance().unregisterClient(this);
On 2016/08/16 00:09:06, bashi1 (slow til 16 Aug) wrote:
> On 2016/08/12 02:55:00, haraken wrote:
> > 
> > We don't need to call this since the client is automatically removed by
> Oilpan's
> > weak processing.
> 
> Removed.
> 
> Does it mean we should have null checks in MemoryCoordinator when it iterates
> over |m_clients|?

No. Cleared entries are automatically removed from the HashSet by Oilpan, so you
don't need to worry about it. (Also it's guaranteed that Oilpan doesn't collect
the objects while someone is iterating the HashSet on the stack.)

[bashi, 2016-08-16 02:39:42]

https://codereview.chromium.org/2230433002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/MemoryCache.cpp (right):

https://codereview.chromium.org/2230433002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/MemoryCache.cpp:123:
MemoryCoordinator::instance().unregisterClient(this);
On 2016/08/16 02:37:04, haraken wrote:
> On 2016/08/16 00:09:06, bashi1 (slow til 16 Aug) wrote:
> > On 2016/08/12 02:55:00, haraken wrote:
> > > 
> > > We don't need to call this since the client is automatically removed by
> > Oilpan's
> > > weak processing.
> > 
> > Removed.
> > 
> > Does it mean we should have null checks in MemoryCoordinator when it
iterates
> > over |m_clients|?
> 
> No. Cleared entries are automatically removed from the HashSet by Oilpan, so
you
> don't need to worry about it. (Also it's guaranteed that Oilpan doesn't
collect
> the objects while someone is iterating the HashSet on the stack.)
> 

I see. Thanks for clarification.

[bashi, 2016-08-16 02:39:48]

The CQ bit was checked by bashi@chromium.org

[bashi, 2016-08-16 02:39:48]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, esprehn@chromium.org, tasak@google.com
Link to the patchset: https://codereview.chromium.org/2230433002/#ps40001
(title: "comments")

[commit-bot: I haz the power, 2016-08-16 02:40:04]

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-08-16 05:42:57]

Description was changed from

==========
Add MemoryCoordinatorClient

Also move MemoryCoordinator from core/dom to platform/. This is a preparation
for experimenting purge and suspend [1,2].

[1]
https://docs.google.com/document/d/1EgLimgxWK5DGhptnNVbEGSvVn6Q609ZJaBkLjEPRJ...
[2] https://codereview.chromium.org/2130683002/
==========

to

==========
Add MemoryCoordinatorClient

Also move MemoryCoordinator from core/dom to platform/. This is a preparation
for experimenting purge and suspend [1,2].

[1]
https://docs.google.com/document/d/1EgLimgxWK5DGhptnNVbEGSvVn6Q609ZJaBkLjEPRJ...
[2] https://codereview.chromium.org/2130683002/
==========

[commit-bot: I haz the power, 2016-08-16 05:42:58]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-08-16 05:44:37]

Description was changed from

==========
Add MemoryCoordinatorClient

Also move MemoryCoordinator from core/dom to platform/. This is a preparation
for experimenting purge and suspend [1,2].

[1]
https://docs.google.com/document/d/1EgLimgxWK5DGhptnNVbEGSvVn6Q609ZJaBkLjEPRJ...
[2] https://codereview.chromium.org/2130683002/
==========

to

==========
Add MemoryCoordinatorClient

Also move MemoryCoordinator from core/dom to platform/. This is a preparation
for experimenting purge and suspend [1,2].

[1]
https://docs.google.com/document/d/1EgLimgxWK5DGhptnNVbEGSvVn6Q609ZJaBkLjEPRJ...
[2] https://codereview.chromium.org/2130683002/

Committed: https://crrev.com/75ce9c3e2c15f2df215184026162dc505df37e4b
Cr-Commit-Position: refs/heads/master@{#412171}
==========

[commit-bot: I haz the power, 2016-08-16 05:44:38]

Patchset 3 (id:??) landed as
https://crrev.com/75ce9c3e2c15f2df215184026162dc505df37e4b
Cr-Commit-Position: refs/heads/master@{#412171}

[boliu, 2016-08-16 17:26:27]

boliu@chromium.org changed reviewers:
+ boliu@chromium.org

[boliu, 2016-08-16 17:26:28]

is this safe to revert?

https://codereview.chromium.org/2230433002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/MemoryCoordinator.cpp (right):

https://codereview.chromium.org/2230433002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/MemoryCoordinator.cpp:58:
client->onMemoryPressure(level);
Getting a null pointer crash here.
https://uberchromegw.corp.google.com/i/internal.client.clank/builders/health-...

	signal 11 (SIGSEGV), code 1, fault addr 0x1ba in tid 9575 (Chrome_InProcRe)
	pid: 9539, tid: 9575, name: Chrome_InProcRe  >>> org.chromium.webview_shell <<<
	signal 11 (SIGSEGV), code 1 (SEGV_MAPERR), fault addr 0x1ba
	     r0 2e49378c  r1 00000002  r2 2e364b90  r3 00000192
	     r4 a0020728  r5 b761ac38  r6 00000002  r7 a0020718
	     r8 a61955bc  r9 a617bdaa  sl b75d2ae4  fp a5c260b6
	     ip ffffff00  sp a00206e8  lr a4ed2399  pc a4ed23c6
	
	Stack Trace:
	  RELADDR   FUNCTION                                                           
       FILE:LINE
	  011ab3c6 
blink::MemoryCoordinator::onMemoryPressure(blink::WebMemoryPressureLevel) 
/b/build/slave/arm-builder-rel/build/src/third_party/WebKit/Source/platform/MemoryCoordinator.cpp:58

[esprehn, 2016-08-16 17:27:50]

On 2016/08/16 at 17:26:28, boliu wrote:
> is this safe to revert?
> 
>
https://codereview.chromium.org/2230433002/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/platform/MemoryCoordinator.cpp (right):
> 
>
https://codereview.chromium.org/2230433002/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/platform/MemoryCoordinator.cpp:58:
client->onMemoryPressure(level);
> Getting a null pointer crash here.
>
https://uberchromegw.corp.google.com/i/internal.client.clank/builders/health-...
> 
> 	signal 11 (SIGSEGV), code 1, fault addr 0x1ba in tid 9575 (Chrome_InProcRe)
> 	pid: 9539, tid: 9575, name: Chrome_InProcRe  >>> org.chromium.webview_shell
<<<
> 	signal 11 (SIGSEGV), code 1 (SEGV_MAPERR), fault addr 0x1ba
> 	     r0 2e49378c  r1 00000002  r2 2e364b90  r3 00000192
> 	     r4 a0020728  r5 b761ac38  r6 00000002  r7 a0020718
> 	     r8 a61955bc  r9 a617bdaa  sl b75d2ae4  fp a5c260b6
> 	     ip ffffff00  sp a00206e8  lr a4ed2399  pc a4ed23c6
> 	
> 	Stack Trace:
> 	  RELADDR   FUNCTION                                                         
         FILE:LINE
> 	  011ab3c6 
blink::MemoryCoordinator::onMemoryPressure(blink::WebMemoryPressureLevel) 
/b/build/slave/arm-builder-rel/build/src/third_party/WebKit/Source/platform/MemoryCoordinator.cpp:58

Yeah just revert it.

[boliu, 2016-08-16 17:29:58]

A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.chromium.org/2252743002/ by boliu@chromium.org.

The reason for reverting is: Null pointer crash. See crbug.com/638274

BUG=638274.