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

Add support for inlining constants #1981

Closed
marvinhagemeister opened this issue Feb 2, 2022 · 6 comments
Closed

Add support for inlining constants #1981

marvinhagemeister opened this issue Feb 2, 2022 · 6 comments

Comments

@marvinhagemeister
Copy link

marvinhagemeister commented Feb 2, 2022

With the fantastic addition in #1977 we're very close to switching our build pipeline away from rollup + terser over to esbuild. The speed improvement of esbuild is crazy good! Our current setup takes 4.8s and esbuild only 0.1s 🎉

Being a little obsessed with output size in the Preact team, the generated code by terser remains a little smaller.

esbuild: 4810 B (gzipped)
current: 4677 B (gzipped)

Comparing the generated output of both tools it seems to be mainly caused by terser being able to inline constant variables.

Example input:

const a = 1 << 1;
console.log(a);

esbuild output:

const a=1<<1;console.log(a);

Terser output:

console.log(2);

Here is the full output comparison of the Preact source: https://gist.github.com/marvinhagemeister/41a1b779650d335550eac7f5a825031f

@evanw
Copy link
Owner

evanw commented Feb 2, 2022

General constant propagation/inlining is pretty complex to implement because of temporal dead zone (TDZ) rules and closures. Currently the only form of constant inlining that esbuild does is TypeScript enums, which aren't bound by TDZ rules and which the TypeScript compiler does unconditionally inline even when doing so violates TDZ behavior. I've been using them for constants in my code and it has been working very well including for the bit flag use case. You might not want to convert your source code to TypeScript though, in which case you wouldn't be able to use esbuild's TypeScript enum inlining.

@marvinhagemeister
Copy link
Author

That's a very good point about TDZ! Didn't think of that. I'm wondering if there is a possibility to prove that a variable is not affected by TDZ for a subset of scenarios. Very much agree with you that a general solution sounds very complex and time consuming to implement.

In the generated file by esbuild the constants are at the top of the file and I'd assume that they are therefore not affected by TDZ. They lowered to var but if I'm not mistaken esbuild does lowering last and knows that this is a constant. That's the cases where I could envision constant inlining to be beneficial.

I'll check if there is some way for us to work around that by converting these constants to TypeScript enums without having to convert the project over to TS.

@evanw
Copy link
Owner

evanw commented Feb 3, 2022

That's a very good point about TDZ! Didn't think of that. I'm wondering if there is a possibility to prove that a variable is not affected by TDZ for a subset of scenarios. Very much agree with you that a general solution sounds very complex and time consuming to implement.

Yes, it's possible to special-case inlining for your specific case. But from your larger code sample I assume you are actually looking for something more robust. One tricky case is this:

const a = 1 << 1;
export function foo() {
  console.log(a);
}

It's not generally possible to inline that into this while respecting TDZ rules, at least not if that code is part of a larger project:

export function foo() {
  console.log(2);
}

ES module initialization happens in two phases with an initial binding pass followed by a later evaluation pass in topological order. This means all import/export binding is complete before any evaluation begins, and it's therefore possible to call a function before the code before it is evaluated due to function hoisting:

// entry.js
import './lib.js';
const a = 1 << 1;
export function foo() {
  console.log(a);
}
// lib.js
import { foo } from './entry.js';
foo();

If you run this in node you'll get the following error:

./entry.js:4
  console.log(a);
              ^

ReferenceError: Cannot access 'a' before initialization
    at foo (./entry.js:4:15)
    at ./lib.js:2:1
    at ModuleJob.run (node:internal/modules/esm/module_job:185:25)
    at async Promise.all (index 0)
    at async ESMLoader.import (node:internal/modules/esm/loader:281:24)
    at async loadESM (node:internal/process/esm_loader:88:5)
    at async handleMainPromise (node:internal/modules/run_main:65:12)

So this would need some pretty complex analysis to determine whether this can be inlined or not. What-calls-what gets even more complex with nested function calls and is impossible in the general case since JavaScript is a dynamic language.

I'll check if there is some way for us to work around that by converting these constants to TypeScript enums without having to convert the project over to TS.

IMO this is most natural if everything is TS. But if you want to, you can just put only the enum in TypeScript and then import it in JS. The inlining will still work. The drawback of this is that you'd then be relying on esbuild-specific behavior that other tools likely won't implement. Although that's not too bad because if you wanted to switch away from esbuild, it's only a small set of constants that you'd need to move back to JS.

@evanw
Copy link
Owner

evanw commented Feb 3, 2022

After thinking about it more, I suppose I could add a special case for a series of const declarations at the top of a file with nothing else before it. As long as they are initialized to primitives (or can be constant folded to a primitive) then they could be treated the same way esbuild currently treats TypeScript enums. I don’t think there can be any TDZ concerns in that case.

This optimization would break if you deviate from that pattern but that doesn’t seem too bad because you could just move the constants to a separate file with nothing else in it and add a comment saying not to deviate from the pattern.

Edit: I forgot that import statements are hoisted too. So the file won't be able to have any import statements anywhere for the optimization to apply. Just moving the import statements after the constants isn't enough.

@marvinhagemeister
Copy link
Author

marvinhagemeister commented Feb 3, 2022

Oh, you're very right, the module initialization adds another layer of complexity on top of that!

This optimization would break if you deviate from that pattern but that doesn’t seem too bad because you could just move the constants to a separate file with nothing else in it and add a comment saying not to deviate from the pattern.

That would be the dream scenario as this is exact setup we have. Our codebase contains a dedicated constants.js file which only exports const variables. There are no functions there or anything that would move us into TDZ territory. Agree with you that it's perfectly fine and very reasonable choice to only support this pattern.

@evanw
Copy link
Owner

evanw commented Feb 6, 2022

This has been released in version 0.14.19.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants