Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

let support for ES3/ES5 #1690

Closed
basarat opened this issue Jan 15, 2015 · 26 comments
Closed

let support for ES3/ES5 #1690

basarat opened this issue Jan 15, 2015 · 26 comments
Labels
Committed The team has roadmapped this issue Fixed A PR has been merged for this issue Suggestion An idea for TypeScript

Comments

@basarat
Copy link
Contributor

basarat commented Jan 15, 2015

Any plan for ES3/ES5 support similar to this

let x = 123;
let _x = 123;
let _x2 = 123;
{
  let x = 56
  console.log(x);
}
var x = 123;
var _x = 123;
var _x2 = 123;
{
  var _x3 = 56;
  console.log(_x3);
}
@RyanCavanaugh RyanCavanaugh added Suggestion An idea for TypeScript Too Complex An issue which adding support for may be too complex for the value it adds labels Jan 15, 2015
@RyanCavanaugh
Copy link
Member

We're not planning on it at the moment. There are some cases like loop variables where the rewriting necessary is very invasive in terms of performance overhead, e.g. in a loop you need to wrap the loop body in a function, which could easily be a 10x perf hit:

for(let i = 0; i < 10; i++) {
  window.setTimeout(() => { 
      console.log(i)
    }, 10);
}

The proper emit for this would be:

for (var i = 0; i < 10; i++) {
  (function (i) {
    window.setTimeout(function () {
      console.log(i);
    }, 10);
  })(i);
}

@basarat
Copy link
Contributor Author

basarat commented Jan 16, 2015

There are some cases like loop variables where the rewriting necessary is very invasive in terms of performance overhead, e.g. in a loop you need to wrap the loop body in a function, which could easily be a 10x perf hit

Probably just me but I don't see it:

var count = 1000;

console.time('var');
for (var i = 1; i <= count; i++) {  
  var varCallCount = 1;
  window.setTimeout(function () {
    if(varCallCount++ == count) console.timeEnd('var');
  }, 10);
}


console.time('let');
for (var i = 1; i <= count; i++) {
  var letCallCount = 1;
  (function (i) {
    window.setTimeout(function () {
    if(letCallCount++ == count) console.timeEnd('let');
    }, 10);
  })(i);
}

My results:

var: 230.205ms
let: 123.337ms

@jods4
Copy link

jods4 commented Jan 17, 2015

@basarat The code you posted doesn't use let so I think you measured a different code? Also I was suspicious of a benchmark involving setTimeout as that could seriously skew what you really want to measure.

I came up with the following code, that creates a closure on the loop variable in 3 different way: using ES6 let, using an immediate function (what 6to5 does), using try-catch (what Traceur does):

'use strict';

var count = 10000;
var callback;

console.time('let');
for (let i = 0; i < count; i++) {
    callback = function() { return i; };
}
console.timeEnd('let');

console.time('function');
for (var j = 0; j < count; j++) {  
    (function (_j) {
        callback = function() { return _j; };
    })(j);
}
console.timeEnd('function');

console.time('try-catch');
for (var k = 0; k < count; k++) {
    try { throw k; }
    catch (_k) {
        callback = function() { return _k; };
    }    
}
console.timeEnd('try-catch');

Results on my machine, IE11:
let: 2.284ms
function: 3.855ms (x1.6)
try-catch: 249.484ms (x109)

Note: IE11 doesn't handle the latest let spec correctly and the code above is not actually correct in this browser.

The immediate function might be OK, especially if you only inject it wherever code analysis shows that it's required. The traceur approach (try-catch) is really much slower.

@vladima
Copy link
Contributor

vladima commented Jan 18, 2015

Wrapping loop body in IIFE can become a reason of interesting memory issues because of closure creation. An example for V8 can be found here. If runtime supports let then bigObject will definitely be reclaimed after createWithLet is finished. However if let is transformed into IIFE (like it is done in createWithVar) - then BigObject will be hold in memory even after function is completed.

@jods4
Copy link

jods4 commented Jan 18, 2015

I modified your tests so that your array isn't sparse. Basically I used new Array(x).join("*") to create a huge string instead.
In the latest Chrome release I couldn't see any memory leak?

