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

Safari deopt with a large ESBuild bundle #478

Closed
iamakulov opened this issue Oct 20, 2020 · 11 comments
Closed

Safari deopt with a large ESBuild bundle #478

iamakulov opened this issue Oct 20, 2020 · 11 comments

Comments

@iamakulov
Copy link

@iamakulov iamakulov commented Oct 20, 2020

This issue is JFYI. It feels more like a Safari issue than a ESBuild one, but maybe you or someone else (a fellow WebKit engineer?) will find it useful.

AFAIK @koenbok already briefly mentioned this issue in a DM to you a month ago.

Problem

We at Framer stumbled upon a weird Safari deoptimization. If we bundle all our code into a single huge bundle, Safari suddenly becomes 6-8x slower at executing that bundle.

(We have to build our code as a single huge bundle due to #399. We're only using ESBuild for local development right now, so the bundle size is not an issue.)

Here's how bundle execution looks when the issue reproduces:

The issue reproduces The issue doesn’t reproduce
The bundle takes 8 seconds to execute after it loads:
The bundle takes 1.1 seconds to execute after it loads:

If you look deeper into the bundle execution trace, there is no single item that takes 7 extra seconds. Instead, it feels like every function suddenly becomes several times slower. Compare “Samples” and “Total time” for identical rows:

The issue reproduces The issue doesn’t reproduce
CleanShot 2020-10-21 at 01 26 46@2x CleanShot 2020-10-21 at 01 26 49@2x

Repro

I didn't manage to find a minimal example that would reproduce this issue :( Still, the issue reliably happens with our codebase.

The issue reproduces with the following ESBuild config:

build({
    // This gives ~7 entry points
    entryPoints: fs
        .readdirSync(path.resolve(__dirname, "entrypoints"))
        .map(fileName => path.resolve(__dirname, "entrypoints", fileName)),
    outdir: WATCH_OUTPUT_PATH,
    bundle: true,
    target: "es2019",
    loader: {
        ".raw.tsx": "text",
        ".tmLanguage": "text",
        ".png": "file",
        ".svg": "file",
        ".css": "text",
    },
    tsconfig: path.resolve(__dirname, "tsconfig.esbuild.json"),
    external: [
        "https",
    ],
    sourcemap: true,
    define: {
        "process.env.PLATFORM": JSON.stringify(PLATFORM),
        "process.env.BUILD_TYPE": JSON.stringify(BUILD_TYPE),
        "process.env.NODE_ENV": JSON.stringify(BUILD_TYPE),
        "process.env.BUILD_NAME": JSON.stringify("vekter"),
        "process.env.BUILD_HASH": JSON.stringify(GIT_HASH),
        "process.env.VERSION": JSON.stringify(isProduction ? GIT_HASH : `${GIT_HASH}-dev`),
        "process.env.EXPERIMENTS": `"${escape(JSON.stringify(EXPERIMENTS))}"`,
        "process.env.DEBUG_HYDRATION": JSON.stringify(DEBUG_HYDRATION),
        "process.env.PROMISE_QUEUE_COVERAGE": "false",
        "require.include": "undefined",
        "require.resolveWeak": "undefined",
        "process.platform": "'linux'",
        global: "window",
    },
})

The issue occurs in Safari 12 and 14 (all versions I tested in). Enabling minification doesn’t help.

The issue doesn't reproduce:

  • if the bundle was built with --format=esm but loaded using a regular <script>. (this setup simply removes an IIFE wrapper around the bundle code)
    • this is the workaround we went with
    • but: if the --format=esm bundle is loaded using a <script type="module">, the issue will reproduce
  • if you load the page and then refresh it once (sometimes twice)
    • I guess some Safari optimizations kick in and use information from previous runs to speed up the execution?
    • but if you refresh the page with DevTools → Timeline open, the issue will reproduce
      • I guess the Timeline tab disables these Safari optimizations
  • if the bundle was built using webpack
    • I suppose webpack structures the bundle a bit differently, and that prevents the deopt from occurring
  • if you enable code splitting (despite #399)
    • apparently, splitting the whole bundle across tens of files fixes the deopt

Possible explanation

My only hypothesis (which might be totally off) is that this is related to the number of top-level definitions:

  • The bundle has 13K top-level definitions (variables, constants, functions, and classes)

  • The issue occurs when these 13K definitions are wrapped into a) an IIFE or b) an ES module. But it doesn’t occur if these 13K definitions live at the top level of the plain <script> tag (and, consequently, become available globally)

  • Perhaps the way definitions inside an IIFE/a ES module are stored is different from the way all global definitions are stored?

    If the former approach is not optimized for storing 13K items, this might increase the lookup time for every name in the bundle – and explain why every function suddenly gets slower.

Chrome

For the sake of completeness: in Chrome (which doesn’t have this issue), the bundle always takes ~1s to execute:

CleanShot 2020-10-20 at 21 38 39@2x

@evanw
Copy link
Owner

@evanw evanw commented Oct 20, 2020

Wow, that is crazy. Thanks for this very thorough experience report. I think your suspicion about JIT issues with a large number of local variables is correct. This sure is unfortunate.

I think I can reproduce this behavior on Figma's code base too. I've never noticed because we use Chrome pretty much exclusively for development.

I'm assuming that when you're talking about a Webpack build it's one with scope hosting (a.k.a. module concatenation) turned off. Is that right? Does the problem reproduce with Webpack as well if you turn on scope hosting/module concatentation?

I was able to speed things up dramatically in Safari for my test case by disabling scope hosting in esbuild. There's no setting for this so I did this instead:

diff --git a/internal/bundler/linker.go b/internal/bundler/linker.go
index 8540aa96..ed90ff18 100644
--- a/internal/bundler/linker.go
+++ b/internal/bundler/linker.go
@@ -384,8 +384,7 @@ func newLinkerContext(
 
                        // Also associate some default metadata with the file
                        repr.meta = fileMeta{
-                               cjsStyleExports: repr.ast.HasCommonJSFeatures() || (repr.ast.HasLazyExport && (c.options.Mode == config.ModePassThrough ||
-                                       (c.options.Mode == config.ModeConvertFormat && !c.options.OutputFormat.KeepES6ImportExportSyntax()))),
+                               cjsStyleExports:          sourceIndex != runtime.SourceIndex,
                                partMeta:                 make([]partMeta, len(repr.ast.Parts)),
                                resolvedExports:          resolvedExports,
                                isProbablyTypeScriptType: make(map[js_ast.Ref]bool),

Basically that just treats every file as a CommonJS file, which wraps it in a closure. Can you try that patch on your machine? What happens then?

By the way, I would not recommend shipping a build where module-level variables are global without a wrapper (the esm without module approach). Probably the most insidious bug I've ever had to deal with was one where a minified variable in our code collided with the global ga Google analytics variable and caused a rare timing-dependent loading failure. This happened because emscripten does not use a closure by default. I think that took at least a week to narrow down.

@guybedford
Copy link
Contributor

@guybedford guybedford commented Oct 20, 2020

This is a fascinating case. I wonder if @Constellation might have some insight here into the webkit scope handling slowdown.

Probably the most insidious bug I've ever had to deal with was one where a minified variable in our code collided with the global ga Google analytics variable and caused a rare timing-dependent loading failure.

I disagree quite strongly with this advice. If the minified code contained a ga reference, the minifier is responsible for ensuring global refs are retained. That would make it a minifier bug, which are insidious however they happen.

That is of course assuming the script was in strict mode.

@evanw
Copy link
Owner

@evanw evanw commented Oct 20, 2020

These might be relevant:

I disagree quite strongly with this advice. If the minified code contained a ga reference, the minifier is responsible for ensuring global refs are retained. That would make it a minifier bug, which are insidious however they happen.

We load multiple scripts in one page. The Google analytics tracker was a separate script.

@guybedford
Copy link
Contributor

@guybedford guybedford commented Oct 20, 2020

We load multiple scripts in one page. The Google analytics tracker was a separate script.

Because ES modules are strict by default, var ga at the top scope is not a global declaration. So I'm just not sure how a conflict could possibly happen without being a minifier bug.

@evanw
Copy link
Owner

@evanw evanw commented Oct 20, 2020

I'm pretty sure that using var instead of let/const speeds up Figma in Safari quite a bit. Here's what I used to test it (only changes top-level ones to avoid most semantic mismatches):

diff --git a/internal/js_printer/js_printer.go b/internal/js_printer/js_printer.go
index 41b29ec1..9dfb73db 100644
--- a/internal/js_printer/js_printer.go
+++ b/internal/js_printer/js_printer.go
@@ -3048,6 +3048,9 @@ func Print(tree js_ast.AST, symbols js_ast.SymbolMap, r renamer.Renamer, options
 
        for _, part := range tree.Parts {
                for _, stmt := range part.Stmts {
+                       if local, ok := stmt.Data.(*js_ast.SLocal); ok {
+                               local.Kind = js_ast.LocalVar
+                       }
                        p.printStmt(stmt)
                        p.printSemicolonIfNeeded()
                }

This change speeds up my test case from ~2200ms to ~650ms. The CommonJS transform is still faster at 350ms. Potentially because we use a lot of classes which still need TDZ handling when they are top-level?

Edit: I confirmed that replacing class Foo with var Foo = class fixes that issue too, so the loading time is now ~220ms. Literally 10x faster.

Because ES modules are strict by default, var ga at the top scope is not a global declaration. So I'm just not sure how a conflict could possibly happen without being a minifier bug.

The solution described above configures the bundler to target a module, but then loads it as a script in the browser instead of loading it as a module. That way all of the module-level variables are global.

@evanw
Copy link
Owner

@evanw evanw commented Oct 21, 2020

I added a flag called --avoid-tdz to do the transformation I described above. Details are in the release notes. It's not enabled by default because it changes the semantics of the code, although I suspect the semantic change is probably fine for many real-world projects. I plan to release it later tonight.

@saambarati
Copy link

@saambarati saambarati commented Oct 21, 2020

Do you have a full example of a reproducing test case? Would like to look at improving this in JSC.

@evanw
Copy link
Owner

@evanw evanw commented Oct 21, 2020

Do you have a full example of a reproducing test case? Would like to look at improving this in JSC.

@saambarati that's great! Thanks so much for offering to help. I'd love to help get this fixed. I do have a way to reproduce this, and I can send it to you. What's the best way to reach you? Maybe I can email it to you?

@othermaciej
Copy link

@othermaciej othermaciej commented Oct 21, 2020

@evanw If it's able to be shared publicly, then linking it from a bug report on bugs.webkit.org would be best. Failing that, getting it to @saambarati in a private way would be good.

@evanw
Copy link
Owner

@evanw evanw commented Oct 21, 2020

Sure. It should be ok to share publicly once I clean it up a bit. I'll work on that.

@evanw
Copy link
Owner

@evanw evanw commented Oct 21, 2020

Ok I just posted the repro to https://bugs.webkit.org/show_bug.cgi?id=199866. Really I hope that helps. Please let me know if there's anything else I can do to help.

Also: Hello HN and Reddit! Just wanted to say that JavaScriptCore is an engineering marvel and I have deep respect for everyone on that team. All software has bugs and I can confidently say from my experience writing esbuild that JavaScript is extremely messy to implement with an unbelievable number of edge cases. V8 has also had crazy performance cliffs so something like this is not unusual, and doesn't say anything about JSC vs. V8. Let's not turn this into a meme. It's awesome that people from JavaScriptCore are reaching out and helping get this fixed. Props to the JSC team.

erwinmombay added a commit to erwinmombay/amphtml that referenced this issue Oct 21, 2020
A discussion in evanw/esbuild#478 has shown
that using let and class in same enclosing level has a signigicant
performance impact on the Safari browser. To alleviate this problem we
transform the let and class constructs into var when it is 'easily'
possible.

Co-authored-by: Justin Ridgewell <jridgewell@google.com>
Co-authored-by: erwin mombay <erwinm@google.com>
erwinmombay added a commit to ampproject/amphtml that referenced this issue Oct 22, 2020
* add babel-plugin-transform-block-scoping

A discussion in evanw/esbuild#478 has shown
that using let and class in same enclosing level has a signigicant
performance impact on the Safari browser. To alleviate this problem we
transform the let and class constructs into var when it is 'easily'
possible.

Co-authored-by: Justin Ridgewell <jridgewell@google.com>
Co-authored-by: erwin mombay <erwinm@google.com>

* turn on mangle on terser

* Comments and test cases

Co-authored-by: Justin Ridgewell <jridgewell@google.com>
ed-bird added a commit to ed-bird/amphtml that referenced this issue Dec 10, 2020
* add babel-plugin-transform-block-scoping

A discussion in evanw/esbuild#478 has shown
that using let and class in same enclosing level has a signigicant
performance impact on the Safari browser. To alleviate this problem we
transform the let and class constructs into var when it is 'easily'
possible.

Co-authored-by: Justin Ridgewell <jridgewell@google.com>
Co-authored-by: erwin mombay <erwinm@google.com>

* turn on mangle on terser

* Comments and test cases

Co-authored-by: Justin Ridgewell <jridgewell@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants