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

Attach internalized orphans and consider types in PRL #8453

Closed
wants to merge 28 commits into from

Conversation

AGawrys
Copy link
Contributor

@AGawrys AGawrys commented Sep 7, 2022

🐰 Pull Request

This PR address experimental bundler issues with CompiledCss extraction enabled.

Problem 1 was that upon internalization of async bundles, we allowed for orphans to be left behind (of type change bundles) and thus were deleted from the graph.

Solution: This PR adds the function attachOrphans which makes sure that any children of an async bundle in the bundleGraph (these will only be non async code split point bundles) are then connected to whatever parent the async bundle was internalized into.

I've also moved the merge bundles of type change step to be after internalization, so that we don't need to remerge any type change bundles after internalization, which could add a lot of extra work.

Problem 2 arose after Issue 1 was fixed. We had been processing type change bundles within the parallel request limit code. This would've been okay, since we only merge back / alter bundles with source bundles. However, when a reused bundle (an async bundle with source bundles) gets merged back, we know it may have children, so we propagate the reused bundle's source bundles to its children and attach it to the new parent. This is because we assumed the children of an async bundle at that point would only be shared bundles.
But that is not the case.

Solution: Instead, upon merging back a reused bundle, its child of a different type must be either connected to the new parent OR its assets merged into the new parent's already existing bundle of that type.

💻 Examples

The Grey Graphs below are the "bare bones" bundleGraph we use in the experimental bundler, which display only bundles loaded in parallel and their children. So all async bundles are all on the same "level" (i.e. connected to root) and their children are parallel or type change bundles, which are connected to what pulls them in.

We maintain sync relationships and async referencing relationships in supplementary graphs which are not shown but the drawings display these relationships.

INTERNALIZATION ORPHAN BUG

First, here is a visual representation of the dependencies each file has.

So, given that setup, we know foo.js should be internalized into index.js. But foo.js has two "children". One is displayed in the graph below as it is a type change and the other is bazz.js which is an async bundle but is referenced by or "pulled in" by foo.

bundleGraphIdeal_POST_STEP1

We need to make sure we patch up these would-be orphans. So, connect them to the parent. In the bazz.js case we change around our supplementary graphs instead (i.e. remove foo async connections and patch them up)

bundleGraphIdeal_POST_INTERNALIZATION

Type bundles must be merged per bundleGroups so this is why I moved STEP MERGE BUNDLES to occur later, so that we don't need to re-merge.
bundleGraphIdeal_POST_MERGE

Finally, assets are placed into bundles.
bundleGraphIdeal_POSTMERGEPOSTASSETPLACEMENT

PARALLEL LIMIT TYPE CHANGE BUG

First, heres a representation of the dependencies each asset has on others

So, given that setup, we see that bar.js and foo.js become their own bundles as they're code split points. (Async relationship). Notice in the graph above, that they share some assets synchronously. Normally, we can create a shared bundle here, which will be a child to both and have a property sourcebundles to reference them. However, in the experimentalbundler, if a bundle requires the total sum of a bundle including it's main Asset Entry (which is foo.js in this case) we simply "reuse" the existing bundle. This creates a hybrid bundle that has a mainEntryAsset, and sourceBundles.

BGIdeal_Step1
The Graph below shows foo.js as a reused bundle. It has a source bundle, bar.js but also remember it is originally referenced by indexcss.js by async relationship.

BGIdeal_StepPlaceAssets
Now, that graph above would be fine, since even though you see two bundles of type css, they're in different bundleGroups. But we have to uphold parallel request limits which has special rules for reused bundles. The current rules are:

  1. Do not fully delete a merged back reused bundle, because it has something originally pulling it in (indexcss.js)
  2. Place the merged back bundle's assets into the source bundles (a.js, b.js, and foo.js are placed into bar.js)
  3. Connect the children to source bundles ( styles.css )
  4. If the child is a shared bundle, update its source bundles. If not, do not.
  5. If the child is a type change bundle, you must either connect it to the new parent or merge it into an existing bundle of that type in that bundleGroup (styles.css asset is duplicated and placed into c.css bundle)

BGIdeal_MergeBack

✔️ PR Todo

  • Added/updated unit tests for this change
  • Tested on large app : Fixed build error
  • Test runtime (on large app)
  • Test runtime without compiledCSS enabled on large app
  • Included links to related issues/PRs
  • Add diagrams
  • Add description
  • Flow
  • Investigate react spectrum failure
  • Add test case for react spectrum failure
  • Update hasEdge with @mattcompiles changes

for (let parentId of parentBundleRootIds) {
if (
!bundleRootGraph.hasEdge(parentId, childId, 2) ||
!bundleRootGraph.hasEdge(parentId, childId, 1)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hasEdge doesn't take an array or object of edge types so this is the current work around 😅 I will update this once thats fixed

@parcel-benchmark
Copy link

parcel-benchmark commented Sep 7, 2022

Benchmark Results

Kitchen Sink ✅

Timings

Description Time Difference
Cold 1.79s -140.00ms 🚀
Cached 413.00ms -22.00ms 🚀

Cold Bundles

Bundle Size Difference Time Difference
dist/legacy/parcel.7cdb0fad.webp 102.94kb +0.00b 106.00ms -224.00ms 🚀
dist/legacy/parcel.7cdb0fad.webp 102.94kb +0.00b 107.00ms -224.00ms 🚀
dist/modern/parcel.7cdb0fad.webp 102.94kb +0.00b 107.00ms -225.00ms 🚀
dist/legacy/index.2c76ad23.js 1.66kb +0.00b 502.00ms -35.00ms 🚀
dist/legacy/index.8aaa89c9.js 1.20kb +0.00b 503.00ms -34.00ms 🚀
dist/modern/index.6be20f01.js 1.13kb +0.00b 502.00ms -35.00ms 🚀
dist/legacy/index.html 826.00b +0.00b 628.00ms -53.00ms 🚀
dist/modern/index.html 749.00b +0.00b 627.00ms -52.00ms 🚀
dist/legacy/index.b8ae99ba.css 94.00b +0.00b 309.00ms -30.00ms 🚀
dist/modern/index.31cedca9.css 94.00b +0.00b 308.00ms -30.00ms 🚀

Cached Bundles

Bundle Size Difference Time Difference
dist/legacy/parcel.7cdb0fad.webp 102.94kb +0.00b 317.00ms +212.00ms ⚠️
dist/legacy/parcel.7cdb0fad.webp 102.94kb +0.00b 318.00ms +212.00ms ⚠️
dist/modern/parcel.7cdb0fad.webp 102.94kb +0.00b 317.00ms +211.00ms ⚠️
dist/legacy/index.html 826.00b +0.00b 696.00ms +37.00ms ⚠️
dist/modern/index.html 749.00b +0.00b 694.00ms +36.00ms ⚠️

React HackerNews ✅

Timings

Description Time Difference
Cold 12.10s +201.00ms
Cached 531.00ms -14.00ms

Cold Bundles

No bundle changes detected.

Cached Bundles

Bundle Size Difference Time Difference
dist/index.js 486.37kb +0.00b 6.32s -358.00ms 🚀
dist/PermalinkedComment.46b19af5.js 4.21kb +0.00b 6.32s -358.00ms 🚀
dist/UserProfile.f8cd7884.js 1.57kb +0.00b 6.32s -358.00ms 🚀
dist/NotFound.960ab92b.js 429.00b +0.00b 6.32s -358.00ms 🚀
dist/logo.c5bb83f1.png 246.00b +0.00b 6.32s -358.00ms 🚀

AtlasKit Editor ✅

Timings

Description Time Difference
Cold 2.16m -1.88s
Cached 2.69s -54.00ms

Cold Bundles

No bundle changes detected.

Cached Bundles

No bundle changes detected.

Three.js ✅

Timings

Description Time Difference
Cold 9.08s -147.00ms
Cached 343.00ms +15.00ms

Cold Bundles

No bundle changes detected.

Cached Bundles

No bundle changes detected.

Click here to view a detailed benchmark overview.

@AGawrys AGawrys marked this pull request as ready for review September 9, 2022 20:39
@AGawrys AGawrys requested a review from mischnic September 9, 2022 20:39
//Do not orphan any bundles in the bundleGraph.... At this point in time,
//A bundle may have a child of type change, that exists in the bundleGraph
//which must be connected to a referencing or parent bundle, which is found in the bundleROOTgraph
//TODO: Get rid of bundleRootGraph and just had one bigger graph with edgetypes :)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a TODO for the future since it'd be a larger refactor

@AGawrys AGawrys requested a review from devongovett September 10, 2022 16:45
@devongovett
Copy link
Member

On the react-spectrum site, I get an error when running with this branch:

@parcel/bundler-experimental: Expected content key 287bfb9caeee50b8 to exist

  Error: Expected content key 287bfb9caeee50b8 to exist
      at nullthrows (/Users/devongovett/Downloads/react-spectrum/node_modules/nullthrows/nullthrows.js:7:15)
      at ContentGraph.getNodeIdByContentKey (/Users/devongovett/Downloads/react-spectrum/node_modules/@parcel/graph/lib/ContentGraph.js:81:38)
      at getReachableBundleRoots (/Users/devongovett/Downloads/react-spectrum/node_modules/@parcel/bundler-experimental/lib/ExperimentalBundler.js:1177:44)
      at createIdealGraph (/Users/devongovett/Downloads/react-spectrum/node_modules/@parcel/bundler-experimental/lib/ExperimentalBundler.js:720:21)

@@ -1163,7 +1230,14 @@ function createIdealGraph(
dependencyBundleGraph.getNodeIdByContentKey(String(mainNodeId)),
]);
}

let mainReachableRoot = reachableRoots.getNodeIdByContentKey(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This section fixes the bug in react-spectrum (missing content key)

When merging type bundles, since I moved the step to later on, we need to patch up the other graphs drawing connections to any children to their new parent

@AGawrys AGawrys removed the request for review from thebriando November 3, 2022 16:05
@AGawrys
Copy link
Contributor Author

AGawrys commented Jun 20, 2023

Closing unless we need this due to bundle groups deleting. (In the case or merging or deleted async bundles)

@AGawrys AGawrys closed this Jun 20, 2023
@mischnic mischnic deleted the agawrys/orphaned-internalize-bug branch November 14, 2023 15:40
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

Successfully merging this pull request may close these issues.

@parcel/bundler-experimental: The expression evaluated to a falsy value
5 participants