Improve minChunkSize algorithm #4723

lukastaegert · 2022-11-25T05:51:01Z

This PR contains:

Are tests included?

yes (bugfixes and features will not be merged without tests)
no

Breaking Changes?

yes (breaking changes will not be merged unless absolutely necessary)
no

List any relevant issue numbers:

Can code splitting be done according to chunk size? #4327 (comment)

Description

This PR improves the minChunkSize algorithm in several ways:

In a new first pass, the algorithm tries to merge small chunks with side effects with suitable chunks without side effects to increase size. A pure chunk is "suitable" for being merged if the entry points depending on that chunk are a sub set of the entry points of the chunk with side effects (that way, the side effect is not introduced to new entry points)
In a second pass, small pure chunks are merged into other pure or non-pure chunks (following the above rule if the merge target has side effects)
Chunks (small and large) are sorted by size first to increase the chance of small chunks finding merge partners vs larger chunks
A merge will only be performed if it does not create a cyclic dependency

Even though we now have a step to merge chunks with side effects, it still requires a sufficient amount of pure chunks to be present, and there is no guarantee suitable merge partners are available. This PR also includes #4718 to allow to try out stuff.

It also contains some simple logging to get some numbers how many chunks were merged and which chunks were not merged.

@gajus would be nice if you could give this one a shot and report the results.

github-actions · 2022-11-25T05:52:56Z

Thank you for your contribution! ❤️

You can try out this pull request locally by installing Rollup via

npm install rollup/rollup#better-minchunksize

or load it into the REPL:
https://deploy-preview-4723--rollupjs.netlify.app/repl/?pr=4723

rollup-bot · 2022-11-25T05:56:46Z

This PR has been released as part of rollup@3.5.0-0. Note that this is a pre-release, so to test it, you need to install Rollup via npm install rollup@3.5.0-0 or npm install rollup@beta. It will likely become part of a regular release later.

codecov · 2022-11-25T05:57:16Z

Codecov Report

Merging #4723 (56f717b) into master (52ba95c) will decrease coverage by 0.03%.
The diff coverage is 98.79%.

❗ Current head 56f717b differs from pull request most recent head 5a7cb45. Consider uploading reports for the commit 5a7cb45 to get more accurate results

@@            Coverage Diff             @@
##           master    #4723      +/-   ##
==========================================
- Coverage   98.99%   98.97%   -0.03%     
==========================================
  Files         219      219              
  Lines        7839     7878      +39     
  Branches     2171     2182      +11     
==========================================
+ Hits         7760     7797      +37     
  Misses         26       26              
- Partials       53       55       +2

Impacted Files	Coverage Δ
src/utils/iterators.ts	`100.00% <ø> (ø)`
src/utils/chunkAssignment.ts	`99.46% <98.75%> (-0.54%)`	⬇️
src/Module.ts	`100.00% <100.00%> (ø)`
src/ast/nodes/Program.ts	`94.44% <100.00%> (-5.56%)`	⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

rollup-bot · 2022-11-27T06:55:54Z

This PR has been released as part of rollup@3.6.0-0. Note that this is a pre-release, so to test it, you need to install Rollup via npm install rollup@3.6.0-0 or npm install rollup@beta. It will likely become part of a regular release later.

rollup-bot · 2022-12-05T08:35:31Z

This PR has been released as part of rollup@3.6.0-1. Note that this is a pre-release, so to test it, you need to install Rollup via npm install rollup@3.6.0-1 or npm install rollup@beta. It will likely become part of a regular release later.

lukastaegert · 2022-12-05T08:36:24Z

@gajus please give this PR another shot and report the results.

gajus · 2022-12-05T14:57:50Z

@gajus please give this PR another shot and report the results.

Is this the 3.6.0-1 release?

If yes, this logs at build time:

Created 439 chunks
----- pure  side effects
small 257   172
  big 0     10

Trying to find merge targets for 172 chunks smaller than 500 kB with side effects...
172 chunks smaller than 500 kB with side effects remaining.

Trying to find merge targets for 41 pure chunks smaller than 500 kB...
5 pure chunks smaller than 500 kB remaining.

and at runtime it breaks with

Uncaught TypeError: S is not a function
    at Ce (chunk-ErrorBoundary.26a1e903.js:79:5156)
    at chunk-usePersistedReferralIntent.252f193a.js:2:30771
Ce @ chunk-ErrorBoundary.26a1e903.js:79
(anonymous) @ chunk-usePersistedReferralIntent.252f193a.js:2

Removing experimentalMinChunkSize configuration makes the app work again.

rollup-bot · 2022-12-06T05:41:47Z

This PR has been released as part of rollup@3.6.1-0. Note that this is a pre-release, so to test it, you need to install Rollup via npm install rollup@3.6.1-0 or npm install rollup@beta. It will likely become part of a regular release later.

lukastaegert · 2022-12-06T05:44:59Z

I see, I think I had some wrong assumptions here. I reworked the algorithm to explicitly avoid creating cycles within chunks, not module dependencies. Could you run it again with rollup@3.6.1-0?

gajus · 2022-12-06T11:33:55Z

3.6.1-0?

haven't had a chance to debug yet, but it is a new error at the moment:

Uncaught TypeError: Cannot read properties of undefined (reading 'exports')
at chunk-index.c5dffacd.js:11:83112

lukastaegert · 2022-12-06T15:58:36Z

I see. I will add some checks to look for pre-existing and introduced cycles. Will get back to you when that is ready.

rollup-bot · 2022-12-08T05:51:08Z

This PR has been released as part of rollup@3.7.1-0. Note that this is a pre-release, so to test it, you need to install Rollup via npm install rollup@3.7.1-0 or npm install rollup@beta. It will likely become part of a regular release later.

lukastaegert · 2022-12-08T06:05:12Z

Ok, rollup@3.7.1-0 will not solve your issues, but it adds logging that will tell you if I am looking in the right direction. Can you give it a spin and tell me what the logging says now?

gajus · 2022-12-08T15:08:13Z

Build logs:

Created 461 chunks
----- pure  side effects
small 265   186
  big 0     10
Unoptimized chunks contain 0 cycles.

Trying to find merge targets for 186 chunks smaller than 500 kB with side effects...
186 chunks smaller than 500 kB with side effects remaining.
Generated chunks contain 179 cycles.

Trying to find merge targets for 7 pure chunks smaller than 500 kB...
1 pure chunks smaller than 500 kB remaining.
Generated chunks contain 200 cycles.

Runtime logs:

chunk-Stripes.eec2eb5e.js:1 Uncaught ReferenceError: Cannot access 'p' before initialization
    at chunk-Stripes.eec2eb5e.js:1:3546

Is there anything in terms of logs that you were seeking for?

lukastaegert · 2022-12-08T15:43:37Z

Generated chunks contain 179 cycles.

Thanks, this answers my question. Need to think a little longer how to properly prevent the creation of cycles. But should be a solvable issue.

rollup-bot · 2022-12-10T06:33:38Z

This PR has been released as part of rollup@3.7.3-0. Note that this is a pre-release, so to test it, you need to install Rollup via npm install rollup@3.7.3-0 or npm install rollup@beta. It will likely become part of a regular release later.

lukastaegert · 2022-12-10T06:35:37Z

Ok, I reworked the cycle prevention algorithm. Could you give rollup@3.7.3-0 another go and report the console output and whether it now works?

gajus · 2022-12-11T23:31:47Z

👋 Let's see...

Console output:

Created 468 chunks
----- pure  side effects
small 269   189
  big 0     10
Unoptimized chunks contain 0 cycles.

Trying to find merge targets for 189 chunks smaller than 500 kB with side effects...
189 chunks smaller than 500 kB with side effects remaining.
Generated chunks contain 118 cycles.

Trying to find merge targets for 46 pure chunks smaller than 500 kB...
3 pure chunks smaller than 500 kB remaining.
Generated chunks contain 174 cycles.

202 chunks remaining.

Same error as earlier though:

chunk-index.module.ae520628.js:17 Uncaught TypeError: Cannot read properties of undefined (reading 'exports')
    at chunk-index.module.ae520628.js:17:14

Let me know if I can provide any other useful information.

rollup-bot · 2022-12-16T10:56:31Z

This PR has been released as part of rollup@3.7.5-0. Note that this is a pre-release, so to test it, you need to install Rollup via npm install rollup@3.7.5-0 or npm install rollup@beta. It will likely become part of a regular release later.

lukastaegert · 2022-12-16T10:56:42Z

Ok, I managed to reproduce the issue locally and hopefully found a way to fix the issue for good. Apparently, I was not tracking transitive dependencies correctly. I hope performance of the current solution is still ok.
Please give rollup@3.7.5-0 another go.

gajus · 2022-12-16T17:12:00Z

Ok, I managed to reproduce the issue locally and hopefully found a way to fix the issue for good. Apparently, I was not tracking transitive dependencies correctly. I hope performance of the current solution is still ok. Please give rollup@3.7.5-0 another go.

Unfortunately, same error. Happy to provide as much context as needed.

lukastaegert · 2022-12-16T17:46:13Z

So, no change in the number of cycles found? What are the logs?

netlify · 2023-02-03T09:45:56Z

✅ Deploy Preview for rollupjs ready!

Name	Link
🔨 Latest commit	`5a7cb45`
🔍 Latest deploy log	https://app.netlify.com/sites/rollupjs/deploys/63dcf1e87004fd00088d1493
😎 Deploy Preview	https://deploy-preview-4723--rollupjs.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

lukastaegert · 2023-02-03T09:58:33Z

I removed the logging and will merge the changes from this branch into master and release them as I think they are still a net-improvement. But I guess we need to keep working on this feature.

rollup-bot · 2023-02-03T12:55:52Z

This PR has been released as part of rollup@3.13.0. You can test it via npm install rollup.

shirotech · 2023-02-03T19:23:42Z

Using manualChunks will just put the two modules into the same chunk

Can you use both manualChunks and experimentalMinChunkSize both will do it's own thing without one replacing the other?

gajus · 2023-02-03T21:48:00Z

@lukastaegert I've been heads down helping team to prepare for the upcoming launch, but now have a bit more time to debug this.

For what it is worth, I can see from debugging our build that the issue is caused by a circular reference.

I've removed all instances that were triggering CYCLIC_CROSS_CHUNK_REEXPORT. However, as soon as I re-add minChunkSize, it is starting to fail – seems like the dependencies that get bundled create a circular reference.

I am making further code changes on our end to reduce the risk of circular dependencies. Will follow up once I know more.

gajus · 2023-02-04T04:54:36Z

Removing all circular references from the codebase did not solve the issue.

lukastaegert · 2023-02-04T05:44:12Z

@shirotech

Can you use both manualChunks and experimentalMinChunkSize both will do it's own thing without one replacing the other?

Exactly. It is possible that experimentalMinChunkSize merges additional chunks into a manual chunk.

@gajus

Not sure if this is any option, but if you can grant me temporary access to your code (under an NDA of course), I could debug it from there, which would be much simpler. I sent you a Discord message regarding this.

gajus · 2023-02-04T08:40:33Z

Not sure if this is any option, but if you can grant me temporary access to your code (under an NDA of course), I could debug it from there, which would be much simpler. I sent you a Discord message regarding this.

I saw your message (sorry again for poor communication), and I've asked to evaluate what would it take to share access. The problem is that app is part of a mono repo that includes a lot of sensitive information.

gajus · 2023-02-04T08:50:49Z

You know, I will just power through this and setup something. Might take a few days, but worth it. Will DM when ready.

gajus · 2023-02-06T05:45:48Z

Still trying to get an isolated app to work.

For what it is worth, the error that we are getting now is:

chunk-index.d8e05bc2.js:17 Uncaught TypeError: Cannot read properties of undefined (reading '__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED')
    at chunk-index.d8e05bc2.js:17:228
    at chunk-index.d8e05bc2.js:147:612

Which is slightly different than what it was before.

It seems almost like there is a circular reference among react dependencies.

gajus · 2023-02-06T18:58:43Z

@lukastaegert So some good news and not so good news.

The not so good news is that I was not able to create an isolated example of what is breaking chunking.

The good news is that while trying to create an isolated project, I narrowed down the issue to just one dependency. So we just need to figure out what that (internal) dependency is doing that is breaking chunking.

lukastaegert · 2023-02-07T05:30:40Z

Great stuff! I will report back with my findings.

gajus · 2023-02-07T05:37:13Z

Heads up that this turned out to be mostly a false-positive finding. Sent an update through Discord.

lukastaegert · 2023-02-07T08:15:22Z

Hi. When I set build.commonjsOptions.strictRequires: true in your vite.config.ts it fixes the issue for me. Can you confirm this for your full build? That would confirm my belief that it has to do something with the commonjs plugin. In any case, setting this option to true is safe and even recommended. I currently ponder making this setting a default of the commonjs plugin.

As background, build.commonjsOptions directly configures @rollup/plugin-commonjs: https://github.com/rollup/plugins/tree/master/packages/commonjs#strictrequires

What puzzles me is how the chunking can trigger a bug here, so I will do some more digging. But maybe in the end, this can already be the solution. Otherwise, your reproduction is also a very good testing ground for build memory and performance improvements 😉

gajus · 2023-02-07T10:08:20Z

So, it is failing for a different reason now:

14chunk-useIsSmallScreen.4fff2a71.js:2048 Uncaught TypeError: Cannot set properties of undefined (setting 'Children')
    at chunk-useIsSmallScreen.4fff2a71.js:2048:26
    at chunk-useIsSmallScreen.4fff2a71.js:2086:9
    at requireReact_development (chunk-useIsSmallScreen.4fff2a71.js:2088:5)
    at chunk-useIsSmallScreen.4fff2a71.js:2098:24
    at requireReact (chunk-useIsSmallScreen.4fff2a71.js:2100:5)
    at chunk-index.e953bbca.js:21:20

which refers to:

        var Children = {
          map: mapChildren,
          forEach: forEachChildren,
          count: countChildren,
          toArray,
          only: onlyChild
        };
        exports.Children = Children;
        exports.Component = Component;
        exports.Fragment = REACT_FRAGMENT_TYPE;

This looks like a fragment of React library, and exports is undefined.

lukastaegert · 2023-02-07T10:30:49Z

Ah, that is very interesting. So it still seems to be related to some bug in the commonjs logic. If have some ideas for how I want to improve that plugin, I will try some things.

gajus · 2023-02-10T18:29:32Z

When doing minChunkSize, is there a way to tell not to merge specific chunks?

I just realized that it is merging page chunks, which is not desirable.

import "./me.page.client.a1bf777a.js";
import "../../chunk-useRoutes.ad41330f.js";
import "../../chunk-index.4988a863.js";
import "../../chunk-usePostHog.af72090a.js";
import "../../chunk-logger.40d3ad32.js";
import "./wallet.page.client.3636703f.js";
import "../../chunk-async-storage.39ded5be.js";
import "../../chunk-WindowDimensionsContext.e0712d35.js";
import "../../chunk-useUserTypeSelector.d0fa7beb.js";
import "../../chunk-index.ff67dbb0.js";
import "../../chunk-index.599a5f27.js";
import "./edit-profile.page.client.6afc5f59.js";

lukastaegert · 2023-02-11T05:55:59Z

Why is that not desireable? Is there a logic that relies on these chunks being present? Or is it causing issues in other places? We can certainly implement some option on top to preserve certain chunks, but the implementation depends on what issues the merge causes. But the more chunks are excluded, the harder it will be to meet any size constraints.

lukastaegert · 2023-02-11T05:56:30Z

Also, please give #4851 a spin!

shirotech · 2023-02-11T06:48:57Z

Hi @lukastaegert great work on the algorithm, just one thing I noticed when working with both manualChunks and minChunkSize together. Hoping to get some advice on that is it possible to override the behaviour of minChunkSize i.e. referring to the core function in manualChunks, and so it's more predictable.

For example I can see the main function is referred here:

rollup/src/utils/chunkAssignment.ts

Line 279 in a7b36ca

function getOptimizedChunks(

Would you be open to exporting all those functions in that file so that it is possible to make use of it within the manualChunks hook itself?

lukastaegert · 2023-02-11T12:59:29Z

Not sure. On the one hand this is still experimental and can and will change a couple more times during upcoming releases. But the main problem is that the algorithm heavily relies on analytics done in other parts of chunking algorithm, while manualChunks runs before other chunking to remove modules from the regular chunking algorithm. It very much depends on what you want to accomplish.

lukastaegert · 2023-02-11T13:24:41Z

When doing minChunkSize, is there a way to tell not to merge specific chunks?

I just noticed that there are still scenarios where side effects are triggered earlier than they should, which may be the problem you are seeing. I have an idea how to fix this, will put this into #4851

lukastaegert · 2023-02-11T21:28:10Z

Ok, I reworked #4851, maybe that addresses the issues you saw with page chunks.

lukastaegert mentioned this pull request Nov 25, 2022

Can code splitting be done according to chunk size? #4327

Closed

lukastaegert force-pushed the better-minchunksize branch from 840ea6c to 03fdcc0 Compare November 27, 2022 06:52

lukastaegert force-pushed the better-minchunksize branch from 28465ce to ae77794 Compare December 5, 2022 08:30

lukastaegert force-pushed the better-minchunksize branch from 3a100e1 to 439012b Compare December 5, 2022 13:34

lukastaegert force-pushed the better-minchunksize branch 2 times, most recently from 107fcdc to a3259be Compare December 8, 2022 05:46

lukastaegert force-pushed the better-minchunksize branch from c2834b8 to c2de6b9 Compare December 10, 2022 06:29

lukastaegert force-pushed the better-minchunksize branch from 2fd4b7a to 1ee5d10 Compare December 16, 2022 10:52

lukastaegert force-pushed the better-minchunksize branch from 900fae0 to b1c586f Compare February 3, 2023 09:45

Remove logging

56f717b

lukastaegert force-pushed the better-minchunksize branch from 8847d3a to 56f717b Compare February 3, 2023 09:56

Improve coverage

5a7cb45

lukastaegert merged commit 0a5ea57 into master Feb 3, 2023

lukastaegert deleted the better-minchunksize branch February 3, 2023 12:41

lukastaegert mentioned this pull request Mar 23, 2023

Handle manual chunks where the entry is not part of the module graph #4921

Merged

9 tasks

Improve minChunkSize algorithm #4723

Improve minChunkSize algorithm #4723

Conversation

lukastaegert commented Nov 25, 2022 • edited

Description

github-actions bot commented Nov 25, 2022 • edited

Thank you for your contribution! ❤️

rollup-bot commented Nov 25, 2022

codecov bot commented Nov 25, 2022 • edited

Codecov Report

rollup-bot commented Nov 27, 2022

rollup-bot commented Dec 5, 2022

lukastaegert commented Dec 5, 2022

gajus commented Dec 5, 2022

rollup-bot commented Dec 6, 2022

lukastaegert commented Dec 6, 2022

gajus commented Dec 6, 2022

lukastaegert commented Dec 6, 2022

rollup-bot commented Dec 8, 2022

lukastaegert commented Dec 8, 2022

gajus commented Dec 8, 2022

lukastaegert commented Dec 8, 2022

rollup-bot commented Dec 10, 2022

lukastaegert commented Dec 10, 2022

gajus commented Dec 11, 2022 • edited

rollup-bot commented Dec 16, 2022

lukastaegert commented Dec 16, 2022

gajus commented Dec 16, 2022

lukastaegert commented Dec 16, 2022 • edited

netlify bot commented Feb 3, 2023 • edited

✅ Deploy Preview for rollupjs ready!

lukastaegert commented Feb 3, 2023

rollup-bot commented Feb 3, 2023

shirotech commented Feb 3, 2023

gajus commented Feb 3, 2023

gajus commented Feb 4, 2023

lukastaegert commented Feb 4, 2023

gajus commented Feb 4, 2023

gajus commented Feb 4, 2023

gajus commented Feb 6, 2023

gajus commented Feb 6, 2023

lukastaegert commented Feb 7, 2023

gajus commented Feb 7, 2023

lukastaegert commented Feb 7, 2023

gajus commented Feb 7, 2023 • edited

lukastaegert commented Feb 7, 2023

gajus commented Feb 10, 2023

lukastaegert commented Feb 11, 2023

lukastaegert commented Feb 11, 2023

shirotech commented Feb 11, 2023

lukastaegert commented Feb 11, 2023

lukastaegert commented Feb 11, 2023

lukastaegert commented Feb 11, 2023

lukastaegert commented Nov 25, 2022 •

edited

github-actions bot commented Nov 25, 2022 •

edited

codecov bot commented Nov 25, 2022 •

edited

gajus commented Dec 11, 2022 •

edited

lukastaegert commented Dec 16, 2022 •

edited

netlify bot commented Feb 3, 2023 •

edited

gajus commented Feb 7, 2023 •

edited