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

Improve performance of incremental bundling #8583

Merged
merged 2 commits into from Oct 31, 2022
Merged

Conversation

devongovett
Copy link
Member

@devongovett devongovett commented Oct 29, 2022

This is a refactor of #6514, which improves performance even more.

  • Runtimes and namers are now no longer applied when incremental bundling, unless one of their sub requests has been invalidated (e.g. config/dev dependency change).
  • The asset graph request is moved to be a sub request of the bundle graph request. This way we can avoid making the bundle graph request id based on the asset graph hash, because it'll be naturally invalidated when the asset graph is. That avoids a traversal over the whole graph. We may be able to do this with the write bundles request as well, but I ran into a couple problems for now.
  • The cache key for the bundle graph is simplified so it doesn't change on every build. This should also save disk space. The tradeoff is that if an invalidation occurred but nothing actually changed, we wouldn't hit the cache. Also if you undid a change. I think these cases are more rare than changing code, so we should optimize for that. In the tests, nothing hit the cache for these cases ever.

return graph;
}
}

Copy link
Member Author

Choose a reason for hiding this comment

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

These cases were never hit at all in the tests, and should be very rare. I think removing the traversal to compute the hashes in the common case outweighs the potential benefits for zero change builds.

@parcel-benchmark
Copy link

parcel-benchmark commented Oct 29, 2022

Benchmark Results

Kitchen Sink ✅

Timings

Description Time Difference
Cold 1.71s -40.00ms
Cached 307.00ms -62.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 8.06s +299.00ms
Cached 345.00ms -18.00ms 🚀

Cold Bundles

No bundle changes detected.

Cached Bundles

Bundle Size Difference Time Difference
dist/Three.js 580.02kb +0.00b 6.21s +712.00ms ⚠️

Click here to view a detailed benchmark overview.

@mischnic
Copy link
Member

The cache key for the request graph is simplified so it doesn't change on every build. This should also save disk space. The tradeoff is that if an invalidation occurred but nothing actually changed, we wouldn't hit the cache. Also if you undid a change. I think these cases are more rare than changing code, so we should optimize for that. In the tests, nothing hit the cache for these cases ever.

Did you commit that? I don't see where that's done

@devongovett
Copy link
Member Author

Yeah, the getCacheKey function was deleted and replaced with this.cacheKey

@mischnic
Copy link
Member

But neither RequestGraph nor RequestTracker contain changes? Did you mean

The cache key for the request bundle graph is simplified

@devongovett
Copy link
Member Author

Oh, oops, yes that is exactly what I meant. 🤦

// If any subrequests are invalid (e.g. dev dep requests or config requests),
// bail on incremental bundling. We also need to invalidate for option changes,
// which are hoisted to direct invalidations on the bundle graph request.
let subRequestsInvalid =
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this mean that changes like adding a runtime would stop incremental bundling from happening? Thats the only issue I encountered when trying to not apply runtimes for incremental.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, if any config changes or plugin changes occurred, then bundling will not be incremental. I think this should be pretty rare so probably acceptable?

@devongovett devongovett merged commit c4a898c into v2 Oct 31, 2022
@devongovett devongovett deleted the incremental-bundling branch October 31, 2022 23:51
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