Recursively check reachability when removing asset graphs from bundles in deduplication #6004

gorakong · 2021-03-16T23:25:36Z

↪️ Pull Request

Fixes some No reachable bundle found containing... errors that occur in scope-hoisted builds.

Before:
During sibling deduplication, unreachable is first removed from Shared Bundle 2 because it is reachable via its sibling bundle (Shared Bundle 1). common (and its subgraph) is then removed from Shared Bundle 1 because it is reachable via Shared Bundle 2. As a result, unreachable is no longer in any of the bundleGroups Shared Bundle 1 is in (thereby becoming unreachable from Shared Bundle 1).

After:
Recursively check reachability when removing asset graphs from bundles in deduplication.

Will be checking to see if this addresses #5770

✔️ PR Todo

Added integration tests for both scope-hoisted and non-scope-hoisted builds
Assessed performance implications on a large app:

Optimizing (avg time)
Before: 3.62s
After: 13.21s
265% slower

Bundling (avg time)
Before: 19.46s
After: 36.70s
88.6% slower

Overall build time, avg:
Before: 438s
After: 464s
5.9% slower

…e-asset

height · 2021-03-16T23:25:38Z

Link Height tasks by mentioning a task ID in the pull request title or description, commit messages, or comments.

💡Tip: You can also use "Close T-X" to automatically close a task when the pull request is merged.

parcel-benchmark · 2021-03-16T23:54:24Z

Benchmark Results

Kitchen Sink ✅

Timings

Description	Time	Difference
Cold	1.83s	+32.00ms
Cached	391.00ms	-2.00ms

Cold Bundles

Bundle	Size	Difference	Time	Difference
dist/legacy/parcel.7cdb0fad.webp	102.94kb	+0.00b	324.00ms	+26.00ms ⚠️
dist/legacy/parcel.7cdb0fad.webp	102.94kb	+0.00b	326.00ms	+27.00ms ⚠️
dist/modern/parcel.7cdb0fad.webp	102.94kb	+0.00b	325.00ms	+26.00ms ⚠️
dist/legacy/index.2c76ad23.js	1.66kb	+0.00b	539.00ms	+41.00ms ⚠️
dist/legacy/index.8aaa89c9.js	1.20kb	+0.00b	539.00ms	+41.00ms ⚠️
dist/modern/index.6be20f01.js	1.13kb	+0.00b	538.00ms	+40.00ms ⚠️
dist/legacy/index.html	826.00b	+0.00b	687.00ms	+60.00ms ⚠️
dist/modern/index.html	749.00b	+0.00b	685.00ms	+60.00ms ⚠️
dist/legacy/index.b8ae99ba.css	94.00b	+0.00b	334.00ms	+28.00ms ⚠️
dist/modern/index.31cedca9.css	94.00b	+0.00b	333.00ms	+28.00ms ⚠️

Cached Bundles

Bundle	Size	Difference	Time	Difference
dist/legacy/parcel.7cdb0fad.webp	102.94kb	+0.00b	294.00ms	+195.00ms ⚠️
dist/legacy/parcel.7cdb0fad.webp	102.94kb	+0.00b	295.00ms	+195.00ms ⚠️
dist/modern/parcel.7cdb0fad.webp	102.94kb	+0.00b	295.00ms	+195.00ms ⚠️

React HackerNews ✅

Timings

Description	Time	Difference
Cold	11.53s	-17.00ms
Cached	513.00ms	-11.00ms

Cold Bundles

No bundle changes detected.

Cached Bundles

No bundle changes detected.

AtlasKit Editor ✅

Timings

Description	Time	Difference
Cold	2.01m	+1.40s
Cached	2.73s	+155.00ms ⚠️

Cold Bundles

No bundle changes detected.

Cached Bundles

No bundle changes detected.

Three.js ✅

Timings

Description	Time	Difference
Cold	8.62s	-53.00ms
Cached	322.00ms	-13.00ms

Cold Bundles

No bundle changes detected.

Cached Bundles

No bundle changes detected.

Click here to view a detailed benchmark overview.

…e-asset

wbinnssmith · 2021-03-18T21:20:17Z

packages/bundlers/default/src/DefaultBundler.js

@@ -435,7 +438,11 @@ function deduplicate(bundleGraph: MutableBundleGraph) {
          bundle.hasAsset(asset) &&
          bundleGraph.isAssetReachableFromBundle(asset, bundle)
        ) {
-          bundleGraph.removeAssetGraphFromBundle(asset, bundle);
+          if (isSiblingDedupe) {


I don't think we should be treating these differently

these tests fail if we do not treat them differently 😞: https://dev.azure.com/devongovett/devongovett/_build/results?buildId=15550&view=logs&j=81a0a40d-e1c6-55f1-066f-a450d17357d3&t=9ab6c9af-8c9a-51e6-60c5-00f9d8182a4a&l=1404

That makes sense — they're no longer reachable because the context changes across those sibling bundles. Let's get #5398 landed, which includes support for multiple contexts per environment [0], which will allow these cross-context shared bundles to have both contexts.

Removal should always be implemented in one way though.

cc @devongovett

[0] https://github.com/parcel-bundler/parcel/pull/5398/files#diff-b2e31de6fe476821294b35d7fb17106cf1fc48b37ebe5800448dcb3608348b6cR774

…e-asset

mischnic · 2022-12-21T21:41:49Z

The test cases pass with the current v2 branch. Should we add the test cases (without the outdated fix) though? @gorakong @AGawrys @devongovett

…reachable-asset

gorakong · 2022-12-22T00:41:32Z

@mischnic sounds good to me! i've updated the PR to only include the test cases.

mischnic · 2022-12-22T10:37:20Z

Looks like you removed too much, these actual tests are missing now:

diff --git packages/core/integration-tests/test/javascript.js packages/core/integration-tests/test/javascript.js
index bceb92c15..22b92cf59 100644
--- packages/core/integration-tests/test/javascript.js
+++ packages/core/integration-tests/test/javascript.js
@@ -6242,6 +6215,23 @@ describe('javascript', function () {
     assert.deepEqual(o2, ['UIIcon', 'Icon']);
   });
 
+  it('should not deduplicate an asset if it will become unreachable', async function () {
+    let b = await bundle(
+      path.join(
+        __dirname,
+        'integration/sibling-deduplicate-unreachable/index.js',
+      ),
+      {
+        mode: 'production',
+        defaultTargetOptions: {
+          shouldScopeHoist: false,
+        },
+      },
+    );
+    let res = await run(b);
+    assert.equal(await res.default, 'target');
+  });
+
   for (let shouldScopeHoist of [false, true]) {
     let options = {
       defaultTargetOptions: {
diff --git packages/core/integration-tests/test/scope-hoisting.js packages/core/integration-tests/test/scope-hoisting.js
index 985d5b672..0c22614ba 100644
--- packages/core/integration-tests/test/scope-hoisting.js
+++ packages/core/integration-tests/test/scope-hoisting.js
@@ -5586,4 +5586,16 @@ describe('scope hoisting', function () {
       await workerFarm.end();
     }
   });
+
+  it('should not deduplicate an asset if it will become unreachable', async function() {
+    let b = await bundle(
+      path.join(
+        __dirname,
+        'integration/sibling-deduplicate-unreachable/index.js',
+      ),
+      {mode: 'production'},
+    );
+    let res = await run(b);
+    assert.equal(res, 'target');
+  });
 });

…e-asset

gorakong · 2023-01-02T23:21:51Z

@mischnic oops good catch! i've added them back 😅

* upstream/v2: (33 commits) v2.8.3 Changelog for v2.8.3 Address bug by updating an asset reference and merge conditions (parcel-bundler#8762) Fix CSS order when merging type change bundles (parcel-bundler#8766) fixing failing build for contributors on Linux using Node 18 (parcel-bundler#8763) Extension: Importers View and separate LSP protocol package (parcel-bundler#8747) Bump swc to fix sourcemaps with Windows line endings (parcel-bundler#8756) Apply HMR updates in topological order (parcel-bundler#8752) Make extension packaging work (parcel-bundler#8730) Typed api.storeResult (parcel-bundler#8732) Refactor LSP to use vscode-jsonrpc (parcel-bundler#8728) Bump swc (parcel-bundler#8742) Recursively check reachability when removing asset graphs from bundles in deduplication (parcel-bundler#6004) Fix tsc sourcemaps metadata (parcel-bundler#8734) Assigning to `this` in CommonJS (parcel-bundler#8737) Don't retarget dependencies if a symbol is imported multiple times with different local names (parcel-bundler#8738) Add a note about using flow in CONTRIBUTING.md (parcel-bundler#8731) filter out title execArgv to workers (parcel-bundler#8719) Document more of the BundleGraph class (parcel-bundler#8711) Fixed the hmr connection with host 0.0.0.0 (parcel-bundler#7357) ...

gorakong added 7 commits March 12, 2021 17:12

unreachable asset problem reproduction

046897a

integration test for sibling deduplication (unreachable asset)

b1fc1d4

added fix for sibling dedupe

cc66306

test passes

c4398f6

add test for non-scope-hoisting

a98551b

revert changes to lockfile

1c6f613

Merge branch 'v2' of github.com:parcel-bundler/parcel into unreachabl…

eeef08a

…e-asset

Merge branch 'v2' of github.com:parcel-bundler/parcel into unreachabl…

9774f40

…e-asset

gorakong changed the title ~~Do not deduplicate an asset if it will become unreachable~~ Recursively check reachability when removing asset graphs from bundles in deduplication Mar 18, 2021

gorakong marked this pull request as ready for review March 18, 2021 21:12

gorakong requested review from wbinnssmith, devongovett and mischnic March 18, 2021 21:12

wbinnssmith reviewed Mar 18, 2021

View reviewed changes

gorakong force-pushed the unreachable-asset branch from f28c04b to 9774f40 Compare March 18, 2021 21:52

Merge branch 'v2' of github.com:parcel-bundler/parcel into unreachabl…

aa4b121

…e-asset

gorakong added 2 commits December 21, 2022 15:37

Merge branch 'v2' of https://github.com/parcel-bundler/parcel into un…

fa88104

…reachable-asset

format

feec73e

gorakong added 2 commits January 2, 2023 15:17

Merge branch 'v2' of github.com:parcel-bundler/parcel into unreachabl…

4e8f253

…e-asset

add tests

0086dcd

Merge branch 'v2' into unreachable-asset

e40ac1c

mischnic approved these changes Jan 3, 2023

View reviewed changes

mischnic merged commit b82f592 into v2 Jan 3, 2023

mischnic deleted the unreachable-asset branch January 3, 2023 21:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Recursively check reachability when removing asset graphs from bundles in deduplication #6004

Recursively check reachability when removing asset graphs from bundles in deduplication #6004

gorakong commented Mar 16, 2021 •

edited

Loading

height bot commented Mar 16, 2021

parcel-benchmark commented Mar 16, 2021 •

edited

Loading

Timings

Cold Bundles

Cached Bundles

Timings

Cold Bundles

Cached Bundles

Timings

Cold Bundles

Cached Bundles

Timings

Cold Bundles

Cached Bundles

wbinnssmith Mar 18, 2021

gorakong Mar 18, 2021

wbinnssmith Mar 18, 2021 •

edited

Loading

mischnic commented Dec 21, 2022 •

edited

Loading

gorakong commented Dec 22, 2022

mischnic commented Dec 22, 2022

gorakong commented Jan 2, 2023

Recursively check reachability when removing asset graphs from bundles in deduplication #6004

Recursively check reachability when removing asset graphs from bundles in deduplication #6004

Conversation

gorakong commented Mar 16, 2021 • edited Loading

↪️ Pull Request

✔️ PR Todo

height bot commented Mar 16, 2021

parcel-benchmark commented Mar 16, 2021 • edited Loading

Benchmark Results

Timings

Cold Bundles

Cached Bundles

Timings

Cold Bundles

Cached Bundles

Timings

Cold Bundles

Cached Bundles

Timings

Cold Bundles

Cached Bundles

wbinnssmith Mar 18, 2021

Choose a reason for hiding this comment

gorakong Mar 18, 2021

Choose a reason for hiding this comment

wbinnssmith Mar 18, 2021 • edited Loading

Choose a reason for hiding this comment

mischnic commented Dec 21, 2022 • edited Loading

gorakong commented Dec 22, 2022

mischnic commented Dec 22, 2022

gorakong commented Jan 2, 2023

gorakong commented Mar 16, 2021 •

edited

Loading

parcel-benchmark commented Mar 16, 2021 •

edited

Loading

wbinnssmith Mar 18, 2021 •

edited

Loading

mischnic commented Dec 21, 2022 •

edited

Loading