[Blink Heap] Move isLowEndDevice to Memory Coordinator, attempt #3

[Blink Heap] Move isLowEndDevice to Memory Coordinator, attempt #3

A preparation for https://codereview.chromium.org/2475293003 to be able to
override this from tests.

Had to go with a static method approach. Seems like hanging it off the heap allocated object breaks blink_heap_unittests on Mac and Windows
(see https://codereview.chromium.org/2503433003 and https://codereview.chromium.org/2510353005).

BUG=None
TEST=Run the blink_heap_unittests locally (on Linux).

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

[whywhat, 2016-11-22 18:03:48]

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

[whywhat, 2016-11-22 18:03:48]

PTAL

[whywhat, 2016-11-22 18:04:33]

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

[commit-bot: I haz the power, 2016-11-22 18:04:48]

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-22 19:12:03]

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

[sof, 2016-11-22 19:12:03]

lgtm, but the description of a commit should describe what it does, and not much
more - the mention of previous CLs that didn't work out, aren't that interesting
for someone looking later.

The heap unittests pass with and without --enable-low-end-device-mode
presumably?

https://codereview.chromium.org/2518273003/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/MemoryCoordinator.cpp (right):

https://codereview.chromium.org/2518273003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/MemoryCoordinator.cpp:15: // static
nit: this is not commonly done for Blink static method definitions, so better
left out.

[commit-bot: I haz the power, 2016-11-22 22:12:00]

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

[commit-bot: I haz the power, 2016-11-22 22:12:01]

Dry run: This issue passed the CQ dry run.

[haraken, 2016-11-22 23:12:36]

LGTM

[whywhat, 2016-11-22 23:25:49]

Description was changed from

==========
[Blink Heap] Move isLowEndDevice to Memory Coordinator, attempt #3

A preparation for https://codereview.chromium.org/2475293003 to be able to
override this from tests.

Seems like hanging it off the heap allocated object breaks blink_heap_unittests
on Mac and Windows
(see https://codereview.chromium.org/2503433003 and
https://codereview.chromium.org/2510353005).

BUG=None
TEST=Run the blink_heap_unittests locally (on Linux).
==========

to

==========
[Blink Heap] Move isLowEndDevice to Memory Coordinator, attempt #3

A preparation for https://codereview.chromium.org/2475293003 to be able to
override this from tests.

Had to go with a static method approach. Seems like hanging it off the heap
allocated object breaks blink_heap_unittests on Mac and Windows
(see https://codereview.chromium.org/2503433003 and
https://codereview.chromium.org/2510353005).

BUG=None
TEST=Run the blink_heap_unittests locally (on Linux).
==========

[whywhat, 2016-11-22 23:39:24]

On 2016/11/22 at 19:12:03, sigbjornf wrote:
> lgtm, but the description of a commit should describe what it does, and not
much more - the mention of previous CLs that didn't work out, aren't that
interesting for someone looking later.

I feel they are relevant because they explain why the static method was chosen
(esp. since I'm going to add overriding mechanism later on).
Added a clarification to the description.

> 
> The heap unittests pass with and without --enable-low-end-device-mode
presumably?

Yes, but the reverted patches passed with and without the flag too :/

> 
>
https://codereview.chromium.org/2518273003/diff/1/third_party/WebKit/Source/p...
> File third_party/WebKit/Source/platform/MemoryCoordinator.cpp (right):
> 
>
https://codereview.chromium.org/2518273003/diff/1/third_party/WebKit/Source/p...
> third_party/WebKit/Source/platform/MemoryCoordinator.cpp:15: // static
> nit: this is not commonly done for Blink static method definitions, so better
left out.

My code search shows that it's used in 19 out of 140 static members in
WebKit/Source/platform (and ~140 out of ~2100 in WebKit) so while it's not that
common I'd prefer to leave it. IMO it improves readability when it's there and
doesn't hurt it when not used across multiple files/modules consistently.
Hopefully the trend in Blink is to use it more often than not.

[whywhat, 2016-11-22 23:39:26]

The CQ bit was checked by avayvod@chromium.org

[commit-bot: I haz the power, 2016-11-22 23:40:08]

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-22 23:54:23]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1479857966607450, "parent_rev":
"1506fdd936d1ec2addb9991e90df0b2260682a15", "commit_rev":
"6635430f174759d7f68baa2e5efaa8659f5307f2"}

[commit-bot: I haz the power, 2016-11-22 23:54:47]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-11-22 23:57:53]

Description was changed from

==========
[Blink Heap] Move isLowEndDevice to Memory Coordinator, attempt #3

