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

Consider sibling in available assets to younger sibling for parallel deps #8414

Merged
merged 5 commits into from Sep 8, 2022

Conversation

AGawrys
Copy link
Contributor

@AGawrys AGawrys commented Aug 23, 2022

Parallel siblings are guaranteed to have the previous sibling's assets loaded on the page before the younger sibling. So for the following example, b.js will have any assets a loaded & a

<script type="module" src="a.js"></script>
<script type="module" src="b.js"></script>

In the experimental bundler, along with using shared bundles, we also will "reuse" bundles that match

💻 Examples

This is what currently (without a fix) happens. Though, the bundles themselves are correct, we unnecessarily draw edges from b.js to a.js in an attempt to reuse a bundle that is shared between parent and sibling.

bundleGraph_bug

This is how it should look. The way we currently determine asset availability is by propagating down availability from all ancestors -> to all parents -> to child, intersecting away assets as we go. For parallel deps, we union the set of assets loaded by sibling a with sibling b and neglected to add the bundleRoot , or in this case a.js to the set of assets available to b.js

bundleGraph_parallel_fix

🚨 Test instructions

✔️ PR Todo

  • Added/updated unit tests for this change
  • Filled out test instructions (In case there aren't any unit tests)

@AGawrys
Copy link
Contributor Author

AGawrys commented Aug 23, 2022

I added a test case to this, however, I'm not sure how to assert this type of error since both incorrect and correct functionality produce "correct" output. Right now the test case is just a copy

  • Add test case for this, should I assert edges somehow?

Updated : The final bundlegraph differs by a reference edge, but younger siblings should not have to reference older siblings' assets since they're guaranteed to be loaded, so the test case asserts getReferenceBundles

@parcel-benchmark
Copy link

parcel-benchmark commented Aug 23, 2022

Benchmark Results

Kitchen Sink ✅

Timings

Description Time Difference
Cold 1.38s -10.00ms
Cached 310.00ms -19.00ms 🚀

Cold Bundles

No bundle changes detected.

Cached Bundles

No bundle changes detected.

React HackerNews 🚨

Timings

Description Time Difference
Cold FAILED -0.00ms
Cached FAILED -0.00ms

Cold Bundles

No bundles found, this is probably a failed build...

Cached Bundles

No bundles found, this is probably a failed build...

AtlasKit Editor 🚨

Timings

Description Time Difference
Cold FAILED -0.00ms
Cached FAILED -0.00ms

Cold Bundles

No bundles found, this is probably a failed build...

Cached Bundles

No bundles found, this is probably a failed build...

Three.js ✅

Timings

Description Time Difference
Cold 6.22s +27.00ms
Cached 244.00ms -11.00ms

Cold Bundles

No bundle changes detected.

Cached Bundles

No bundle changes detected.

Click here to view a detailed benchmark overview.

@mischnic mischnic merged commit b934741 into v2 Sep 8, 2022
@mischnic mischnic deleted the agawrys/parallel-availability-fix branch September 8, 2022 11:58
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.

None yet

4 participants