Introduce a kill-switch for shipping features.

Introduce a kill-switch for shipping features.

R=rossberg@chromium.org
TBR=hpayer@chromium.org

Committed: https://chromium.googlesource.com/v8/v8/+/d6285620868b15c84eb9affa59f6b4e869f074b8

[Dmitry Lomov (no reviews), 2014-11-28 12:16:28]

PTAL

[rossberg, 2014-11-28 12:28:36]

https://codereview.chromium.org/763273002/diff/1/src/flag-definitions.h
File src/flag-definitions.h (right):

https://codereview.chromium.org/763273002/diff/1/src/flag-definitions.h#newco...
src/flag-definitions.h:206: DEFINE_IMPLICATION(harmony_shipping, id)
Hm, don't you also need a NEG_IMPLICATION here to actually make
--noharmony-shipping a kill switch?

https://codereview.chromium.org/763273002/diff/1/test/mjsunit/debug-script.js
File test/mjsunit/debug-script.js (right):

https://codereview.chromium.org/763273002/diff/1/test/mjsunit/debug-script.js...
test/mjsunit/debug-script.js:29: // Flags: --allow-natives-syntax
--noharmony-shipping
Why is this needed?

[Dmitry Lomov (no reviews), 2014-11-28 13:34:41]

comments addressed

https://codereview.chromium.org/763273002/diff/1/src/flag-definitions.h
File src/flag-definitions.h (right):

https://codereview.chromium.org/763273002/diff/1/src/flag-definitions.h#newco...
src/flag-definitions.h:206: DEFINE_IMPLICATION(harmony_shipping, id)
On 2014/11/28 12:28:36, rossberg wrote:
> Hm, don't you also need a NEG_IMPLICATION here to actually make
> --noharmony-shipping a kill switch?

No, why? NEG_IMPLICATION(flag1,flag2) means `if (flag1) flag2 = false` so not
applicable here.

The way this works here is that features are off by default, and
harmony_shipping turns them on.
harmony_shipping is on by default, so features are on.
If harmony_shipping is turned off via command line, feature flags take their
default value which is 'off'.

https://codereview.chromium.org/763273002/diff/1/test/mjsunit/debug-script.js
File test/mjsunit/debug-script.js (right):

https://codereview.chromium.org/763273002/diff/1/test/mjsunit/debug-script.js...
test/mjsunit/debug-script.js:29: // Flags: --allow-natives-syntax
--noharmony-shipping
On 2014/11/28 12:28:36, rossberg wrote:
> Why is this needed?

This test asserts, in line 63, that the number of scripts reported as native via
Debug.scripts() is the same as a number of core native scripts. Native scripts
that are added by --harmony-shipping are classified as 'EXPERIMENTAL' so disable
those.

Added comment to clarify that.

[rossberg, 2014-11-28 13:41:03]

lgtm

https://codereview.chromium.org/763273002/diff/1/src/flag-definitions.h
File src/flag-definitions.h (right):

https://codereview.chromium.org/763273002/diff/1/src/flag-definitions.h#newco...
src/flag-definitions.h:206: DEFINE_IMPLICATION(harmony_shipping, id)
On 2014/11/28 13:34:41, Dmitry Lomov (chromium) wrote:
> On 2014/11/28 12:28:36, rossberg wrote:
> > Hm, don't you also need a NEG_IMPLICATION here to actually make
> > --noharmony-shipping a kill switch?
> 
> No, why? NEG_IMPLICATION(flag1,flag2) means `if (flag1) flag2 = false` so not
> applicable here.
> 
> The way this works here is that features are off by default, and
> harmony_shipping turns them on.
> harmony_shipping is on by default, so features are on.
> If harmony_shipping is turned off via command line, feature flags take their
> default value which is 'off'.

Ah, I see.

[Dmitry Lomov (no reviews), 2014-11-28 13:42:24]

The CQ bit was checked by dslomov@chromium.org

[commit-bot: I haz the power, 2014-11-28 13:42:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/763273002/20001

[commit-bot: I haz the power, 2014-11-28 14:04:43]

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

[commit-bot: I haz the power, 2014-11-28 14:04:43]

Try jobs failed on following builders:
  v8_linux_arm64_rel on tryserver.v8
(http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel/builds/1402)

[Dmitry Lomov (no reviews), 2014-11-28 14:32:24]

Committed patchset #2 (id:20001) manually as
d6285620868b15c84eb9affa59f6b4e869f074b8 (presubmit successful).

[Dmitry Lomov (no reviews), 2014-11-28 18:27:24]

A revert of this CL (patchset #2 id:20001) has been created in
https://codereview.chromium.org/763353002/ by dslomov@chromium.org.

The reason for reverting is: Reverted for breaking nosnap:
http://chromegw.corp.google.com/i/client.v8/builders/V8%20Linux%20-%20nosnap/....

[Dmitry Lomov (no reviews), 2014-11-28 19:36:34]

dslomov@chromium.org changed reviewers:
+ hpayer@chromium.org

[Dmitry Lomov (no reviews), 2014-11-28 19:38:29]

Hannes, could you take a post-commit look at changes in test-api.cc?
This test started to fail under nosnap, and the reason is, I am pretty sure,
that with a bigger snapshot we do not fit in the new space, and the objects are
pushed into old space, where CollectAllGarbage is not guaranteed to execute all
weak handle callbacks

[Dmitry Lomov (no reviews), 2014-11-28 19:38:32]

The CQ bit was checked by dslomov@chromium.org

[commit-bot: I haz the power, 2014-11-28 19:39:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/763273002/40001

[commit-bot: I haz the power, 2014-11-28 20:07:17]

Committed patchset #3 (id:40001)

[Hannes Payer (out of office), 2014-12-01 16:17:22]

the test fix does not look good, you broke the test semantics.
CollectAllAvailableGarbage will call the emergency gc mechanism which will
result in seven full garbage collections. please verify what is going on and fix
the test appropriately. e.g. check if incremental marking is started. if this is
the case, you have to call
CollectAllGarbage(Heap::kAbortIncrementalMarkingMask);

[Dmitry Lomov (no reviews), 2014-12-01 17:34:52]

On 2014/12/01 16:17:22, Hannes Payer wrote:
> the test fix does not look good, you broke the test semantics.
> CollectAllAvailableGarbage will call the emergency gc mechanism which will
> result in seven full garbage collections. please verify what is going on and
fix
> the test appropriately. e.g. check if incremental marking is started. if this
is
> the case, you have to call
> CollectAllGarbage(Heap::kAbortIncrementalMarkingMask);

Thank you! Done in https://codereview.chromium.org/767453004/