Move ProcessHeap::isLowEndDevice to MemoryCoordinator

Move ProcessHeap::isLowEndDevice to MemoryCoordinator

Since this is just moving code around, both me and haraken@ agreed that
it couldn't be the real reason behind the blink_heap_unittests failures.
Hence relanding https://codereview.chromium.org/2503433003

BUG=None
TEST=Run the failing tests locally (on Linux)

Committed: https://crrev.com/a3fa4889e144bcee7491380c0364c46eb9fa4da4
Cr-Commit-Position: refs/heads/master@{#433613}

[whywhat, 2016-11-18 19:15:09]

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

[whywhat, 2016-11-18 19:15:22]

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

[whywhat, 2016-11-18 19:15:23]

Please, take a look.

[commit-bot: I haz the power, 2016-11-18 19:16:07]

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

[sof, 2016-11-18 20:17:26]

sigbjornf@opera.com changed reviewers:
+ sigbjornf@opera.com

[sof, 2016-11-18 20:17:26]

Strange one; lgtm to retry.

[commit-bot: I haz the power, 2016-11-18 21:35:05]

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

[commit-bot: I haz the power, 2016-11-18 21:35:06]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[haraken, 2016-11-19 11:43:36]

LGTM

[whywhat, 2016-11-21 16:57:24]

The CQ bit was checked by avayvod@chromium.org

[commit-bot: I haz the power, 2016-11-21 16:57:58]

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-11-21 19:52:39]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1479747444622140, "parent_rev":
"f0cdb2eeada19a8ea9358385b69b1ef2fdecb2b6", "commit_rev":
"739fed5f8e57792b41287719a03d97445a5ac2cc"}

[commit-bot: I haz the power, 2016-11-21 19:53:07]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-11-21 19:58:21]

Description was changed from

==========
Move ProcessHeap::isLowEndDevice to MemoryCoordinator

Since this is just moving code around, both me and haraken@ agreed that
it couldn't be the real reason behind the blink_heap_unittests failures.
Hence relanding https://codereview.chromium.org/2503433003

BUG=None
TEST=Run the failing tests locally (on Linux)
==========

to

==========
Move ProcessHeap::isLowEndDevice to MemoryCoordinator

Since this is just moving code around, both me and haraken@ agreed that
it couldn't be the real reason behind the blink_heap_unittests failures.
Hence relanding https://codereview.chromium.org/2503433003

BUG=None
TEST=Run the failing tests locally (on Linux)

Committed: https://crrev.com/a3fa4889e144bcee7491380c0364c46eb9fa4da4
Cr-Commit-Position: refs/heads/master@{#433613}
==========

[commit-bot: I haz the power, 2016-11-21 19:58:22]

Patchset 1 (id:??) landed as
https://crrev.com/a3fa4889e144bcee7491380c0364c46eb9fa4da4
Cr-Commit-Position: refs/heads/master@{#433613}

[piman, 2016-11-21 20:34:59]

A revert of this CL (patchset #1 id:1) has been created in
https://codereview.chromium.org/2522653002/ by piman@chromium.org.

The reason for reverting is: Likely cause for blink_heap_unittests and
webkit_unit_tests failures. See
https://uberchromegw.corp.google.com/i/chromium.webkit/builders/WebKit%20Win%....

[whywhat, 2016-11-21 20:40:41]

well, okay, I'm out of options :)

are the tests flaky?
why don't they run on the trybots?

[sof, 2016-11-21 21:07:18]

On 2016/11/21 20:40:41, whywhat wrote:
> well, okay, I'm out of options :)
> 
> are the tests flaky?
> why don't they run on the trybots?

Could it be that the test is touching MemoryCoordinator::instance() for the
first time? => It causes a heap allocation, which makes the allocated object
sizes that the test expects, not match up.

[piman, 2016-11-21 21:15:17]

On 2016/11/21 21:07:18, sof wrote:
> On 2016/11/21 20:40:41, whywhat wrote:
> > well, okay, I'm out of options :)
> > 
> > are the tests flaky?
> > why don't they run on the trybots?
> 
> Could it be that the test is touching MemoryCoordinator::instance() for the
> first time? => It causes a heap allocation, which makes the allocated object
> sizes that the test expects, not match up.

The revert appears to not have helped... investigating more. I will reland once
we found the cause if this is unrelated.

[piman, 2016-11-21 21:41:54]

On 2016/11/21 21:15:17, piman wrote:
> On 2016/11/21 21:07:18, sof wrote:
> > On 2016/11/21 20:40:41, whywhat wrote:
> > > well, okay, I'm out of options :)
> > > 
> > > are the tests flaky?
> > > why don't they run on the trybots?
> > 
> > Could it be that the test is touching MemoryCoordinator::instance() for the
> > first time? => It causes a heap allocation, which makes the allocated object
> > sizes that the test expects, not match up.
> 
> The revert appears to not have helped... investigating more. I will reland
once
> we found the cause if this is unrelated.

Nevermind, it's a display issue on the waterfall, I checked that the revert does
fix things locally, and that the bots that still have problems haven't actually
picked up the revert. So this CL is likely the culprit.

[whywhat, 2016-11-21 21:43:57]

On 2016/11/21 at 21:41:54, piman wrote:
> On 2016/11/21 21:15:17, piman wrote:
> > On 2016/11/21 21:07:18, sof wrote:
> > > On 2016/11/21 20:40:41, whywhat wrote:
> > > > well, okay, I'm out of options :)
> > > > 
> > > > are the tests flaky?
> > > > why don't they run on the trybots?
> > > 
> > > Could it be that the test is touching MemoryCoordinator::instance() for
the
> > > first time? => It causes a heap allocation, which makes the allocated
object
> > > sizes that the test expects, not match up.
> > 
> > The revert appears to not have helped... investigating more. I will reland
once
> > we found the cause if this is unrelated.
> 
> Nevermind, it's a display issue on the waterfall, I checked that the revert
does fix things locally, and that the bots that still have problems haven't
actually picked up the revert. So this CL is likely the culprit.

Were you able to reproduce the failure locally? What platform? The tests passed
for me on Linux.

[sof, 2016-11-21 22:00:23]

On 2016/11/21 21:43:57, whywhat wrote:
> On 2016/11/21 at 21:41:54, piman wrote:
> > On 2016/11/21 21:15:17, piman wrote:
> > > On 2016/11/21 21:07:18, sof wrote:
> > > > On 2016/11/21 20:40:41, whywhat wrote:
> > > > > well, okay, I'm out of options :)
> > > > > 
> > > > > are the tests flaky?
> > > > > why don't they run on the trybots?
> > > > 
> > > > Could it be that the test is touching MemoryCoordinator::instance() for
> the
> > > > first time? => It causes a heap allocation, which makes the allocated
> object
> > > > sizes that the test expects, not match up.
> > > 
> > > The revert appears to not have helped... investigating more. I will reland
> once
> > > we found the cause if this is unrelated.
> > 
> > Nevermind, it's a display issue on the waterfall, I checked that the revert
> does fix things locally, and that the bots that still have problems haven't
> actually picked up the revert. So this CL is likely the culprit.
> 
> Were you able to reproduce the failure locally? What platform? The tests
passed
> for me on Linux.

The worker tests are failing after the main thread has shut down, I reckon --
accessing MemoryCoordinator::instance() from a non-main thread isn't safe.

I suggest you expose

   static bool MemoryCoordinator::isLowMemoryDevice();

instead & don't involve the (heap allocated) singleton.

[whywhat, 2016-11-22 17:48:43]

On 2016/11/21 at 22:00:23, sigbjornf wrote:
> On 2016/11/21 21:43:57, whywhat wrote:
> > On 2016/11/21 at 21:41:54, piman wrote:
> > > On 2016/11/21 21:15:17, piman wrote:
> > > > On 2016/11/21 21:07:18, sof wrote:
> > > > > On 2016/11/21 20:40:41, whywhat wrote:
> > > > > > well, okay, I'm out of options :)
> > > > > > 
> > > > > > are the tests flaky?
> > > > > > why don't they run on the trybots?
> > > > > 
> > > > > Could it be that the test is touching MemoryCoordinator::instance()
for
> > the
> > > > > first time? => It causes a heap allocation, which makes the allocated
> > object
> > > > > sizes that the test expects, not match up.
> > > > 
> > > > The revert appears to not have helped... investigating more. I will
reland
> > once
> > > > we found the cause if this is unrelated.
> > > 
> > > Nevermind, it's a display issue on the waterfall, I checked that the
revert
> > does fix things locally, and that the bots that still have problems haven't
> > actually picked up the revert. So this CL is likely the culprit.
> > 
> > Were you able to reproduce the failure locally? What platform? The tests
passed
> > for me on Linux.
> 
> The worker tests are failing after the main thread has shut down, I reckon --
accessing MemoryCoordinator::instance() from a non-main thread isn't safe.
> 
> I suggest you expose
> 
>    static bool MemoryCoordinator::isLowMemoryDevice();
> 
> instead & don't involve the (heap allocated) singleton.

The purpose of this change is also for being able to override the bool in tests
via Internals.cpp. How would I initialize a static bool with
base::SysInfo::IsLowEndDevice unless it's overridden?

Could I use a static wtf::Optional<bool> or a bool pointer something? It would
need to be thread-safe too I guess (though MemoryCoordinator is designed to only
be accessed via the main thread, isLowEndDevice is probably accessed from
another thread as the tests show).

[sof, 2016-11-22 18:47:23]

On 2016/11/22 17:48:43, whywhat wrote:
> On 2016/11/21 at 22:00:23, sigbjornf wrote:
> > On 2016/11/21 21:43:57, whywhat wrote:
> > > On 2016/11/21 at 21:41:54, piman wrote:
> > > > On 2016/11/21 21:15:17, piman wrote:
> > > > > On 2016/11/21 21:07:18, sof wrote:
> > > > > > On 2016/11/21 20:40:41, whywhat wrote:
> > > > > > > well, okay, I'm out of options :)
> > > > > > > 
> > > > > > > are the tests flaky?
> > > > > > > why don't they run on the trybots?
> > > > > > 
> > > > > > Could it be that the test is touching MemoryCoordinator::instance()
> for
> > > the
> > > > > > first time? => It causes a heap allocation, which makes the
allocated
> > > object
> > > > > > sizes that the test expects, not match up.
> > > > > 
> > > > > The revert appears to not have helped... investigating more. I will
> reland
> > > once
> > > > > we found the cause if this is unrelated.
> > > > 
> > > > Nevermind, it's a display issue on the waterfall, I checked that the
> revert
> > > does fix things locally, and that the bots that still have problems
haven't
> > > actually picked up the revert. So this CL is likely the culprit.
> > > 
> > > Were you able to reproduce the failure locally? What platform? The tests
> passed
> > > for me on Linux.
> > 
> > The worker tests are failing after the main thread has shut down, I reckon
--
> accessing MemoryCoordinator::instance() from a non-main thread isn't safe.
> > 
> > I suggest you expose
> > 
> >    static bool MemoryCoordinator::isLowMemoryDevice();
> > 
> > instead & don't involve the (heap allocated) singleton.
> 
> The purpose of this change is also for being able to override the bool in
tests
> via Internals.cpp. How would I initialize a static bool with
> base::SysInfo::IsLowEndDevice unless it's overridden?
> 

This would be a static predicate, so it could consult whatever initialized local
static state to accomplish that.

base::SysInfo does some of this already for its low-end predicate, consulting
command-line switches.