A preparation for https://codereview.chromium.org/2475293003 to be able to
override this from tests.

Had to go with a static method approach. Seems like hanging it off the heap
allocated object breaks blink_heap_unittests on Mac and Windows
(see https://codereview.chromium.org/2503433003 and
https://codereview.chromium.org/2510353005).

BUG=None
TEST=Run the blink_heap_unittests locally (on Linux).
==========

to

==========
[Blink Heap] Move isLowEndDevice to Memory Coordinator, attempt #3

A preparation for https://codereview.chromium.org/2475293003 to be able to
override this from tests.

Had to go with a static method approach. Seems like hanging it off the heap
allocated object breaks blink_heap_unittests on Mac and Windows
(see https://codereview.chromium.org/2503433003 and
https://codereview.chromium.org/2510353005).

BUG=None
TEST=Run the blink_heap_unittests locally (on Linux).

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

[commit-bot: I haz the power, 2016-11-22 23:57:54]

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

[sof, 2016-11-23 06:18:09]

On 2016/11/22 23:39:24, whywhat wrote:
> On 2016/11/22 at 19:12:03, sigbjornf wrote:
> > lgtm, but the description of a commit should describe what it does, and not
> much more - the mention of previous CLs that didn't work out, aren't that
> interesting for someone looking later.
> 
> I feel they are relevant because they explain why the static method was chosen
> (esp. since I'm going to add overriding mechanism later on).
> Added a clarification to the description.
> 

The description is unfocused (and badly formatted); target doesn't matter
here (see the initial comment on why the NestedAllocation test failing.)

The initial approach you went with wasn't complete, so mentioning
those is rather uninteresting.

[whywhat, 2016-11-23 18:29:42]

On 2016/11/23 at 06:18:09, sigbjornf wrote:
> On 2016/11/22 23:39:24, whywhat wrote:
> > On 2016/11/22 at 19:12:03, sigbjornf wrote:
> > > lgtm, but the description of a commit should describe what it does, and
not
> > much more - the mention of previous CLs that didn't work out, aren't that
> > interesting for someone looking later.
> > 
> > I feel they are relevant because they explain why the static method was
chosen
> > (esp. since I'm going to add overriding mechanism later on).
> > Added a clarification to the description.
> > 
> 

Let me try to understand this better so I could improve :)

> The description is unfocused (and badly formatted);

I take 'badly formatted' is because one sentence is longer than 72 chars so it
looks bad in git log? I admit it's my bad.
I'd take constructive criticism for the "unfocused" part. What would you write?

> target doesn't matter
> here (see the initial comment on why the NestedAllocation test failing.)

Do you mean static vs. non-static member by "target"?

IIRC, you mentioned two possible causes for the failures: extra heap allocation
during the test and the test being non-thread-safe.
I wasn't able to confirm these though because the tests passed locally for me.

> 
> The initial approach you went with wasn't complete, so mentioning
> those is rather uninteresting.

By 'complete' do you mean that I could've fixed the failing tests?

----------------------------------------------------

My view was that someone may stumble upon the static method in the future and
think, "hey, why don't we make it non-static and hang it off the already static
MC instance". Well, if they look at this change, they will find out it was tried
already and what were the obstacles by reading through the comments on the
previous CLs.

[sof, 2016-11-23 19:25:08]

On 2016/11/23 18:29:42, whywhat wrote:
> On 2016/11/23 at 06:18:09, sigbjornf wrote:
> > On 2016/11/22 23:39:24, whywhat wrote:
> > > On 2016/11/22 at 19:12:03, sigbjornf wrote:
> > > > lgtm, but the description of a commit should describe what it does, and
> not
> > > much more - the mention of previous CLs that didn't work out, aren't that
> > > interesting for someone looking later.
> > > 
> > > I feel they are relevant because they explain why the static method was
> chosen
> > > (esp. since I'm going to add overriding mechanism later on).
> > > Added a clarification to the description.
> > > 
> > 
> 
> Let me try to understand this better so I could improve :)
> 
> > The description is unfocused (and badly formatted);
> 
> I take 'badly formatted' is because one sentence is longer than 72 chars so it
> looks bad in git log? I admit it's my bad.
> I'd take constructive criticism for the "unfocused" part. What would you
write?
> 
> > target doesn't matter
> > here (see the initial comment on why the NestedAllocation test failing.)
> 
> Do you mean static vs. non-static member by "target"?
> 

No, Mac and Windows - the underlying problems could show up on any
target/platform, depending on GC timing and heap state.

> IIRC, you mentioned two possible causes for the failures: extra heap
allocation
> during the test and the test being non-thread-safe.

Yes, the failures highlighted two problems afaict

 - a unit test that happens to trigger a GC
     => touches the heap-allocated singleton for the first time when it sweeps
      => brings about extra allocations
       => skews the heap size counting that the test depends on.

 - service worker tests that access the main thread heap's
MemoryCoordinator::instance() object after the heap has shut down. 

> I wasn't able to confirm these though because the tests passed locally for me.
> 

Stepping through the code to either come up with or verify semi-plausible
theories for seemingly non-deterministic GC issues, is sometimes all there is.

Running the unit tests in single-process mode might have elicited the first
problem..not sure.

> > 
> > The initial approach you went with wasn't complete, so mentioning
> > those is rather uninteresting.
> 
> By 'complete' do you mean that I could've fixed the failing tests?
> 

Not 'complete' in that didn't consider the (non-obvious) implications of using a
main thread heap object from inside the Oilpan GC.

> ----------------------------------------------------
> 
> My view was that someone may stumble upon the static method in the future and
> think, "hey, why don't we make it non-static and hang it off the already
static
> MC instance". Well, if they look at this change, they will find out it was
tried
> already and what were the obstacles by reading through the comments on the
> previous CLs.

If the description had said "This replaces an approach which used
MC::instance(), because..", they'd be informed. Or you could have put a comment
next to the static predicate to hint why it is kept separate.

[whywhat, 2016-11-23 20:40:39]

On 2016/11/23 at 19:25:08, sigbjornf wrote:
> On 2016/11/23 18:29:42, whywhat wrote:
> > On 2016/11/23 at 06:18:09, sigbjornf wrote:
> > > On 2016/11/22 23:39:24, whywhat wrote:
> > > > On 2016/11/22 at 19:12:03, sigbjornf wrote:
> > > > > lgtm, but the description of a commit should describe what it does,
and
> > not
> > > > much more - the mention of previous CLs that didn't work out, aren't
that
> > > > interesting for someone looking later.
> > > > 
> > > > I feel they are relevant because they explain why the static method was
> > chosen
> > > > (esp. since I'm going to add overriding mechanism later on).
> > > > Added a clarification to the description.
> > > > 
> > > 
> > 
> > Let me try to understand this better so I could improve :)
> > 
> > > The description is unfocused (and badly formatted);
> > 
> > I take 'badly formatted' is because one sentence is longer than 72 chars so
it
> > looks bad in git log? I admit it's my bad.
> > I'd take constructive criticism for the "unfocused" part. What would you
write?
> > 
> > > target doesn't matter
> > > here (see the initial comment on why the NestedAllocation test failing.)
> > 
> > Do you mean static vs. non-static member by "target"?
> > 
> 
> No, Mac and Windows - the underlying problems could show up on any
target/platform, depending on GC timing and heap state.
> 
> > IIRC, you mentioned two possible causes for the failures: extra heap
allocation
> > during the test and the test being non-thread-safe.
> 
> Yes, the failures highlighted two problems afaict
> 
>  - a unit test that happens to trigger a GC
>      => touches the heap-allocated singleton for the first time when it sweeps
>       => brings about extra allocations
>        => skews the heap size counting that the test depends on.
> 
>  - service worker tests that access the main thread heap's
MemoryCoordinator::instance() object after the heap has shut down. 
> 
> > I wasn't able to confirm these though because the tests passed locally for
me.
> > 
> 
> Stepping through the code to either come up with or verify semi-plausible
theories for seemingly non-deterministic GC issues, is sometimes all there is.
> 
> Running the unit tests in single-process mode might have elicited the first
problem..not sure.
> 
> > > 
> > > The initial approach you went with wasn't complete, so mentioning
> > > those is rather uninteresting.
> > 
> > By 'complete' do you mean that I could've fixed the failing tests?
> > 
> 
> Not 'complete' in that didn't consider the (non-obvious) implications of using
a main thread heap object from inside the Oilpan GC.
> 
> > ----------------------------------------------------
> > 
> > My view was that someone may stumble upon the static method in the future
and
> > think, "hey, why don't we make it non-static and hang it off the already
static
> > MC instance". Well, if they look at this change, they will find out it was
tried
> > already and what were the obstacles by reading through the comments on the
> > previous CLs.
> 
> If the description had said "This replaces an approach which used
MC::instance(), because..", they'd be informed. Or you could have put a comment
next to the static predicate to hint why it is kept separate.

Thanks for your patient replies! I will add a comment in the follow-up CL that I
have in the queue.