[api] Add a switch that controls if ES2015 tail call elimination feature is enabled or not.

[api] Add a switch that controls if ES2015 tail call elimination feature is enabled or not.

BUG=v8:4698
LOG=N
TBR=rossberg@chromium.org

Committed: https://crrev.com/037f7f62159fe673e29d12419033ddc53ac5002e
Cr-Commit-Position: refs/heads/master@{#35132}

[Igor Sheludko, 2016-03-29 15:10:42]

ishell@chromium.org changed reviewers:
+ jochen@chromium.org, verwaest@chromium.org

[Igor Sheludko, 2016-03-29 15:10:43]

PTAL. Jochen - API, Toon - the rest.

[jochen (gone - plz use gerrit), 2016-03-29 15:12:14]

jochen@chromium.org changed reviewers:
+ dgozman@chromium.org, yangguo@chromium.org
- verwaest@chromium.org

[jochen (gone - plz use gerrit), 2016-03-29 15:12:15]

+yang/dimitri for their input

https://codereview.chromium.org/1842763002/diff/1/include/v8-debug.h
File include/v8-debug.h (right):

https://codereview.chromium.org/1842763002/diff/1/include/v8-debug.h#newcode284
include/v8-debug.h:284: static void SetTailCallEliminationEnabled(Isolate*
isolate, bool enabled);
Can you add a comment saying whether this deopts all existing code?

In the future, it might be the case that you can't disable TCO, right? maybe
SetTail... should return a bool indicating whether or not it was indeed
disabled?

or should we just automatically disable TCO if the debugger is engaged?

[Igor Sheludko, 2016-03-29 15:32:18]

ishell@chromium.org changed reviewers:
+ pfeldman@chromium.org

[Igor Sheludko, 2016-03-29 15:32:19]

+pfeldman

https://codereview.chromium.org/1842763002/diff/1/include/v8-debug.h
File include/v8-debug.h (right):

https://codereview.chromium.org/1842763002/diff/1/include/v8-debug.h#newcode284
include/v8-debug.h:284: static void SetTailCallEliminationEnabled(Isolate*
isolate, bool enabled);
On 2016/03/29 15:12:15, jochen - slow wrote:
> Can you add a comment saying whether this deopts all existing code?
> 
> In the future, it might be the case that you can't disable TCO, right? maybe
> SetTail... should return a bool indicating whether or not it was indeed
> disabled?
> 
> or should we just automatically disable TCO if the debugger is engaged?

1) Probably when we decide to avoid disabling TCO we should remove these
functions from the API.
2) We decided to separate "TCO enabled" from "Debugger is enabled" in order to
let the user debug his code the same way it is run in without debugger. But if
user wants to see less confusing picture it could disable TCO in DevTools.

[Igor Sheludko, 2016-03-29 15:34:26]

Description was changed from

==========
[api] Added switch that controls if ES2015 tail call elimination feature is
enabled or not.

BUG=v8:4698
LOG=N
==========

to

==========
[api] Add a switch that controls if ES2015 tail call elimination feature is
enabled or not.

BUG=v8:4698
LOG=N
==========

[Igor Sheludko, 2016-03-29 15:35:09]

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

[commit-bot: I haz the power, 2016-03-29 15:35:12]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1842763002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1842763002/20001

[commit-bot: I haz the power, 2016-03-29 15:50:58]

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

[commit-bot: I haz the power, 2016-03-29 15:50:59]

Dry run: Try jobs failed on following builders:
  v8_linux64_asan_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel/builds/...)

[Igor Sheludko, 2016-03-29 16:51:59]

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

[commit-bot: I haz the power, 2016-03-29 16:52:10]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1842763002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1842763002/40001

[commit-bot: I haz the power, 2016-03-29 17:05:25]

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

[commit-bot: I haz the power, 2016-03-29 17:05:26]

Dry run: Try jobs failed on following builders:
  v8_mac_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel/builds/17660)

[Igor Sheludko, 2016-03-29 17:34:32]

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

[commit-bot: I haz the power, 2016-03-29 17:34:42]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1842763002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1842763002/60001

[commit-bot: I haz the power, 2016-03-29 17:56:51]

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

[commit-bot: I haz the power, 2016-03-29 17:56:52]

Dry run: This issue passed the CQ dry run.

[Yang, 2016-03-30 07:33:02]

On 2016/03/29 17:56:52, commit-bot: I haz the power wrote:
> Dry run: This issue passed the CQ dry run.

I don't think we should automatically disable TCO on enabling devtools. That
would immediately change both the script's behavior and the performance
characteristics. Instead, we should make the user aware of the option to turn
TCO off.

[Yang, 2016-03-30 07:38:55]

On 2016/03/30 07:33:02, Yang wrote:
> On 2016/03/29 17:56:52, commit-bot: I haz the power wrote:
> > Dry run: This issue passed the CQ dry run.
> 
> I don't think we should automatically disable TCO on enabling devtools. That
> would immediately change both the script's behavior and the performance
> characteristics. Instead, we should make the user aware of the option to turn
> TCO off.

LGTM.

[Benedikt Meurer, 2016-03-30 10:56:29]

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

[Benedikt Meurer, 2016-03-30 10:56:29]

LGTM on compiler.

[jochen (gone - plz use gerrit), 2016-03-30 10:59:15]

lgtm

[Igor Sheludko, 2016-03-30 11:01:22]

Description was changed from

==========
[api] Add a switch that controls if ES2015 tail call elimination feature is
enabled or not.

BUG=v8:4698
LOG=N
==========

to

==========
[api] Add a switch that controls if ES2015 tail call elimination feature is
enabled or not.

BUG=v8:4698
LOG=N
TBR=rossberg@chromium.org
==========

[Igor Sheludko, 2016-03-30 11:02:30]

The CQ bit was checked by ishell@chromium.org

[commit-bot: I haz the power, 2016-03-30 11:02:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1842763002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1842763002/60001

[commit-bot: I haz the power, 2016-03-30 11:04:27]

Description was changed from

==========
[api] Add a switch that controls if ES2015 tail call elimination feature is
enabled or not.

BUG=v8:4698
LOG=N
TBR=rossberg@chromium.org
==========

to

==========
[api] Add a switch that controls if ES2015 tail call elimination feature is
enabled or not.

BUG=v8:4698
LOG=N
TBR=rossberg@chromium.org
==========

[commit-bot: I haz the power, 2016-03-30 11:04:29]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-03-30 11:05:26]

Description was changed from

==========
[api] Add a switch that controls if ES2015 tail call elimination feature is
enabled or not.

BUG=v8:4698
LOG=N
TBR=rossberg@chromium.org
==========

to

==========
[api] Add a switch that controls if ES2015 tail call elimination feature is
enabled or not.

BUG=v8:4698
LOG=N
TBR=rossberg@chromium.org

Committed: https://crrev.com/037f7f62159fe673e29d12419033ddc53ac5002e
Cr-Commit-Position: refs/heads/master@{#35132}
==========

[commit-bot: I haz the power, 2016-03-30 11:05:27]

Patchset 4 (id:??) landed as
https://crrev.com/037f7f62159fe673e29d12419033ddc53ac5002e
Cr-Commit-Position: refs/heads/master@{#35132}

[dgozman, 2016-03-30 16:51:00]

On 2016/03/30 07:33:02, Yang wrote:
> On 2016/03/29 17:56:52, commit-bot: I haz the power wrote:
> > Dry run: This issue passed the CQ dry run.
> 
> I don't think we should automatically disable TCO on enabling devtools. That
> would immediately change both the script's behavior and the performance
> characteristics. Instead, we should make the user aware of the option to turn
> TCO off.

I think we can do better here: disable TCO when debugging (with option to enable
it back?), and enable TCO while profiling.
Most developers don't need to know about disabling TCO, and if they debug
something really affected by TCO - should be able to figure out how to turn it
on.

[Yang, 2016-03-30 16:55:57]

On 2016/03/30 16:51:00, dgozman wrote:
> On 2016/03/30 07:33:02, Yang wrote:
> > On 2016/03/29 17:56:52, commit-bot: I haz the power wrote:
> > > Dry run: This issue passed the CQ dry run.
> > 
> > I don't think we should automatically disable TCO on enabling devtools. That
> > would immediately change both the script's behavior and the performance
> > characteristics. Instead, we should make the user aware of the option to
turn
> > TCO off.
> 
> I think we can do better here: disable TCO when debugging (with option to
enable
> it back?), and enable TCO while profiling.
> Most developers don't need to know about disabling TCO, and if they debug
> something really affected by TCO - should be able to figure out how to turn it
> on.

Toggling tco deopts all optimized code, which is both expensive by itself and
resets everything to running non-optimized code. We should avoid it as much as
possible. Then again the decision to toggle is entirely on your end.

[Igor Sheludko, 2016-03-30 16:57:35]

On 2016/03/30 16:51:00, dgozman wrote:
> On 2016/03/30 07:33:02, Yang wrote:
> > On 2016/03/29 17:56:52, commit-bot: I haz the power wrote:
> > > Dry run: This issue passed the CQ dry run.
> > 
> > I don't think we should automatically disable TCO on enabling devtools. That
> > would immediately change both the script's behavior and the performance
> > characteristics. Instead, we should make the user aware of the option to
turn
> > TCO off.
> 
> I think we can do better here: disable TCO when debugging (with option to
enable
> it back?), and enable TCO while profiling.
> Most developers don't need to know about disabling TCO, and if they debug
> something really affected by TCO - should be able to figure out how to turn it
> on.

Now we have a separate switch for enabling/disabling TCO and DevTools could use
it to implement whatever logic is best for developers.

[pfeldman, 2016-03-30 17:31:47]

> Now we have a separate switch for enabling/disabling TCO and DevTools could
use
> it to implement whatever logic is best for developers.

Exactly. dgozman@ does not have enough context for the matter, I'll bring him
up-to-date.