I see what you tried to do. Because bigObject is referenced inside the IIFE closure it should be included inside createWithVar context, which is held alive by callbacks closure.

I don't know why I can't see the leak, maybe the V8 optimized further since 2012? The optimizer could notice that IIFE doesn't escape the function scope and hence doesn't need any closure at all. Or they may notice that the result only needs the IIFE context, not createWithVar's context. In any case I don't see the leak.

Is this a motivation enough to hold off implementation? Anyway if I am required to code this, I will use the IIFE technique by hand or use 6to5, which will do the same...

On the other hand, looking forward, I think that the IIFE transform may have complicated interactions with other features, such as async and generators? For instance, playing with 6to5 REPL, the following code doesn't generate correct ES5 code:

async function test(y) {
  var result = [];
  for (let i = 0; i < 4; i++) {
    await y;
    result.push(() => i);
  }
  return result;
}

@vladima
Copy link
Contributor

vladima commented Jan 18, 2015

Just tried Chrome 42.0.2279.2 (Official Build) canary (64-bit), V8 4.2.10
If you open the page, click on test var and take a memory shapshot - snapshot will contain instance of BigObject being hold by the context.
What I'm trying to say is that I'd prefer to avoid transformations that can introduce subtle issues like this one.

@jods4
Copy link

jods4 commented Jan 18, 2015

Yes, I don't know what I missed before.
I'm on another computer now with Chrome 39 (stable) 32 bits and I see it, too.
Out of curiosity I checked IE11 and it retains bigObject as well.

@RyanCavanaugh RyanCavanaugh added In Discussion Not yet reached consensus and removed Too Complex An issue which adding support for may be too complex for the value it adds labels Jan 22, 2015
@RyanCavanaugh RyanCavanaugh reopened this Jan 22, 2015
@RyanCavanaugh
Copy link
Member

Reopening based on feedback

@Alxandr
Copy link

Alxandr commented Mar 7, 2015

Babel actually handles cases with loops fairly well at this time. It has some quirks where perf could probably be better (though the ones I found a while back has all been fixed).

The following for instance, does not produce any IIFE:

// original
for(let i = 0; i < 10; i++) {
  console.log(`1: ${i}`);
  for (let i = 0; i < 10; i++) {
    console.log(`2: ${i}`);
  }
}

// transpiled
for (var i = 0; i < 10; i++) {
  console.log("1: " + i);
  for (var _i = 0; _i < 10; _i++) {
    console.log("2: " + _i);
  }
}

However, if I do things that requires i to be bound, like creating a function it does:

// original
let arr = [];
for(let i = 0; i < 10; i++) {
  arr.push(() => i);
}

// transpiled
var arr = [];
for (var i = 0; i < 10; i++) {
  (function (i) {
    arr.push(function () {
      return i;
    });
  })(i);
}

I've seen that in most of the cases where I use let, it does not indeed result in IIFEs, but that's obviously not a broad test. It does however have some slight quirks (as far as I can understand it) if you technically only need an IIFE in the inner loop:

// original
let arr = [];
for(let i = 0; i < 10; i++) {
  for (let i = 0; i < 10; i++) {
    arr.push(() => i);
  }
}

// transpiled
var arr = [];
for (var i = 0; i < 10; i++) {
  (function (i) {
    for (var _i = 0; _i < 10; _i++) {
      (function (_i) {
        arr.push(function () {
          return _i;
        });
      })(_i);
    }
  })(i);
}

As far as I can tell, it shouldn't need to create an IIFE on the outer loop here, but that might just be me thinking about things wrong.

@DanielRosenwasser
Copy link
Member

Pretty sure #2161 should close this issue out.

Pretty sure #2161 at least partially addresses this issue.

@jods4
Copy link

jods4 commented Mar 7, 2015

Babel was improved recently and is quite good at transpiling let. The thing that impresses me most is that it handles async/await and yield. Try this:

async function test() {
  for (let i in [0,1,2]) {
    await y(() => i);
  }
}

It is also relatively clever when it decides to create the IIFE. For instance, in this case it needs a function to capture the scope of b:

// This code:
for (var a = 0; a++; a < 10) {
  let b = a + 1;
  y(() => a * b);
}
// Generates:
for (var a = 0; a++; a < 10) {
  (function () {
    var b = a + 1;
    y(function () {
      return a * b;
    });
  })();
}

But if I change a to be declared with let instead of var, it only generates a capture for a. b does not need its own:

// This code:
for (let a = 0; a++; a < 10) {
  let b = a + 1;
  y(() => a * b);
}
// Generates:
for (var a = 0; a++; a < 10) {
  (function (a) {
    var b = a + 1;
    y(function () {
      return a * b;
    });
  })(a);
}

Multiple let in the for get passed as multiple parameters to one function:

// This code:
for (let a = 0, b = 2; a++, b--; b > 0) {
  y(() => a * b);
}
// Generates:
for (var a = 0, b = 2; a++, b--; b > 0) {
  (function (a, b) {
    y(function () {
      return a * b;
    });
  })(a, b);
}

There is a lot of work and special cases in this feature... It makes me wonder: can't you reuse babel code? Babel transformers use the de-facto standard AST that was created by Mozilla. I don't know which one TypeScript uses...

@CyrusNajmabadi
Copy link
Contributor

@jods TypeScript does not use Mozilla's syntax tree model. It uses one more suitable for use in the IDE scenarios that TS cares a lot about. The AST we use is deisgned to work extremely well in the presence of broken code (the normal case in an IDE). It is also designed to support extremely fast incremental updates, allowing edits to be incorporated orders of magnitude faster than normal parsing. Finally, it is designed to have a very small memory footprint, eschewing large values for ints, and storing redundant data on the prototype instead of on the instance.

@jods4
Copy link

jods4 commented Mar 7, 2015

@CyrusNajmabadi To be honest, I was expecting that!

Well, maybe you can take inspiration from the babeljs logic anyway, or even just their test cases.
I was looking at those (their tests) and they support lots of other nasty cases that haven't been mentionned in this thread yet.

E.g. goto labels

var stack = [];

loop1:
for (let j = 0; j < 10; j++) {
  loop2:
  for (let i = 0; i < 10; i++) {
    for (let x = 0; x < 10; x++) {
      stack.push(() => [j, i, x]);
      if (stack.length > 5) continue loop2;
      stack.push(() => [j, i , x]);
    }
  }
}

Or return statements

(function () { 
  for (let i in nums) { 
    fns.push(function () { return i; }); 
    return;
  } 
})(); 

@jlennox
Copy link

jlennox commented Mar 8, 2015

@Alxandr Allowing "let" to always define a self calling function hurts performance too much. See your sample here: http://jsperf.com/let-by-function-in-loop

@Alxandr
Copy link

Alxandr commented Mar 8, 2015

@jlennox Yes. But let shouldn't always create IIFEs. It just has to do that in special cases. And mostly special cases that (at least in my case) aren't used. I never said it should always create IIFEs. Also, the two pieces of code you created in that jsperf result in vastly different results, so compering them is generally meaningless, as it's completely different code.

@hdachev
Copy link

hdachev commented Mar 8, 2015

There's also the option for the compiler to just error out or warn if it fears performance will suffer. Just like jslint's "don't create functions in loops" - it's generally not something you should do when you care about performance.

It'd be great if we had this in ts1.5! let and const are among the biggest es wins, it'd be kinda amazing if we could finally start using them.

@Alxandr
Copy link

Alxandr commented Mar 8, 2015

@hdachev jshint actually tells you to create IIFEs though ^^

http://jshint.com/docs/options/#loopfunc

It's not about perf, it's about the fact that the variables aren't bound to the value, but the function scoped variable, so you get unexpected behavior. let and const fixes this.

@hdachev
Copy link

hdachev commented Mar 8, 2015

Ha! Dunno why I never thought about that, it's kinda obvious.

In any case, my point was that the compiler could output a warning when it has performance concerns about the code it's emitting (not saying I think the occasional IIFE in a loop can realistically become a bottleneck in anyone's application). Much better than not having es5 emit at all.

@jods4
Copy link

jods4 commented Mar 8, 2015

Assuming that IIFE are only emitted when really required, which is inside loop closures. Forget perf for a second: what would you replace them with anyway?

for (let i in [0,1,2])
  y(() => i);

Assume you need to create a piece of code that performs exactly what the piece of code above does. What will you do in ES5? In those cases I use a IIFE anyway, do you have a better solution?

If you have no better solution, then perf doesn't matter at all. IIFE is what will be used to achieve this functionnality in a ES5 browser. Transpiled or coded by hand (I prefer transpiled).

@CyrusNajmabadi
Copy link
Contributor

@jods4 "If you have no better solution, then perf doesn't matter at all" The goal with downlevel emit isn't to support 100% of all use cases people may want to use ES6 for. We want to hit a sweet spot where we support the majority of use cases that people care about most of the time, while also not introducing a large amount of complexity into our compiler.

For example, if doing this helped out a handful of customers, in scenarios where they could just workaround the issue themselves, but it added 50k lines of code, then it probably would not be worth it.

Now, if it turns out this is a really popular feature that many customers are requesting, then that will change our evaluation of how worthwhile it is to do.

@CyrusNajmabadi
Copy link
Contributor

@Alxandr You'll note that TS handles this case as well. The current TS compiler will produce:

for (var i = 0; i < 10; i++) {
    console.log("1: " + i);
    for (var _i = 0; _i < 10; _i++) {
        console.log("2: " + _i);
    }
}

@jods4
Copy link

jods4 commented Mar 9, 2015

@CyrusNajmabadi I don't see this problem as a "ES6 feature". I think of it as a coding requirement.

Sometimes you have to create a lambda for each element of a loop iteration (maybe an array). And I don't know any better way to achieve that in ES5 than using an IIFE in a loop. That's what I've always done, even before ES6 was "available".

But this practice is ugly, not as performant as it could be and may unwillingly capture huge amounts of memory.

ES6 finally brings a perfect fix to this issue, by introducing a way to scope a variable to the loop body. That's a huge win for anyone who has to write such code.

Now I understand your argument about complexity. We can always do TS -> Babel -> ES5 anyway.

@jlennox
Copy link

jlennox commented Mar 9, 2015

@Alxandr You must of misunderstood the purpose of the test. The top case was to demonstrate the expected performance, the second to demonstrate what the actual compiled code performance would of been. They both result in arr == [0, 1, 2, 3, 4, 5, 6, 7, 8, 9].

The compiler may be able to work around using an inline function sometimes, and you could make it give a warning when there would be performance degradation, but this is an unsolvable problem.

  1. I can not predict performance degradation. I would need to learn when and where the compiler would compile to an inline function and when it would not, and then, it would be a best guess and be dependent on compiler versions. I can not have indeterminate results.

  2. It can not predict performance degradation. What if a function is called inside a loop, which returns an non function, that's execute by the loop, and now that anon function now contains a multitude of inline functions from the let compile?

  3. And more importantly, I would never expect over 80% performance loss to result in just a warning, or to even be possible. This is broken mode by default. 80% is a very very large number.

@ivogabe
Copy link
Contributor

ivogabe commented Mar 10, 2015

It's possible to get a huge increase in performance by using a temporary function declared after the loop, like this:

for (var i = 0; i < 100; i++) _a(i);
function _a(i) {
     // for body
}

See http://jsperf.com/downlevel-let-in-loop

@Alxandr
Copy link

Alxandr commented Mar 10, 2015

@jlennox yeah, with the suggestion of @ivogabe you're down to a 33% perf reduction (http://jsperf.com/let-by-function-in-loop/2).

Also, the thing to note though, is that the only other way to achieve the same result, is by writing that code by hand. Which would in turn make perf worse when we get to a point where browsers understand it, because then you'd have hand-written code that calls functions instead of simply looping.

@danquirk
Copy link
Member

danquirk commented Apr 9, 2015

This is now checked in.

@danquirk danquirk closed this as completed Apr 9, 2015
@danquirk danquirk added Fixed A PR has been merged for this issue Committed The team has roadmapped this issue and removed In Discussion Not yet reached consensus labels Apr 9, 2015
@microsoft microsoft locked and limited conversation to collaborators Jun 18, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Committed The team has roadmapped this issue Fixed A PR has been merged for this issue Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests