evanw / esbuild

An extremely fast bundler for the web
https://esbuild.github.io/
MIT License
38.1k stars 1.15k forks source link

Uses of a reexported namespace import inconsistently go through exports object #2636

Open jakebailey opened 2 years ago

jakebailey commented 2 years ago

This is something I'm hitting on the TypeScript compiler. In order to emulate our old TS namespaces, I have created modules per TS namespace (which I'll call "namespace barrels") which reexport each file that used to contribute to that namespace. When imported, they behave nearly identically to the old namespaces.

Namespace barrels can reexport other namespace barrels to simulate nested TypeScript namespaces (e.g., in ts.ts, we might have export * as server from "./ts.server"). However, using these reexported ES namespace import objects produces suboptimal bundled code. Uses go through an exports object produced by __export, even though they could be direct.

For example, the output of my repro (minimized for brevity):

// src/_namespaces/ns.sub.js
var ns_sub_exports = {};
__export(ns_sub_exports, {
    callme: () => callme,
});

// src/sub.js
function callme() {}

// entrypoint.js

// import { sub } from "./src/_namespaces/ns";
// sub.callme();
ns_sub_exports.callme();

// import * as sub2 from "./src/_namespaces/ns.sub";
// sub2.callme();
callme();

// import { callme } from "./src/_namespaces/ns.sub";
// callme();
callme();

A workaround is to do direct imports (even if it's importing the namespace barrel directly rather than via a reexport), but this only goes so far (since my conversion method also handles namespace merging cross-project).

Here's a full repo: https://github.com/jakebailey/esbuild-namespace-import-issue

hyrious commented 2 years ago

It happens when you're capturing the namespace via import * as ns, esbuild is not smart enough to destruct this object across multiple files, i.e.

import * as ns from './ns'
ns.func()

// esbuild will treat this file as below so that
// the namespace object is dropped
import { func } from './ns'
func()
// foo.js
export * as bar from './bar'

// main.js
import { bar } from './foo'
bar.func() // <- esbuild cannot destruct this object automatically

Minimal Reproduction REPL

Related: #1737

evanw commented 1 year ago

Unfortunately "devirtualization" of properties on module namespace objects is currently only one level deep. This issue has been previously reported in #1420, although the request there was about tree shaking instead of run-time performance. The workaround so far has been to just avoid doing this if optimal output is desired.

The reason that this isn't trivial to fix has to do with how esbuild's speed-optimized architecture handles cross-module references. Right now accessing a property off of a module namespace object is represented as a special AST node during parsing, which esbuild calls EImportIdentifier. The symbol that this identifier references knows that it represents a property access because of its NamespaceAlias property. This allows the linker to find all of these references without doing another full AST traversal pass, which is avoided for speed. Instead the linker can just scan over the symbol table and merge these special imported symbols with the corresponding exported symbols to implement the devirtualization optimization. The actual AST is never mutated. Instead the substitution of this property access with the corresponding identifier happens at print time, which is again for speed.

This could potentially be implemented without a big performance impact by having the parser represent arbitrarily-long property access chains as symbols. But it would introduce additional complexity to at least the linker, and maybe also other parts of the code too. Maybe it wouldn't be too bad but I'm not sure. Hard to say without actually attempting it.

How much of a problem is this for you? Are you currently blocked by the lack of this optimization? From what I understand, the transition to esbuild has already improved performance. Are you just hoping to get even more of a performance benefit from a feature like this? Also, is the use of re-exported module namespace objects the intended final state of the code base or is it just a useful way to minimize diffs and conversion work during the transition from namespaces to ESM?

jakebailey commented 1 year ago

Answering your questions out of order (since the last piece is the one that's interesting):

Also, is the use of re-exported module namespace objects the intended final state of the code base or is it just a useful way to minimize diffs and conversion work during the transition from namespaces to ESM?

Internally, my intent is to slowly march us toward direct imports, however our public API still needs to look like it did when our package was namespaces, so the idea of these barrels are unlikely to go away on that front.

How much of a problem is this for you? Are you currently blocked by the lack of this optimization?

We aren't blocked by it, no; performance testing of the new module-ified / bundled TS compiler are still positive when the bulk of the calls happen within the bundle. And I can mostly work around things by selectively changing imports to optimize them since I have control over internal imports.

What goes beyond this issue (which is really just about an inconsistency I noticed in emit) is that I've tested the performance overhead of esbuild's __export object, and found that the overhead of accessing exports is consistently 4x slower than TS's old output (plain objects). I've been blocked on some perf tooling to try and investigate this more directly, but this represents a somewhat significant slowdown for users of our API; when using fully qualified accesses like ts.server.something, it compounds.

I actually considered using rollup as it emits better code without any of these objects, but turned out to have other execution ordering problems I couldn't fix. For now, we're ignoring the problem in hopes that people's use of the TS library spends most of its time within the library itself (e.g. gets that 10-25% number I've mentioned before).

I hadn't reported this bit as it's probably along the lines of #1079 and #2448; it's mostly unrelated to this issue, and I can report it separately if that makes more sense, or just comment on those other issues.

jakebailey commented 1 year ago

To be clear, the tooling is for me to use to figure out how to speed up __export; I was able to get a modest improvement just by guessing at a change (declare known const things differently), but the tooling ties into v8 so we can see what is actually getting optimized (or not). Hopefully it'll be out soon.