Skip to content

feat(bundler): implement enum inlining / more constant folding - #12144

Merged
Jarred-Sumner merged 39 commits into
mainfrom
dave/the-enum-pr
Jul 3, 2024
Merged

Jarred-Sumner merged 39 commits into
mainfrom
dave/the-enum-pr

Conversation

@paperclover

@paperclover paperclover commented Jun 25, 2024 •

Copy link
Copy Markdown
Contributor

Initially was a PR to fix an enum bug, but it turned out the bug was due to a lack of a feature: enum inlining. So I decided to do it, since it was a feature I personally have a big use for (For example, the bun.report project has a regex hack workaround to force enum inlining, because the codegen otherwise is an abomination).

edit: As time went, this PR turned into not just that, but a lot of changes to constant folding. i am much happier with how the bundler handles this now.

Fixes #11764
Fixes #2945
Fixes #2889
Fixes #12285
Fixes
Draft TODO:

  • e_inlined_enum
  • inline enum as well as in simplify functions
  • Enum use-before-define behavior
  • Implement required changes in TS namespaces
  • Fix sibling namespace scopes
  • Cross-file enums while bundling
  • Tests
  • Note: late-stage numeric folding (js printer) is not implemented, and will not be for now

extras:

  • constnat folding now happens with all numeric operators
  • constant folding now happens with n + "baa", where n is any constant number. this uses webkit's code to properly adhere to JS spec
  • constant folding now happens with +"1.2". this uses webkit's code to properly adhere to JS spec
  • constant folding requires --minify-syntax, where previously it seemed to always be applied. Though keep in mind the JS runtime transpiles all files with --minify-syntax
  • constant folding is now enabled in the context of require, import, and macro calls; even if minify-syntax is disabled. This was previously not an issue because it was always enabled.
  • string and template string folding is much more advanced now. this may need more tests

Simple Sample

enum A {
  Hello = 1,
  World,
}

console.log(A.Hello);
console.log(A.World);
console.log(A.Hello + A.World);

After:

// index.ts
console.log(1 /* Hello */);
console.log(2 /* World */);
console.log(3);

Before:

// index.ts
var A;
(function(A2) {
  A2[A2["Hello"] = 1] = "Hello";
  A2[A2["World"] = 2] = "World";
})(A || (A = {}));
console.log(A.Hello);
console.log(A.World);
console.log(A.Hello + A.World);

If you reference the enum directly, you can get the two-way map. This even works with const enum, as it is TSC's job to prohibit such usage.

enum A {
  Hello = 1,
  World,
}

console.log(A);

The codegen of these enums have been improved. Projects with heavy enum usage should see smaller bundle sizes. Now:

var A;
((A2) => {
  A2[A2.Hello = 1] = "Hello";
  A2[A2.World = 2] = "World";
})(A ||= {});

Before:

// index.ts
var A;
(function(A2) {
  A2[A2["Hello"] = 1] = "Hello";
  A2[A2["World"] = 2] = "World";
})(A || (A = {}));
console.log(A);

It would be interesting as a future project to see if the size of these enums could be shrunk further, but esbuild doesn't do that, and I think personally it's fine to leave enum in the dust as const enum and inlining all values becomes the new norm.

@github-actions

github-actions Bot commented Jun 25, 2024 •

Copy link
Copy Markdown
Contributor

❌ @Jarred-Sumner, your commit has failing tests :(

🐧💪 1 failing tests Linux AARCH64

  • test/js/deno/crypto/webcrypto.test.ts 1 failing

🐧🖥 1 failing tests Linux x64 baseline

  • test/js/bun/shell/leak.test.ts 1 failing

🪟💻 4 failing tests Windows x64 baseline

  • test/cli/install/registry/bun-install-registry.test.ts 1 failing
  • test/cli/install/registry/bun-install-windowsshim.test.ts code 1
  • test/js/bun/dns/resolve-dns.test.ts SIGKILL
  • test/js/node/child_process/child_process.test.ts 1 failing

🪟💻 3 failing tests Windows x64

  • test/cli/install/registry/bun-install-registry.test.ts SIGKILL
  • test/cli/install/registry/bun-install-windowsshim.test.ts code 1
  • test/js/node/child_process/child_process.test.ts 1 failing

View logs

@paperclover paperclover changed the title feat(bundler): implement enum inlining / ts namespace fixes feat(bundler): implement enum inlining / more constant folding Jun 27, 2024
@robobun

This comment was marked as outdated.

Comment thread src/js_ast.zig
/// by calling out to the APIs in WebKit which are responsible for this operation.
///
/// This can return `null` in wasm builds to avoid linking JSC
pub fn toString(this: Number, allocator: std.mem.Allocator) ?string {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: this is renamed from toStringSafely to toString, not because it is now less safe, but because the original intent of "we are taking a safer approach" does not apply.

Comment thread build.zig

pub fn build(b: *Build) !void {
std.debug.print("zig build v{s}\n", .{builtin.zig_version_string});
std.log.info("zig compiler v{s}", .{builtin.zig_version_string});

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

apologies for doing a ton of other random things in this pr. i fixed the banned.json linter which resulted in needing to apply a ton of small transformations. i did this because i was sick of std.debug.print showing at all in my cmd+shift+f menu. this also resulted in the deletion of some old files, which some did not even past astgen.

additionally, i have been using a client side spell-checker, so it just becomes annoying to see a ton of yellow underlines. maybe i will upstream a ci check to keep spelling somewhat intact (i have been very impressed with the typos cli and lsp, it is very low false positive).

Comment thread src/bun.zig
}
var x = 0; // if you hit an error on this line, you are not in a comptime context
_ = &x;
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comptime @inComptime is a compiler error in 0.14 and in 0.13 does not do what we want.

Comment thread src/feature_flags.zig

pub const react_specific_warnings = true;

pub const log_allocations = false;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this behavior is now provided via BUN_DEBUG_mimalloc=1

@paperclover
paperclover marked this pull request as ready for review June 29, 2024 04:25
Comment thread src/bundler/bundle_v2.zig Outdated
Comment thread src/js_ast.zig Outdated
/// generated proxy symbols that represent the property access "x3.y". This
/// map is unique per namespace block because "x3" is the argument symbol that
/// is specific to that particular namespace block.
property_accesses: std.StringArrayHashMapUnmanaged(Ref) = .{},

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
property_accesses: std.StringArrayHashMapUnmanaged(Ref) = .{},
property_accesses: bun.StringArrayHashMapUnmanaged(Ref) = .{},

@paperclover
paperclover marked this pull request as draft June 29, 2024 05:29
Comment thread src/js_parser.zig
Comment thread src/js_parser.zig
@paperclover

Copy link
Copy Markdown
Contributor Author

two bugs remain

  • utf16 identifiers
  • template rope thing

Comment thread src/js_parser.zig Outdated
@@ -6819,7 +7200,7 @@ fn NewParser_(
}
}

if (comptime !Environment.isRelease) {
if (comptime Environment.allow_assert) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (comptime Environment.allow_assert) {
if (comptime Environment.isDebug) {

Comment thread src/js_parser.zig
@@ -8286,11 +8650,7 @@ fn NewParser_(
else => {
if (comptime get_metadata) {
const find_result = p.findSymbol(logger.Loc.Empty, p.lexer.identifier) catch unreachable;
if (p.known_enum_values.contains(find_result.ref)) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What was this change?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

known_enum_values was removed in favor of real enum inlining.

Comment thread src/js_parser.zig
.loc = ns.name.loc,
},
);
// try p.ref_to_ts_namespace_member.put(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this deliberately commented out?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this second put is seen in esbuild's code but i cannot think of a situation where the value is not already in the map

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

Labels

None yet

Projects

None yet

4 participants