[Symbol Propagation] Non-deterministic bundle hashes

[gorakong]

↪️ Pull Request

In collaboration with @lettertwo
This PR addresses non-determinism in bundle hashes in the absence of file content changes.
In our test case,

• foo imports bag
• bar imports both baz and bag

We end up with either one of two output bundle contents (and hence hashes) depending on whether foo or bar is transformed first (example below):

💻 Examples

When transformation of foo.js is delayed, baz is ordered before bag since we see baz being used first (in bar.js):

$parcel$export($aca58ea34dfb6a71$exports, "baz", () => $20d363e97eb3a92a$export$eab138534834282b);	
$parcel$export($aca58ea34dfb6a71$exports, "bag", () => $20d363e97eb3a92a$export$35e48f1ff4f2bc86);	

The order of these exports are flipped when we delay bar.js's transformation.

💡Proposed Fix

Keep track of dependencies that have changes to their used symbols and sort their used symbols after symbol propagation.

🚨 Test instructions

The test case implements a custom readFile that proxies overlayFS to delay transformation. This was done (as opposed to deferring) to avoid changing source content of assets being transformed when introducing these delays.

[gorakong]

@mischnic would like to know what you think 🙂

[parcel-benchmark]

Benchmark Results

Kitchen Sink ✅

Timings

Description	Time	Difference
Cold	1.43s	+7.00ms
Cached	306.00ms	-5.00ms

Cold Bundles

No bundle changes detected.

Cached Bundles

No bundle changes detected.

React HackerNews ✅

Timings

Description	Time	Difference
Cold	8.63s	-105.00ms
Cached	424.00ms	-7.00ms

Cold Bundles

No bundle changes detected.

Cached Bundles

No bundle changes detected.

AtlasKit Editor ✅

Timings

Description	Time	Difference
Cold	1.44m	-1.12s
Cached	2.37s	-30.00ms

Cold Bundles

Bundle	Size	Difference	Time	Difference
dist/16.069344b7.js	905.00b	+0.00b	35.22s	+2.98s ⚠️
dist/simpleHasher.46d6f2e5.js	742.00b	+0.00b	1.07m	+28.37s ⚠️
dist/index.html	240.00b	+0.00b	30.70s	+2.26s ⚠️

Cached Bundles

No bundle changes detected.

Three.js ✅

Timings

Description	Time	Difference
Cold	6.31s	+135.00ms
Cached	272.00ms	+15.00ms ⚠️

Cold Bundles

No bundle changes detected.

Cached Bundles

No bundle changes detected.

Click here to view a detailed benchmark overview.

[lettertwo]

The dependency graph for test case currently looks like this:

graph TB;
  index[index.js]
  library[library/index.js]
  foo[library/foo.js]
  bar[library/bar.js]
  utils[utils/index.js]
  bag[utils/bag.js]
  empty[utils/empty.js]

  index-->|"import {foo, bar} from 'library'"|library
  library-->|"export {foo} from './foo'"|foo
  library-->|"export {bar} from './bar'"|bar
  foo-->|"import {bag} from '../utils'"|utils
  bar-->|"import {baz, bag} from '../utils'"|utils
  utils-->|"export * from './bag'"|bag
  utils-->|"export * from './empty'"|empty

Loading

We aren't sure if this is the minimal form or not. Maybe the diamond shape isn't actually necessary?

[mischnic]

I do think the diamond (plus the "open" diamond at the end) is needed. But that linear chain before the diamond (html, index.js) should be unnecessary in theory. The order of outgoing dependencies is fixed for uncached builds, but the order of incoming dependencies depends on the order of transformation.

(And you could also run into this situation if you do a build with only the "right" diamond branch, and then do another cached build with the "left" diamond branch added. The result would also have a different order of incoming dependencies compared to adding them in the reverse order).

So this is just an inherent problem of getIncomingDependencies not having a consistent order across builds. This function is also iterated over in

• the experimental bundler to call internalizeAsyncDependency (which might be fine if the order is irrelevant here)
• the CSS packager to find the @media(...) {} types that a CSS asset should be wrapped with. This could definitely cause differing bundle outputs
• (all other usages just do find , some or every

The only solution I can think of is sorting the dependencies somehow. There is simply no existing/inherent order for incoming dependencies.

[mischnic]

It has to be that new workerfarm which is causing CI to fail. Using the existing one should work better:

diff --git packages/core/integration-tests/test/scope-hoisting.js packages/core/integration-tests/test/scope-hoisting.js
index 3375ac891..7509826fb 100644
--- packages/core/integration-tests/test/scope-hoisting.js
+++ packages/core/integration-tests/test/scope-hoisting.js
@@ -2,7 +2,6 @@ import assert from 'assert';
 import path from 'path';
 import nullthrows from 'nullthrows';
 import {normalizePath} from '@parcel/utils';
-import {createWorkerFarm} from '@parcel/core';
 import {md} from '@parcel/diagnostic';
 import {
   assertBundles,
@@ -18,6 +17,7 @@ import {
   overlayFS,
   run,
   runBundle,
+  workerFarm,
 } from '@parcel/test-utils';

 const bundle = (name, opts = {}) => {
@@ -5535,10 +5535,6 @@ describe('scope hoisting', function () {
   });

   it('produce the same bundle hash regardless of transformation order', async function () {
-    let workerFarm = createWorkerFarm({
-      maxConcurrentWorkers: 0,
-    });
-
     let testDir = path.join(
       __dirname,
       'integration/scope-hoisting/es6/non-deterministic-bundle-hashes',

But that makes the test pass sometimes when commenting out the fix...

[mischnic]

Another thing I noticed: await workerFarm.end(); is missing, e.g. like here

parcel/packages/core/integration-tests/test/cache.js

Lines 3932 to 3938 in a9193ce

	} finally {
	process.versions.pnp = origPnpVersion;
	// $FlowFixMe[prop-missing]
	Module._resolveFilename = origModuleResolveFilename;
	await workerFarm.end();
	}
	});

So having two workerfarms still running and receiving events from each other probably triggers the assertion

[gorakong]

@mischnic doesn't the workerFarm in test-utils use the same createWorkerFarm (from @parcel/core) under the hood? The test case loses its non-determinism without maxConcurrentWorkers set to 0 as it affects the proxied readFile, so I added a workerFarmZeroConcurrent export in test-utils and used that instead. Test is still failing in CI though..