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.

Sign up for GitHub

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

Jump to bottom

Update without bundling for a 'simple' non-dependency related change #6514

Merged
merged 17 commits into from
Oct 29, 2022
Merged

Update without bundling for a 'simple' non-dependency related change #6514

merged 17 commits into from
Oct 29, 2022

Conversation

joeyslater
Copy link
Contributor

@joeyslater joeyslater commented Jun 25, 2021 •
edited by AGawrys
Loading

↪️ Pull Request

[UPDATE from @AGawrys ] This PR now makes the default behavior to skip bundles on "simple" cases. To opt-out, users will need to specify --no-incremental or need to set NO_INCREMENTAL=1 for tests.

This is automatically disabled for production builds, config changes, and dependency related bundling scenarios.

This the most basic example of incrementally bundling, adding something like:

  • add a comment
  • update a string or number value
  • add a console log
  • updating a namer
  • multiple files adding a comment (like a patch or stopping Parcel, making changes, then starting again)

The behavior now looks into whether the update should force a full re-bundle or re-use the existing bundle graph.

When to do a full re-bundle

  • if a dependency is added or removed
  • if a symbol is no longer used
  • if bundler config has updated
  • if a new bundler has been introduced

💻 Examples

🚨 Test instructions

  • Make sure all integration tests pass with the flag on and off.
  • Add new test integrations to start the incremental bundling suite for when it should bundle and when it shouldn't

✔️ PR Todo

  • Added/updated unit tests for this change
  • Filled out test instructions (In case there aren't any unit tests)
  • Included links to related issues/PRs
  • Make opt out @AGawrys + test

@height
Copy link

height bot commented Jun 25, 2021

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.

@joeyslater joeyslater changed the title Jslater/non dependency increments Update without bundling for a 'simple' non-dependency related change Jun 25, 2021
@joeyslater joeyslater requested a review from AGawrys June 25, 2021 20:40
@parcel-benchmark
Copy link

parcel-benchmark commented Jun 25, 2021 •
edited
Loading

Benchmark Results

Kitchen Sink ✅

Timings

Description Time Difference
Cold 1.40s -29.00ms
Cached 325.00ms -5.00ms

Cold Bundles

Bundle Size Difference Time Difference
dist/legacy/index.html 826.00b +0.00b 491.00ms -25.00ms 🚀

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.68s -42.00ms
Cached 282.00ms -16.00ms 🚀

Cold Bundles

No bundle changes detected.

Cached Bundles

No bundle changes detected.

Click here to view a detailed benchmark overview.

@AGawrys AGawrys mentioned this pull request Jul 16, 2021
Merged
@AGawrys AGawrys self-requested a review July 20, 2021 14:54
@AGawrys AGawrys changed the title Update without bundling for a 'simple' non-dependency related change [PAUSED] Update without bundling for a 'simple' non-dependency related change Jul 23, 2021
@wbinnssmith wbinnssmith marked this pull request as draft July 27, 2021 19:25
@AGawrys AGawrys force-pushed the jslater/non-dependency-increments branch from 9b91be6 to 6c1e807 Compare August 2, 2022 22:48
@AGawrys AGawrys requested a review from mischnic August 3, 2022 17:34
@AGawrys AGawrys self-requested a review August 8, 2022 18:11
@AGawrys AGawrys removed their request for review August 9, 2022 16:40
@AGawrys AGawrys marked this pull request as ready for review August 9, 2022 20:43
@AGawrys AGawrys changed the title [PAUSED] Update without bundling for a 'simple' non-dependency related change Update without bundling for a 'simple' non-dependency related change Aug 12, 2022
@AGawrys AGawrys self-requested a review September 13, 2022 17:21
@AGawrys AGawrys removed their request for review September 20, 2022 01:14
devongovett
devongovett reviewed Oct 17, 2022
View reviewed changes
packages/core/core/src/requests/BundleGraphRequest.js
@@ -285,7 +337,7 @@ class BundlerRunner {

await this.nameBundles(internalBundleGraph);

let changedAssets = await applyRuntimes({
let changedRuntimes = await applyRuntimes({
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need to apply runtimes in an incremental non-dependency update? I profiled, and after this change, applyRuntimes is still taking a significant amount of time. Is it because naming also occurs, and could potentially change bundle names? Should we disable both of these from running on incremental updates in dev? Development builds don't use content hashes anyway...

@devongovett
Copy link
Member

Something I noticed is that initial uncached builds appear to be significantly slower with this branch vs v2. The no dependency change builds are indeed faster though.

  • v2
    • Initial build: 19.58s
    • Change build: 4s
  • This branch
    • Initial build: 25.67s
    • Change build: 1.50s

This seems to be repeatable for me, i.e. not some environmental factor. In verbose logging mode, I also noticed more output, almost like it was rebuilding the same assets multiple times. Maybe related?

devongovett
devongovett approved these changes Oct 29, 2022
View reviewed changes
Copy link
Member

@devongovett devongovett left a comment

Choose a reason for hiding this comment

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

Going to merge this and refactor slightly to handle my comment above re runtimes.

@devongovett devongovett merged commit 05d0bc0 into v2 Oct 29, 2022
@devongovett devongovett deleted the jslater/non-dependency-increments branch October 29, 2022 19:22
@devongovett devongovett mentioned this pull request Oct 29, 2022
Merged
@mischnic mischnic mentioned this pull request Jun 5, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mischnic mischnic mischnic left review comments

@AGawrys AGawrys AGawrys approved these changes

@devongovett devongovett devongovett approved these changes

@wbinnssmith wbinnssmith Awaiting requested review from wbinnssmith

@thebriando thebriando Awaiting requested review from thebriando

@gorakong gorakong Awaiting requested review from gorakong

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@joeyslater @parcel-benchmark @devongovett @mischnic @AGawrys