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

Optimize webpack chunking #38329

Merged
merged 4 commits into from May 24, 2023
Merged

Optimize webpack chunking #38329

merged 4 commits into from May 24, 2023

Conversation

pulsejet
Copy link
Member

@pulsejet pulsejet commented May 17, 2023

Supercedes and closes #35432

This takes a much simpler approach for achieving something similar to the linked PR. Instead of lazy loading every component, webpack directly allows only splitting components used in more than n modules. By tweaking this parameter (3 seems to have great results), we can split the chunks efficiently achieving similar performance gains. This is also much less invasive and more semantically meaningful.

Some amount of async loading is still needed since apps greedily attempt to load a lot of things.

Before After
wp_before wp_after

Files app (no cache):

Before After
135 requests 138 requests
5.3 MB transferred 3.7 MB transferred
21.0 MB resources 12.2 MB resources
DOMContentLoaded: 4.19 s DOMContentLoaded: 3.02 s

Files app (with cache):

Before After
133 requests 133 requests
72.4 kB transferred 72.4 kB transferred
21.0 MB resources 12.2 MB resources
DOMContentLoaded: 2.60 s DOMContentLoaded: 1.39 s

Memories (no cache):

Before After
83 requests 83 requests
5.2 MB transferred 3.3 MB transferred
21.8 MB resources 12.3 MB resources
DOMContentLoaded: 3.15 s DOMContentLoaded: 2.25 s

Video of load performance (before on top, after on bottom)
https://github.com/nextcloud/server/assets/10709794/5ab1144e-c811-4b8d-9798-463982b46cb7

@juliushaertl
Copy link
Member

I think this is a great initiative. I'd have one concern which is due to the fact that we currently still require to have the bundles committed in the repository itself. The dynamic naming with numeric identifiers has the risk that we end up having leftover chunks in the dist folder if they are not cleaned up before each production build, so we need to double check that this is the case.

@juliushaertl
Copy link
Member

Ok, might actually be the case already with https://github.com/nextcloud/server/blob/pulsejet/patch-webpack/webpack.common.js#L59-L61

Copy link
Member

@juliushaertl juliushaertl left a comment

Choose a reason for hiding this comment

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

Eslint is not happy, otherwise this seems good from my perspective.

@marcelklehr
Copy link
Member

Thank you @pulsejet for not giving up on this 💙

@pulsejet
Copy link
Member Author

Ok, might actually be the case already with https://github.com/nextcloud/server/blob/pulsejet/patch-webpack/webpack.common.js#L59-L61

Never knew about this, but seems it'll do the trick: https://webpack.js.org/guides/output-management/#cleaning-up-the-dist-folder.

ESLint should pass now.

@pulsejet
Copy link
Member Author

Just noticed something funny: this is actually also a bug fix at certain points. For instance, we explicitly want to do lazy loading (before this PR) at this line for caniuse-lite. But as is evident from the bundle, the library was present in core-common.js all along, so the lazy loading never really worked.

async beforeMount() {
// Dynamic load big list of user agents
// eslint-disable-next-line n/no-extraneous-import
const { agents } = await import('caniuse-lite')
this.agents = agents
},

pulsejet added a commit that referenced this pull request May 17, 2023
This check is very expensive, and will pass almost 100% of the time.

Related #36728
Depends on #38329

Signed-off-by: Varun Patil <varunpatil@ucla.edu>
nextcloud-command pushed a commit that referenced this pull request May 19, 2023
This check is very expensive, and will pass almost 100% of the time.

Related #36728
Depends on #38329

Signed-off-by: Varun Patil <varunpatil@ucla.edu>
nextcloud-command pushed a commit that referenced this pull request May 19, 2023
This check is very expensive, and will pass almost 100% of the time.

Related #36728
Depends on #38329

Signed-off-by: Varun Patil <varunpatil@ucla.edu>
Signed-off-by: nextcloud-command <nextcloud-command@users.noreply.github.com>
@pulsejet
Copy link
Member Author

Short before (top) and after (bottom) video for this (warm cache for both):

prtest.mp4

@pulsejet pulsejet force-pushed the pulsejet/patch-webpack branch 2 times, most recently from e3c1b5d to a150c8c Compare May 21, 2023 16:46
@pulsejet
Copy link
Member Author

Bump? I really don't understand what keeps blocking this...

@artonge
Copy link
Contributor

artonge commented May 23, 2023

@nextcloud/server-frontend could you review/approve ?

pulsejet and others added 4 commits May 23, 2023 22:34
This makes tree shaking possible

Signed-off-by: Varun Patil <varunpatil@ucla.edu>
Signed-off-by: Varun Patil <varunpatil@ucla.edu>
Signed-off-by: Varun Patil <varunpatil@ucla.edu>
Signed-off-by: Daniel Kesselberg <mail@danielkesselberg.de>
@kesselb
Copy link
Contributor

kesselb commented May 23, 2023

@pulsejet rebased, compiled and force-pushed your branch. Used node 18.7 and npm 8.15 to build locally.

@pulsejet
Copy link
Member Author

Thanks @kesselb. Just curious: doesn't the bot do that usually or I should be pushing compiled assets to the PR?

@skjnldsv
Copy link
Member

Bump? I really don't understand what keeps blocking this...

27 being released and master being in feature freeze.
This is now branched-off and we can resume

@skjnldsv skjnldsv merged commit 4811a02 into master May 24, 2023
37 checks passed
@skjnldsv skjnldsv deleted the pulsejet/patch-webpack branch May 24, 2023 07:03
@skjnldsv
Copy link
Member

Thanks @kesselb. Just curious: doesn't the bot do that usually or I should be pushing compiled assets to the PR?

the bot needs to be called for it :)
/compile amend /

@skjnldsv skjnldsv added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels May 24, 2023
@pulsejet
Copy link
Member Author

Thanks for pushing this through @skjnldsv 🎉

@major-mayer
Copy link

Now that I finally upgraded my instance to 28, I want to say thank you for this PR again @pulsejet
This increases the performance noticeably!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish javascript performance 🚀
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants