Is TDZ avoidance still needed?

[robpalme]

When bundling with ESNext output, classes can still get downleveled, due to an intentional performance-workaround called TDZ avoidance. This creates a consequential performance hazard when private fields end up using WeakMaps.

It looks like TDZ avoidance workaround originated in ESBuild as a response to this issue which highlighted a performance bug in Safari. That bug was fixed over two years ago and has long since rolled out to all stable versions of Safari. So it looks like Safari performance is no longer a justification for this feature.

ESBuild still has the avoidTDZ code branches and the attached comment explains it exists due to Safari.

Is it ok to remove this feature now? Or should the comment be updated to describe some other reason?

[evanw]

Part of the reason for doing this is indeed TDZ avoidance. It was originally added to work around a severe bug in Safari, but it sounds like it's still relevant despite that bug being fixed. When the TypeScript compiler's build process was switched to esbuild, it was reported that if esbuild did TDZ avoidance at all levels instead of at the top level, the TypeScript compiler would make their parser 5-9% faster: #297 (comment). So I assume that removing esbuild's TDZ would have very real performance consequences. There is also an argument for doing even more TDZ avoidance to improve performance further.

Another reason that this exists is that esbuild's linker may need to turn a module into a lazily-evaluated one in certain situations such as if it's dynamically imported. To avoid introducing binding overhead between modules when this happens, the linker represents module exports as variables outside of the lazy evaluation closure instead of inside.

So this:

export class Foo {}

turns into this when the file needs to be lazily-evaluated:

var Foo;
var init_a = __esm(() => {
  Foo = class {
  };
});

instead of something to this effect:

var a_exports = {};
var init_a = __esm(() => {
  __export(a_exports, {
    Foo: () => Foo
  });
  class Foo {
  }
});

That way external files can reference Foo directly instead of having to reference a_exports.Foo, which has additional performance overhead.

So exported top-level variables have to be hoisted out of the lazy evaluation closure. This is straightforward for variable declarations: var is trivially hoisted, and let/const are converted to var (with compile-time safety checks that const variables are never reassigned). This is also trivial for function declarations since they are already hoisted and don't evaluate any code by themselves. But hoisting class declarations outside the closure would be incorrect as class declarations cause code evaluation in many ways (the base class expression, computed static fields, static blocks, etc...).

This means classes must be rewritten to store themselves outside of their scope. It's not necessarily trivial to expose the class to the outer scope. You might thing you could just do exported_Foo = Foo after the class body. But JavaScript lets you reassign class declaration symbols (e.g. Foo = decorate(Foo)) and the exported live binding symbol needs to change when that happens. Storing the class outside of the scope is a straightforward way to make this work.

Due to esbuild's parallel and performance-oriented design, esbuild does this transformation at the parsing stage. However, it's not yet known at that point whether or not the current module will need to be lazily-evaluated or not, so esbuild always does this transformation when bundling is active. It turns out this transformation is the same thing as TDZ avoidance.

This transformation could likely be improved. It's something I'll probably get around to doing at some point. I've been tracking several large areas of upcoming work (e.g. tsconfig.json issues, code splitting, plugin API) and the category of class declaration problems is one of these.

[jakebailey]

Just to link back here, I re-tried applying babel's let/const transformation to the TypeScript compiler (which I did many months ago while working on the module conversion, but not on its own), and the results are really stunning: microsoft/TypeScript#52656 (comment)

Back on #297 (comment) I mentioned that I was willing to ignore this problem because my impression was that eventually we'd have ESM and export var could be slower (and we already got a perf bump in most other areas that compensated), but it turns out that esbuild already does emit export var.

I'd really love for engines to get better at this, but at this point in time the performance bump is so great that I am sort of flipping back to wanting to propose a minimal form of this downleveling (i.e. just the easy ones ignoring loops) in esbuild solely for performance (so not the entirety of #297), which is unfortunately the opposite direction @robpalme is looking for.

[jakebailey]

wanting to propose a minimal form of this downleveling

Just to make it clear what this proposal would be, a new option like "--avoid-tdz" would simply replace let/const with var where doing so wouldn't require any additional codegen (i.e. the loop/switch/whatever cases), and potentially, the flag also controls the avoidTDZ flag added for that one Safari case.

For the TypeScript compiler, I would enable this option for all of our outputs except for our test bundle; that combined with type checking would allow us to catch any real TDZ violations while improving performance.

But, I know that this is exactly the opposite of this issue's premise, which is definitely unfortunate.

[evanw]

I'd really love for engines to get better at this, but at this point in time the performance bump is so great that I am sort of flipping back to wanting to propose a minimal form of this downleveling (i.e. just the easy ones ignoring loops) in esbuild solely for performance (so not the entirety of #297), which is unfortunately the opposite direction @robpalme is looking for.

Thanks for following up. Yeah I'd like to do this in esbuild too (to help make tsc faster). I think it'd have to be opt-in instead of applied automatically because it would change semantics around TDZ in nested scopes. Currently esbuild mostly handles TDZ in nested scopes correctly.

If it's just to improve performance, I'm wondering if it would be a better idea to only transform let/const if it's not closed over by a closure in a loop. Then there wouldn't be any need to hoist loop bodies out into a closure, which is the complex transformation that makes this non-trivial. I have a suspicion (but haven't done any testing) that down-leveling let/const that's closed over by a closure in a loop would be slower than just leaving it alone and letting the VM handle it due to the extra function call and scope creation overhead. My suspicion is that this case is pretty rare already and the overhead of adding an additional function call for every loop iteration in these cases is less than the overhead of a few TDZ checks. In any case, skipping let/const => var for complex cases is less work and complexity.

I've been thinking of a new flag --opt:ignore-tdz that would enable the ignore-tdz optimization, which would tell esbuild to pretend that TDZ can't happen. Sort of like undefined behavior in C/C++ I guess. This would enable this let/const => var transformation to improve performance. But it would also allow esbuild to potentially do other things too (maybe only when minifying) such as inlining const constants, which isn't currently done right now due to TDZ correctness. The structure of the --opt: flag is for extensibility, to allow for additional optimizations in the future.

[jakebailey]

Semi-jinx? 😄

Yeah, this is sort of what I was thinking; I'm definitely happy to test something like this out (I was going to try and implement it myself but I'm sure you can do it a lot faster than me).

The reality is that TS before 5.0 already downleveled down to ES5 specifically for this performance win; and I gave it up by switching to esbuild (which still made things better despite this regression), so from my perspective, I'm okay with the "undefined" behavior, especially now that we actually do run tests with TDZ enabled (which caught two bugs: microsoft/TypeScript#50151).

[robpalme]

FYI I've created a bug report for this in upstream V8.

https://bugs.chromium.org/p/v8/issues/detail?id=13723

[ChrisShank]

I've been thinking of a new flag --opt:ignore-tdz that would enable the ignore-tdz optimization, which would tell esbuild to pretend that TDZ can't happen.

To clarify, would this flag prevent the case where export const foo = 1; is transformed to var o=1;export{o as foo}; (playground)? I am trying to use esbuild to bundle a JavaScript library, but this TDZ optimization is preventing dead code elimination when consumers bundle their code for production.

Slightly related to #2695 (comment).

[guilhermesimoes]

At this point in time the performance bump is so great that I am sort of flipping back to wanting to propose a minimal form of this downleveling (i.e. just the easy ones ignoring loops) in esbuild solely for performance

If I may, I'd like to throw support behind this idea.

We at PeacockTV and SkyShowtime care deeply about performance and about how our apps run on older hardware. We need to support all kinds of TVs and set-top boxes, some of which were launched more than 10 years ago. For those, we transpile everything to ES5, but there are some middle-tier devices that can use ES6 (or even ES2018) that are based on older versions of Chromium, and are likely never going to be updated.

While @robpalme's V8 ticket will certainly help us in the future, for those of us who are "stuck in the past" giving us the option to var downlevel would be most valuable.

So here's my support for a --supported=let-and-const:false as suggested by #297 (comment).

[evanw]

This creates a consequential performance hazard when private fields end up using WeakMaps.

FWIW #3167 should resolve this particular issue.

[evanw]

I have written up a FAQ entry about this: https://esbuild.github.io/faq/#top-level-var. I'm closing this issue because a) yes, it's still needed, b) the performance issue regarding private fields has been fixed, and c) it's now described in the documentation.