Do not attempt nested TCO #721

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
2 participants
@peferron

peferron commented Feb 8, 2015

With v3.5.0 and v3.5.1, The following snippet throws when executed with 6to5-node, but not with 6to5-node --blacklist es6.tailCall:

function foo() {
  return "foo";
}

function fooWrapper() {
  return foo();
}

function test() {
  var str = fooWrapper();
  if (str !== "foo") {
    throw new Error();
  }
}

function testWrapper() {
  return test();
}

testWrapper();

I have submitted a very simple change to fix the issue. There might be a more optimized way to do it, and I didn't add tests because I had trouble getting them to run (first time attempting to contribute to 6to5), however breaking valid code is an urgent issue so I just went ahead and created this pull request. I just saw that a few minutes ago 3.5.2 completely disabled TCO, so I guess it's not urgent anymore. :)

If you want me to add tests I'll be happy to work a bit more on it tomorrow.

@kittens

This comment has been minimized.

Show comment
Hide comment
@kittens

kittens Feb 8, 2015

Member

Yeah, it's an issue with nested callbacks that we didn't anticipate. Transforming code is hard work 😜 This isn't necessary anymore as I just pushed out 3.5.3 which uses the first implementation (only optimises self referencing tail calls). Thanks for this though! Great to see more interest in the codebase. I'll work on getting a more extensive CONTRIBUTING guide up soon that explains the different internal components and development workflow.

Member

kittens commented Feb 8, 2015

Yeah, it's an issue with nested callbacks that we didn't anticipate. Transforming code is hard work 😜 This isn't necessary anymore as I just pushed out 3.5.3 which uses the first implementation (only optimises self referencing tail calls). Thanks for this though! Great to see more interest in the codebase. I'll work on getting a more extensive CONTRIBUTING guide up soon that explains the different internal components and development workflow.

@kittens kittens closed this Feb 8, 2015

@peferron

This comment has been minimized.

Show comment
Hide comment
@peferron

peferron Feb 8, 2015

Even if you manage to make nested calls work, please also pay attention to maintaining performance in traditional ES5 situations. For example, this perfectly fine ES5 snippet:

function addOne(a) {
  return add(a, 1);
}

function add(a, b) {
  return a + b;
}

var i = 0;
while (i < 1e8) {
  i = addOne(i);
}

runs almost 100x slower with 3.5.0/3.5.1 TCO than without:

$ cat bench.js | time node
node  0.31s user 0.02s system 100% cpu 0.324 total

$ 6to5 bench.js | time node
node  21.65s user 0.22s system 98% cpu 22.197 total

because the TCO adds a ton of overhead every time addOne calls add.

I don't use 6to5 in production, but I'm pretty sure that people who do would rather be missing an ES6 feature rather than seeing their perfectly fine ES5 code suddenly start underperforming.

Lots of love to 6to5 though 👍

peferron commented Feb 8, 2015

Even if you manage to make nested calls work, please also pay attention to maintaining performance in traditional ES5 situations. For example, this perfectly fine ES5 snippet:

function addOne(a) {
  return add(a, 1);
}

function add(a, b) {
  return a + b;
}

var i = 0;
while (i < 1e8) {
  i = addOne(i);
}

runs almost 100x slower with 3.5.0/3.5.1 TCO than without:

$ cat bench.js | time node
node  0.31s user 0.02s system 100% cpu 0.324 total

$ 6to5 bench.js | time node
node  21.65s user 0.22s system 98% cpu 22.197 total

because the TCO adds a ton of overhead every time addOne calls add.

I don't use 6to5 in production, but I'm pretty sure that people who do would rather be missing an ES6 feature rather than seeing their perfectly fine ES5 code suddenly start underperforming.

Lots of love to 6to5 though 👍

@kittens

This comment has been minimized.

Show comment
Hide comment
@kittens

kittens Feb 8, 2015

Member

@peferron Yeah, if it's implemented beyond self-recursion in the future it'll be behind an optional transformer. Definently not comfortable with adding significant perf impact. Lesson learnt, definently going to hold back in the future with these kind of signifcant changes until they can be vetted more thoroughly.

Member

kittens commented Feb 8, 2015

@peferron Yeah, if it's implemented beyond self-recursion in the future it'll be behind an optional transformer. Definently not comfortable with adding significant perf impact. Lesson learnt, definently going to hold back in the future with these kind of signifcant changes until they can be vetted more thoroughly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment