Allow parallel type change bundles to be reused by async siblings #9504

devongovett · 2024-01-23T16:21:39Z

This is a test for an "issue" (a potential optimization) in the bundler for discussion and eventual fix. As shown in the asset graph below, there's an entry HTML file, with a parallel dependency on a CSS file and a JS file. The CSS file in turn depends on another common CSS file. The JS file has an asynchronous dependency on another JS file, which also imports the common CSS file.

Currently, this results in common.css being broken out into a separate shared bundle, even though it would already be loaded due to the parallel dependency from the HTML file when async.js loads. In other words, I think it should be considered a reachable ancestor from async.js. This does not happen if all of the files are JS files, so I think the type change is throwing it off somehow. @AGawrys any ideas?

devongovett · 2024-01-24T02:13:44Z

I think the issue here is actually earlier. During the initial traversal, we eagerly create bundles for type changes. This will happen when we visit the dependency between async.js and common.css. However that happens before we have collected ancestry information, so we don't know that common.css will actually already be available so that type change bundle is not necessary.

AGawrys · 2024-01-25T03:14:35Z

Do you think this only happens in situations where the two .css files have this parent-child relationship (styles.css and common.css) ? Because if so, then if we allow the merging step to, it would've merged them in and that could solve our issue in the interim. @devongovett

devongovett · 2024-01-25T05:26:14Z

@AGawrys I pushed a fix which actually simplifies the whole bundling algorithm a bit as well. Basically, instead of considering type changes as bundle roots and eagerly creating bundles during the first pass (which we have to merge together later), we can delay creating type change bundles until the very end when we are placing assets into bundles. This way we already know all of the ancestry information and can produce more optimal output. If a reachable root for an asset is of a different type, we lazily create sibling bundles of that type (one per bundle group) and reuse them if they already exist (similar to shared bundles). For shared bundles we ensure that all assets are of the same type by adding the type to the unique key.

This allows us to remove the type change merging step completely, and rely on shared bundles to handle that. For the most part, the output is the same. However, type change merging previously did not respect the "minBundleSize" setting, and now that we are relying on shared bundles for that I had to update some tests to use that.

One other difference is that type change bundles could previously be shared between different targets unlike other types of bundles. Actually we created two separate bundles in the ideal graph, but they happened to have the same uniqueKey and so they were merged when updating the mutable bundle graph. They no longer have the same unique key (which is now based on the bundle group id), so this doesn't happen anymore. I think this is ok, not sure what benefit that was providing and it was different from every other type of bundle.

Let me know if I missed something that would require type changes to be handled early!

devongovett · 2024-01-25T05:28:37Z

packages/core/integration-tests/test/css-modules.js

-        type: 'css',
-        assets: ['index.module.css'],
+        type: 'js',
+        assets: ['a.module.css', 'b.module.css', 'index.module.css'],
      },


these changes are due to turning on "minBundleSize": 0

devongovett · 2024-01-25T05:29:27Z

packages/core/integration-tests/test/css.js

      },
      {
        type: 'js',
        assets: ['esmodule-helpers.js', 'index.js'],
      },
-      {name: 'Foo.css', type: 'css', assets: ['foo.css']},
      {name: 'entry.css', type: 'css', assets: ['foo.css', 'main.css']},


I think this was actually a similar case to the one I originally described in the description. foo.css doesn't need to be duplicated because it's already loaded in a parallel bundle

devongovett · 2024-01-25T05:29:46Z

packages/core/integration-tests/test/html.js

+      {
+        type: 'css',
+        assets: ['a.module.css'],
+      },
      {
        type: 'html',
        assets: ['searchfield.html'],


above changes are also due to "minBundleSize"

devongovett · 2024-01-25T05:30:11Z

packages/core/integration-tests/test/monorepos.js

@@ -856,7 +876,7 @@ describe('monorepos', function () {
      ),
      'utf8',
    );
-    assert(contents.includes('import "./pkg-b.cjs.css"'));
+    assert(contents.includes('import "./pkg-b.module.css"'));
  });



changes in this file are due to css bundles no longer being reused between different targets, so there is now both a CJS and ESM version

devongovett · 2024-01-25T05:31:36Z

packages/core/core/src/BundleGraph.js

@@ -1137,6 +1137,7 @@ export default class BundleGraph {
        ? firstAsset
        : // Otherwise, find the first asset that belongs to this bundle.
          assets.find(asset => this.bundleHasAsset(bundle, asset)) ||
+          assets.find(a => a.type === bundle.type) ||


fixes a separate bug that was exposed by these changes: resolving a dependency to an asset should try to resolve to an asset of the same type as the bundle it's in. without this, there was a crash in the JS packager that resolved an dependency to a CSS asset.

AGawrys · 2024-02-09T16:52:51Z

What happens in this test case:it.skip('create a new css bundle to maintain one css bundle per bundlegroup constraint' ?

See this comment for more context. It is basically a scenario where merging the type bundles causes over fetching, but not merging results in two css bundles for an entry bundle group.

AGawrys · 2024-02-09T17:23:06Z

Instead of considering type changes as bundle roots and eagerly creating bundles during the first pass (which we have to merge together later), we can delay creating type change bundles until the very end when we are placing assets into bundles. This way we already know all of the ancestry information and can produce more optimal output. If a reachable root for an asset is of a different type, we lazily create sibling bundles of that type (one per bundle group) and reuse them if they already exist (similar to shared bundles). For shared bundles we ensure that all assets are of the same type by adding the type to the unique key.

This makes a lot of sense to me, I always thought we should use ancestry & reachableRoots to determine type change asset placement 🤷

This allows us to remove the type change merging step completely, and rely on shared bundles to handle that. For the most part, the output is the same. However, type change merging previously did not respect the "minBundleSize" setting, and now that we are relying on shared bundles for that I had to update some tests to use that.

I guess what about naming? I think with the previous implementation we let type bundles have a mainEntryAsset, which became part of the name. Not sure if the mainEntryAsset had a function besides naming for these types of bundles though...

One other difference is that type change bundles could previously be shared between different targets unlike other types of bundles. Actually we created two separate bundles in the ideal graph, but they happened to have the same uniqueKey and so they were merged when updating the mutable bundle graph. They no longer have the same unique key (which is now based on the bundle group id), so this doesn't happen anymore. I think this is ok, not sure what benefit that was providing and it was different from every other type of bundle.

That was probably an oversight, I think not sharing between targets generally makes more sense

Only question I'm left with is about the over-fetching scenario I commented about above, but that was never handled properly anyway so I don't mind approving this. ✔️

AGawrys

Once this is merged, I can go back into the contrib docs I wrote for the bundler and update the steps/ test cases. LGTM 👍

devongovett · 2024-02-10T19:07:01Z

@AGawrys

What happens in this test case:it.skip('create a new css bundle to maintain one css bundle per bundlegroup constraint' ?

This is the output:

[
  {
    name: 'entry.js',
    type: 'js',
    assets: [ 'bundle-url.js', 'cacheLoader.js', 'entry.js', 'js-loader.js' ]
  },
  {
    name: 'Foo.4de85387.js',
    type: 'js',
    assets: [ 'esmodule-helpers.js', 'index.js' ]
  },
  {
    name: 'index-sib.361c9295.js',
    type: 'js',
    assets: [ 'index-sib.js' ]
  },
  { name: 'entry.css', type: 'css', assets: [ 'foo.css', 'main.css' ] }
]

So, foo.css is reused from the entry.css bundle rather than having a separate bundle. And index-sib.js was perhaps missing from the expected test result entirely?

Let me know what you think we should do with this one. For now I'll merge this and we can follow up if we want to update that test.

add test

43b5d52

fix: do not consider type changes to be bundle roots

900aaab

devongovett changed the title ~~Add test for bundler issue reusing parallel type change bundles~~ Allow parallel type change bundles to be reused by async siblings Jan 25, 2024

devongovett marked this pull request as ready for review January 25, 2024 05:27

devongovett requested a review from AGawrys January 25, 2024 05:27

github-actions bot deployed to Preview January 25, 2024 05:31 View deployment

cleanup

84f9c9d

devongovett commented Jan 25, 2024

View reviewed changes

github-actions bot requested a deployment to Preview January 25, 2024 05:48 In progress

devongovett added 2 commits January 25, 2024 10:33

sort bundles by type

f92d276

make order not matter in assertBundles

0bf6b0e

github-actions bot deployed to Preview January 26, 2024 03:20 View deployment

AGawrys approved these changes Feb 9, 2024

View reviewed changes

Merge branch 'v2' into bundler-issue

71a5bb7

github-actions bot deployed to Preview February 9, 2024 18:09 View deployment

devongovett merged commit a93af6a into v2 Feb 10, 2024
14 of 16 checks passed

devongovett deleted the bundler-issue branch February 10, 2024 19:07

apiiro-snyk mentioned this pull request Jul 2, 2024

[Snyk] Upgrade parcel from 2.10.0 to 2.12.0 apiiro-snyk/esbuild#1

Open

hashim21223445 mentioned this pull request Aug 9, 2024

[Snyk] Upgrade parcel from 2.10.0 to 2.12.0 hashim21223445/esbuild#13

Merged

Codimow mentioned this pull request Oct 30, 2024

[Snyk] Upgrade parcel from 2.0.0-beta.3 to 2.12.0 Codimow/bun#5

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow parallel type change bundles to be reused by async siblings #9504

Allow parallel type change bundles to be reused by async siblings #9504

devongovett commented Jan 23, 2024

devongovett commented Jan 24, 2024

AGawrys commented Jan 25, 2024

devongovett commented Jan 25, 2024

devongovett Jan 25, 2024

devongovett Jan 25, 2024

devongovett Jan 25, 2024

devongovett Jan 25, 2024

devongovett Jan 25, 2024

AGawrys commented Feb 9, 2024

AGawrys commented Feb 9, 2024

AGawrys left a comment

devongovett commented Feb 10, 2024

Allow parallel type change bundles to be reused by async siblings #9504

Allow parallel type change bundles to be reused by async siblings #9504

Conversation

devongovett commented Jan 23, 2024

devongovett commented Jan 24, 2024

AGawrys commented Jan 25, 2024

devongovett commented Jan 25, 2024

devongovett Jan 25, 2024

Choose a reason for hiding this comment

devongovett Jan 25, 2024

Choose a reason for hiding this comment

devongovett Jan 25, 2024

Choose a reason for hiding this comment

devongovett Jan 25, 2024

Choose a reason for hiding this comment

devongovett Jan 25, 2024

Choose a reason for hiding this comment

AGawrys commented Feb 9, 2024

AGawrys commented Feb 9, 2024

AGawrys left a comment

Choose a reason for hiding this comment

devongovett commented Feb 10, 2024