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

Non-deterministic module ordering with splitting: true (affects chunk hashes) #1075

Closed
iamakulov opened this issue Mar 28, 2021 · 13 comments
Closed

Comments

@iamakulov
Copy link

I’ve stumbled upon an issue where the order of modules in the bundle in non-deterministic. (As in, I run the same build twice, and esbuild puts some modules in a different place in the bundle.)

With code splitting, this causes the same chunk to have a different hash on every rebuild. Ultimately, this breaks long-term caching.

I was not able to compose a minimally reproducible example, but here’re my findings that might be relevant:

  • Multiple entrypoints. This is reproducible on the Framer codebase. Framer has 10 entry points, but the issue occurs even if you comment out some of them (down to 3-4 active ones).

  • Code splitting. This only happens when splitting: true is set.

  • It’s different modules. Depending on build settings, I was able to reproduce the issue with popmotion, framer-motion, @babel/runtime and react-router. Eg:

    • I build the codebase twice
    • I diff the build directories and find the chunk that caused build hashes to differ
    • I diff that chunk and see that react-router moved 800 lines higher:
      Screen Shot 2021-03-28 at 17 03 57

    However, the module – and the module’s oscillating location – stays the same unless you change entrypoints or externals. So it’s not totally random.

  • sideEffects: false might have an effect? While investigating the issue, I tried to remove the sideEffects: false flag in node_modules/framer-motion/package.json. This produced a different kind of drift – ie, with sideEffects: false, modules were reshuffled one way; but without sideEffects: false, they were reshuffled in a different way.

    However, @babel/runtime does not have the sideEffects: false flag – and it still was moved around in one setup. So the issue is likely about the module order in general, not about a specific package.json flag.

  • Concrete example. Here’s one concrete case from my experiments:

    Frame 11111

    These are import graphs for two consecutive builds with the same codebase and config. The left build was first, the right one was second. For simplicity, these graphs were built only for a single entrypoint (there were 4 or 5 entrypoints in total). (Zoomable graph version.)

    In both builds, all leafs and some intermediate nodes have the same hashes because they are identical. However, there’s a single chunk (highlighted with a red border) that reshuffles a few modules:

    Diff
    # diff build-left/chunk-SAY7ULKW.js build-right/chunk-SYCGVUYW.js
    105,260d104
    < // ../../../node_modules/framer-motion/dist/es/utils/array.js
    < function addUniqueItem(arr, item) {
    <   arr.indexOf(item) === -1 && arr.push(item);
    < }
    < function removeItem(arr, item) {
    <   var index = arr.indexOf(item);
    <   index > -1 && arr.splice(index, 1);
    < }
    < 
    < // ../../../node_modules/framer-motion/dist/es/utils/subscription-manager.js
    < var SubscriptionManager = function() {
    <   function SubscriptionManager2() {
    <     this.subscriptions = [];
    <   }
    <   SubscriptionManager2.prototype.add = function(handler) {
    <     var _this = this;
    <     addUniqueItem(this.subscriptions, handler);
    <     return function() {
    <       return removeItem(_this.subscriptions, handler);
    <     };
    <   };
    <   SubscriptionManager2.prototype.notify = function(a, b, c) {
    <     var numSubscriptions = this.subscriptions.length;
    <     if (!numSubscriptions)
    <       return;
    <     if (numSubscriptions === 1) {
    <       this.subscriptions[0](a, b, c);
    <     } else {
    <       for (var i = 0; i < numSubscriptions; i++) {
    <         var handler = this.subscriptions[i];
    <         handler && handler(a, b, c);
    <       }
    <     }
    <   };
    <   SubscriptionManager2.prototype.getSize = function() {
    <     return this.subscriptions.length;
    <   };
    <   SubscriptionManager2.prototype.clear = function() {
    <     this.subscriptions.length = 0;
    <   };
    <   return SubscriptionManager2;
    < }();
    < 
    < // ../../../node_modules/framer-motion/dist/es/value/index.js
    < var isFloat = function(value) {
    <   return !isNaN(parseFloat(value));
    < };
    < var MotionValue = function() {
    <   function MotionValue2(init) {
    <     var _this = this;
    <     this.timeDelta = 0;
    <     this.lastUpdated = 0;
    <     this.updateSubscribers = new SubscriptionManager();
    <     this.velocityUpdateSubscribers = new SubscriptionManager();
    <     this.renderSubscribers = new SubscriptionManager();
    <     this.canTrackVelocity = false;
    <     this.updateAndNotify = function(v, render) {
    <       if (render === void 0) {
    <         render = true;
    <       }
    <       _this.prev = _this.current;
    <       _this.current = v;
    <       var _a = getFrameData(), delta = _a.delta, timestamp = _a.timestamp;
    <       if (_this.lastUpdated !== timestamp) {
    <         _this.timeDelta = delta;
    <         _this.lastUpdated = timestamp;
    <         es_default.postRender(_this.scheduleVelocityCheck);
    <       }
    <       if (_this.prev !== _this.current) {
    <         _this.updateSubscribers.notify(_this.current);
    <       }
    <       if (_this.velocityUpdateSubscribers.getSize()) {
    <         _this.velocityUpdateSubscribers.notify(_this.getVelocity());
    <       }
    <       if (render) {
    <         _this.renderSubscribers.notify(_this.current);
    <       }
    <     };
    <     this.scheduleVelocityCheck = function() {
    <       return es_default.postRender(_this.velocityCheck);
    <     };
    <     this.velocityCheck = function(_a) {
    <       var timestamp = _a.timestamp;
    <       if (timestamp !== _this.lastUpdated) {
    <         _this.prev = _this.current;
    <         _this.velocityUpdateSubscribers.notify(_this.getVelocity());
    <       }
    <     };
    <     this.hasAnimated = false;
    <     this.prev = this.current = init;
    <     this.canTrackVelocity = isFloat(this.current);
    <   }
    <   MotionValue2.prototype.onChange = function(subscription) {
    <     return this.updateSubscribers.add(subscription);
    <   };
    <   MotionValue2.prototype.clearListeners = function() {
    <     this.updateSubscribers.clear();
    <   };
    <   MotionValue2.prototype.onRenderRequest = function(subscription) {
    <     subscription(this.get());
    <     return this.renderSubscribers.add(subscription);
    <   };
    <   MotionValue2.prototype.attach = function(passiveEffect) {
    <     this.passiveEffect = passiveEffect;
    <   };
    <   MotionValue2.prototype.set = function(v, render) {
    <     if (render === void 0) {
    <       render = true;
    <     }
    <     if (!render || !this.passiveEffect) {
    <       this.updateAndNotify(v, render);
    <     } else {
    <       this.passiveEffect(v, this.updateAndNotify);
    <     }
    <   };
    <   MotionValue2.prototype.get = function() {
    <     return this.current;
    <   };
    <   MotionValue2.prototype.getPrevious = function() {
    <     return this.prev;
    <   };
    <   MotionValue2.prototype.getVelocity = function() {
    <     return this.canTrackVelocity ? velocityPerSecond(parseFloat(this.current) - parseFloat(this.prev), this.timeDelta) : 0;
    <   };
    <   MotionValue2.prototype.start = function(animation) {
    <     var _this = this;
    <     this.stop();
    <     return new Promise(function(resolve) {
    <       _this.hasAnimated = true;
    <       _this.stopAnimation = animation(resolve);
    <     }).then(function() {
    <       return _this.clearAnimation();
    <     });
    <   };
    <   MotionValue2.prototype.stop = function() {
    <     if (this.stopAnimation)
    <       this.stopAnimation();
    <     this.clearAnimation();
    <   };
    <   MotionValue2.prototype.isAnimating = function() {
    <     return !!this.stopAnimation;
    <   };
    <   MotionValue2.prototype.clearAnimation = function() {
    <     this.stopAnimation = null;
    <   };
    <   MotionValue2.prototype.destroy = function() {
    <     this.updateSubscribers.clear();
    <     this.renderSubscribers.clear();
    <     this.stop();
    <   };
    <   return MotionValue2;
    < }();
    < function motionValue(init) {
    <   return new MotionValue(init);
    < }
    < 
    986a831,986
    > // ../../../node_modules/framer-motion/dist/es/utils/array.js
    > function addUniqueItem(arr, item) {
    >   arr.indexOf(item) === -1 && arr.push(item);
    > }
    > function removeItem(arr, item) {
    >   var index = arr.indexOf(item);
    >   index > -1 && arr.splice(index, 1);
    > }
    > 
    > // ../../../node_modules/framer-motion/dist/es/utils/subscription-manager.js
    > var SubscriptionManager = function() {
    >   function SubscriptionManager2() {
    >     this.subscriptions = [];
    >   }
    >   SubscriptionManager2.prototype.add = function(handler) {
    >     var _this = this;
    >     addUniqueItem(this.subscriptions, handler);
    >     return function() {
    >       return removeItem(_this.subscriptions, handler);
    >     };
    >   };
    >   SubscriptionManager2.prototype.notify = function(a, b, c) {
    >     var numSubscriptions = this.subscriptions.length;
    >     if (!numSubscriptions)
    >       return;
    >     if (numSubscriptions === 1) {
    >       this.subscriptions[0](a, b, c);
    >     } else {
    >       for (var i = 0; i < numSubscriptions; i++) {
    >         var handler = this.subscriptions[i];
    >         handler && handler(a, b, c);
    >       }
    >     }
    >   };
    >   SubscriptionManager2.prototype.getSize = function() {
    >     return this.subscriptions.length;
    >   };
    >   SubscriptionManager2.prototype.clear = function() {
    >     this.subscriptions.length = 0;
    >   };
    >   return SubscriptionManager2;
    > }();
    > 
    > // ../../../node_modules/framer-motion/dist/es/value/index.js
    > var isFloat = function(value) {
    >   return !isNaN(parseFloat(value));
    > };
    > var MotionValue = function() {
    >   function MotionValue2(init) {
    >     var _this = this;
    >     this.timeDelta = 0;
    >     this.lastUpdated = 0;
    >     this.updateSubscribers = new SubscriptionManager();
    >     this.velocityUpdateSubscribers = new SubscriptionManager();
    >     this.renderSubscribers = new SubscriptionManager();
    >     this.canTrackVelocity = false;
    >     this.updateAndNotify = function(v, render) {
    >       if (render === void 0) {
    >         render = true;
    >       }
    >       _this.prev = _this.current;
    >       _this.current = v;
    >       var _a = getFrameData(), delta = _a.delta, timestamp = _a.timestamp;
    >       if (_this.lastUpdated !== timestamp) {
    >         _this.timeDelta = delta;
    >         _this.lastUpdated = timestamp;
    >         es_default.postRender(_this.scheduleVelocityCheck);
    >       }
    >       if (_this.prev !== _this.current) {
    >         _this.updateSubscribers.notify(_this.current);
    >       }
    >       if (_this.velocityUpdateSubscribers.getSize()) {
    >         _this.velocityUpdateSubscribers.notify(_this.getVelocity());
    >       }
    >       if (render) {
    >         _this.renderSubscribers.notify(_this.current);
    >       }
    >     };
    >     this.scheduleVelocityCheck = function() {
    >       return es_default.postRender(_this.velocityCheck);
    >     };
    >     this.velocityCheck = function(_a) {
    >       var timestamp = _a.timestamp;
    >       if (timestamp !== _this.lastUpdated) {
    >         _this.prev = _this.current;
    >         _this.velocityUpdateSubscribers.notify(_this.getVelocity());
    >       }
    >     };
    >     this.hasAnimated = false;
    >     this.prev = this.current = init;
    >     this.canTrackVelocity = isFloat(this.current);
    >   }
    >   MotionValue2.prototype.onChange = function(subscription) {
    >     return this.updateSubscribers.add(subscription);
    >   };
    >   MotionValue2.prototype.clearListeners = function() {
    >     this.updateSubscribers.clear();
    >   };
    >   MotionValue2.prototype.onRenderRequest = function(subscription) {
    >     subscription(this.get());
    >     return this.renderSubscribers.add(subscription);
    >   };
    >   MotionValue2.prototype.attach = function(passiveEffect) {
    >     this.passiveEffect = passiveEffect;
    >   };
    >   MotionValue2.prototype.set = function(v, render) {
    >     if (render === void 0) {
    >       render = true;
    >     }
    >     if (!render || !this.passiveEffect) {
    >       this.updateAndNotify(v, render);
    >     } else {
    >       this.passiveEffect(v, this.updateAndNotify);
    >     }
    >   };
    >   MotionValue2.prototype.get = function() {
    >     return this.current;
    >   };
    >   MotionValue2.prototype.getPrevious = function() {
    >     return this.prev;
    >   };
    >   MotionValue2.prototype.getVelocity = function() {
    >     return this.canTrackVelocity ? velocityPerSecond(parseFloat(this.current) - parseFloat(this.prev), this.timeDelta) : 0;
    >   };
    >   MotionValue2.prototype.start = function(animation) {
    >     var _this = this;
    >     this.stop();
    >     return new Promise(function(resolve) {
    >       _this.hasAnimated = true;
    >       _this.stopAnimation = animation(resolve);
    >     }).then(function() {
    >       return _this.clearAnimation();
    >     });
    >   };
    >   MotionValue2.prototype.stop = function() {
    >     if (this.stopAnimation)
    >       this.stopAnimation();
    >     this.clearAnimation();
    >   };
    >   MotionValue2.prototype.isAnimating = function() {
    >     return !!this.stopAnimation;
    >   };
    >   MotionValue2.prototype.clearAnimation = function() {
    >     this.stopAnimation = null;
    >   };
    >   MotionValue2.prototype.destroy = function() {
    >     this.updateSubscribers.clear();
    >     this.renderSubscribers.clear();
    >     this.stop();
    >   };
    >   return MotionValue2;
    > }();
    > function motionValue(init) {
    >   return new MotionValue(init);
    > }
    > 

    This causes other chunks dependent on that chunk to also change their hashes. That’s partly because the import is now different, but partly because a few modules have drifted inside these chunks as well:

    Diffs
    # diff build-left/chunk-EDGD4IK4.js build-right/chunk-G2H6ULYX.js
    5c5
    < } from "./chunk-SAY7ULKW.js";
    ---
    > } from "./chunk-SYCGVUYW.js";
    95,104d94
    < // ../../../node_modules/framer-motion/dist/es/context/MotionConfigContext.js
    < var import_react5 = __toModule(require_react());
    < var MotionConfigContext = (0, import_react5.createContext)({
    <   transformPagePoint: function(p) {
    <     return p;
    <   },
    <   features: [],
    <   isStatic: false
    < });
    < 
    106c96
    < var import_react7 = __toModule(require_react());
    ---
    > var import_react6 = __toModule(require_react());
    109,110c99,100
    < var import_react6 = __toModule(require_react());
    < var MotionContext = (0, import_react6.createContext)({});
    ---
    > var import_react5 = __toModule(require_react());
    > var MotionContext = (0, import_react5.createContext)({});
    112c102
    <   return (0, import_react6.useContext)(MotionContext).visualElement;
    ---
    >   return (0, import_react5.useContext)(MotionContext).visualElement;
    172,173c162,163
    <   var _a = getCurrentTreeVariants(props, (0, import_react7.useContext)(MotionContext)), initial = _a.initial, animate2 = _a.animate;
    <   return (0, import_react7.useMemo)(function() {
    ---
    >   var _a = getCurrentTreeVariants(props, (0, import_react6.useContext)(MotionContext)), initial = _a.initial, animate2 = _a.animate;
    >   return (0, import_react6.useMemo)(function() {
    201c191
    < var import_react8 = __toModule(require_react());
    ---
    > var import_react7 = __toModule(require_react());
    1177c1167
    <     (0, import_react8.useEffect)(function() {
    ---
    >     (0, import_react7.useEffect)(function() {
    1195a1186,1195
    > // ../../../node_modules/framer-motion/dist/es/context/MotionConfigContext.js
    > var import_react8 = __toModule(require_react());
    > var MotionConfigContext = (0, import_react8.createContext)({
    >   transformPagePoint: function(p) {
    >     return p;
    >   },
    >   features: [],
    >   isStatic: false
    > });
    > 
    # diff build-left/chunk-WLTKE4AK.js build-right/chunk-75FV2GDA.j
    5c5
    < } from "./chunk-EDGD4IK4.js";
    ---
    > } from "./chunk-G2H6ULYX.js";
    10c10
    < } from "./chunk-SAY7ULKW.js";
    ---
    > } from "./chunk-SYCGVUYW.js";
    # diff build-left/preview-next-wrapper.46SEYDM5.js build-right/preview-next-wrapper.H3WST3JQ.js
    12,13c12,13
    < } from "./chunk-WLTKE4AK.js";
    < import "./chunk-EDGD4IK4.js";
    ---
    > } from "./chunk-75FV2GDA.js";
    > import "./chunk-G2H6ULYX.js";
    21c21
    < } from "./chunk-SAY7ULKW.js";
    ---
    > } from "./chunk-SYCGVUYW.js";
  • esbuild config. The esbuild config that reproduces the issue on the Framer codebase looks like this:

    build({
         outdir: path.resolve(process.cwd(), OUTPUT_PATH),
         bundle: true,
         format: "esm",
         splitting: true,
         logLevel: "info",
         loader: {
             ".raw.tsx": "text",
             ".tmLanguage": "text",
             ".png": "file",
             ".svg": "file",
             ".ttf": "file",
             ".mp4": "file",
             ".jpg": "file",
         },
         external: [
             // Mock for @framerjs/framer-tracking-client
             "https",
         ],
         define: {
            "process.env.NODE_ENV": JSON.stringify(BUILD_TYPE),
         },
         entryPoints: appEntrypoints,
         entryNames: isProduction ? "[name].[hash]" : "[name].debug",
         tsconfig,
         watch: false,
    })

I’m happy to provide more info if there’s anything that can help to figure out the source of the issue!

@kzc
Copy link
Contributor

kzc commented Mar 28, 2021

Just out of curiosity, is the single-threaded esbuild-wasm determinisitic on your test case?

@evanw
Copy link
Owner

evanw commented Mar 28, 2021

Thanks for the detailed report. However, I can't reproduce this on any codebase I have access to, including the Figma which is usually large enough and complex enough to catch most issues. I assume sharing the Framer codebase is not an option, so this might be tricky to debug. I really wish I had access to more large codebases for testing (especially automated tests).

A start could be to know for sure which commit caused the issue. You can build esbuild locally using these instructions: #969 (comment). I assume you'd need the installForTests method since you're probably using plugins. Then you could do a git bisect to find the earliest commit with the problem.

I appreciate you helping to solve this. The linker has been relatively stable up until recently, but it's currently undergoing a lot of changes due to the rewrite. I'm hoping to get it to a stable state again soon but with the ordering issues you had previously identified fixed.

@iamakulov
Copy link
Author

iamakulov commented Mar 29, 2021

A start could be to know for sure which commit caused the issue.

Looks like this reproduces as far back as in v0.5.16 (where code splitting was introduced) 😕 Symptoms are identical – it’s a few modules being moved up or down, and it’s the same modules (I’ve noticed popmotion and @babel/runtime).

Figuring out if I can provide more information or compose at least some repro.

@iamakulov
Copy link
Author

iamakulov commented Mar 31, 2021

Figuring out if I can provide more information or compose at least some repro.

Hm, would you be open to pair-debugging this remotely on my machine? This is something that Framer folks are definitely open towards. I’m not sure I’ll be able to produce a working repro for this – all my attempts so far were unsuccessful.

If you’re open & have time for this, what’d be the best way to reach out to you to agree on date/time? I’m available this weekend & any day after Apr 12th. I’m located in Europe, but timezone differences should not be an issue.

Here’s how I imagine the setup:

  • From my side, I’ll prepare the dev environment to be able to build esbuild locally.
  • During the debugging session, I’ll give you remote access to my machine for as long as needed.
  • And I’ll also be around to help you reproduce the issue or answer any questions about the Framer codebase.

@kzc
Copy link
Contributor

kzc commented Apr 1, 2021

Have you tried building the non-determinisitic scenario with esbuild run with Go's race detector?

https://golang.org/doc/articles/race_detector

@iamakulov
Copy link
Author

iamakulov commented Apr 1, 2021

Right, thank you! Missed your suggestion 👍

Just out of curiosity, is the single-threaded esbuild-wasm determinisitic on your test case?

Just tested with esbuild-wasm@0.11.2 (the latest version) and esbuild-wasm@0.9.7 (the Framer’s current esbuild version). The issue still reproduces, unfortunately.

Have you tried building the non-determinisitic scenario with esbuild run with Go's race detector?

There’s no sense checking this given that esbuild-wasm is also non-deterministic, right? :/


Just thinking out loud.

  • Is there a chance esbuild uses any container (a HashSet? anything else? not super familiar with Go) that doesn’t always guarantee the order of elements – and that’s what causes the issue?

    For example, in JS, until recently, the order of keys in an object was not guaranteed to be consistent. (It was consistent in most cases, so your code might’ve relied on this, but the spec was not enforcing it.)

  • Or is there a chance esbuild is sorting modules with a sorting function that is not stable? That might explain why some modules oscillate between two locations.

@kzc
Copy link
Contributor

kzc commented Apr 1, 2021

@kzc
Copy link
Contributor

kzc commented Apr 1, 2021

Is the traversal of jsChunks deterministic?

Groan... nevermind:

for key := range jsChunks {
sortedKeys = append(sortedKeys, key)
}
sort.Strings(sortedKeys)

@evanw
Copy link
Owner

evanw commented Apr 2, 2021

Is there a chance esbuild uses any container (a HashSet? anything else? not super familiar with Go) that doesn’t always guarantee the order of elements – and that’s what causes the issue?

Yes! Maps are used all over the place, and map iteration order in Go is deliberately random. In fact Go's guarantees are stronger than this; map iteration order is actively randomized to try to create bugs:

In Go 1, the order in which elements are visited when iterating over a map using a for range statement is defined to be unpredictable, even if the same loop is run multiple times with the same map. Code should not assume that the elements are visited in any particular order.

This change means that code that depends on iteration order is very likely to break early and be fixed long before it becomes a problem. Just as important, it allows the map implementation to ensure better map balancing even when programs are using range loops to select an element from a map.

I'm very aware of this and I always try to use sorted iteration order when relevant. I agree that this is likely the culprit, but finding the spot isn't going to be easy for me because esbuild works fine with everything I run it on (including Figma, which is a huge codebase).

Or is there a chance esbuild is sorting modules with a sorting function that is not stable? That might explain why some modules oscillate between two locations.

Yes, that could also be the culprit. The specific function is sort.Sort() and it's used by esbuild around 10 times. It can be made into a stable sort using sort.Stable(). However, I just reviewed them and nothing stands out to me. In particular, some of the arrays to be sorted are the result of iterating over a map, so if there are duplicate keys then a stable sort wouldn't help (the order it would be preserving is the randomized map iteration order). It could still be worth trying out sort.Stable() instead of course since it's easy to try, but I don't expect that to be the fix.

Looks like this reproduces as far back as in v0.5.16 (where code splitting was introduced) 😕 Symptoms are identical – it’s a few modules being moved up or down, and it’s the same modules (I’ve noticed popmotion and @babel/runtime).

Huh. One one hand that's not great because it's been around so long. But on the other hand that means it's not a recent regression, which means might be somewhat obscure since it's only coming up now.

Hm, would you be open to pair-debugging this remotely on my machine? This is something that Framer folks are definitely open towards. I’m not sure I’ll be able to produce a working repro for this – all my attempts so far were unsuccessful.

If you’re open & have time for this, what’d be the best way to reach out to you to agree on date/time? I’m available this weekend & any day after Apr 12th. I’m located in Europe, but timezone differences should not be an issue.

Thanks for the offer! Offering your time is very generous. That should definitely let us get to the bottom of this. I'd like to try to investigate this more myself first before taking your time but maybe we can do that this weekend sometime if it's still not figured out by then?

@evanw
Copy link
Owner

evanw commented Apr 3, 2021

Any chance the commits above (on master) do anything to fix this? They switch the tie-breaking sort key from the file path to the index of the file in DFS order over all entry points. File paths are supposed to be unique but after a lot of searching, I found one case where there weren't: auto-generated stubs for CSS files that are imported into JS files. These stub files are completely empty so their order shouldn't matter, put it's possible they could still affect the hash.

@iamakulov
Copy link
Author

Any chance the commits above (on master) do anything to fix this?

Nope :/ I’m still seeing drifts in consecutive builds even with esbuild manually built from the latest master.

Just emailed you to see how we can connect.

@evanw
Copy link
Owner

evanw commented Apr 4, 2021

@iamakulov and I just finished a debugging session for this. Thanks very much for helping to debug this with me!

The good news is that it looks like this problem no longer reproduces in version 0.11.5 (the latest version). We believe that this is also related to incorrect handling of sideEffects just like #1081 and #1070 since the workaround for sideEffects issues (#1110) makes this problem go away.

I'm adding some notes about this session below, mostly for me and anyone else who is interested:

For context: during tree shaking, each file keeps track of which entry points that it is reachable from, as well as the minimum "distance" from any entry point (the number of import edges away from an entry point in the import graph). Code splitting chunks are determined by grouping files by the set of entry points that they are reachable from, and then order of files within a chunk is determined by sorting on the minimum distance entry point.

Note that the order within a chunk is different than ESM, which is another problem: #399. I plan to eventually replace all of this code with something entirely different that better matches ESM semantics, which will make this bug irrelevant. But for now this bug still matters while the current legacy code splitting algorithm is still active.

The underlying cause of the non-determinism appears to be that the calculation of distanceFromEntryPoint is incorrect. Specifically, the traversal involves includeFile and includePart (a "part" is a top-level statement in a file, and is the tree shaking granularity in esbuild) and these functions call each other to find all live code until a fixed point is reached. The minimum distanceFromEntryPoint for each file is computed simultaneously with the set of entry points which can reach that file.

The function includeFile updates distanceFromEntryPoint before the fixed point short-circuit check:

// Track the minimum distance to an entry point
if distanceFromEntryPoint < file.distanceFromEntryPoint {
file.distanceFromEntryPoint = distanceFromEntryPoint
}
distanceFromEntryPoint++
// Don't mark this file more than once
if file.entryBits.HasBit(entryPointBit) {
return
}
file.entryBits.SetBit(entryPointBit)

But includePart updates distanceFromEntryPoint after the fixed point short-circuit check, which seems like it could lead to it being incorrect because stopping early means it might not actually be the minimum value (which was the case during the debugging session):

// Don't mark this part more than once
if partMeta.lastEntryBit == ast.MakeIndex32(uint32(entryPointBit)) {
return
}
partMeta.lastEntryBit = ast.MakeIndex32(uint32(entryPointBit))
part := &repr.ast.Parts[partIndex]
// Include the file containing this part
c.includeFile(sourceIndex, entryPointBit, distanceFromEntryPoint)

I suspect this ended up mattering when certain import graph edges aren't traversed due to sideEffects: false. One way to fix this could be to re-traverse the part if the passed-in input distance from the nearest entry point is lower than the recorded one for the current file.

Since this is a fixed-point iteration that computes some minimums and sets, the result should be deterministic regardless of traversal order, so traversal order shouldn't matter. The reason why it mattered was that the computation for the minimum was incorrect. The minimum computation needs to be fixed.

@evanw
Copy link
Owner

evanw commented Apr 11, 2021

I'm closing this as I believe this should now be fixed. In addition to this no longer reproducing in version 0.11.5, I believe the separation of tree shaking and code splitting into two separate passes in version 0.11.7 should ensure that distanceFromEntryPoint is calculated correctly.

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

4 participants
@evanw @iamakulov @kzc and others