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 dev mode performance #1232

Merged
merged 8 commits into from
Nov 2, 2022
Merged

Improve dev mode performance #1232

merged 8 commits into from
Nov 2, 2022

Conversation

Janpot
Copy link
Member

@Janpot Janpot commented Oct 28, 2022

As per last retro, removing this (import { Error as ErrorIcon } from '@mui/icons-material';) import improves performance a lot. For now we can just pass a component down. Hopefully we can eventually move forward with mui/material-ui#30510

Alternatively we can explore compiling these packages to commonjs

@apedroferreira @bytasv please try out in your local and let me know if it improves things

@Janpot Janpot added the core Infrastructure work going on behind the scenes label Oct 28, 2022
@render
Copy link

render bot commented Oct 28, 2022

@Janpot Janpot marked this pull request as ready for review October 28, 2022 17:04
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Oct 30, 2022
Signed-off-by: Jan Potoms <2109932+Janpot@users.noreply.github.com>
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Oct 31, 2022
.eslintrc.js Outdated Show resolved Hide resolved
.eslintrc.js Outdated Show resolved Hide resolved
Janpot and others added 2 commits October 31, 2022 11:11
Co-authored-by: Vytautas Butkus <vytautas.butkus@gmail.com>
Signed-off-by: Jan Potoms <2109932+Janpot@users.noreply.github.com>
Co-authored-by: Vytautas Butkus <vytautas.butkus@gmail.com>
Signed-off-by: Jan Potoms <2109932+Janpot@users.noreply.github.com>
Copy link
Contributor

@bytasv bytasv left a comment

Choose a reason for hiding this comment

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

Hmm, I can't really tell the difference between how these changes work and what was before 🤔

Could it be that we are looking at different things? I'm checking by switching between pages, but it "feels" like the time it takes to load is very similar. Nevertheless approving this ✅

Screen.Recording.2022-10-31.at.12.14.40.mov

@Janpot
Copy link
Member Author

Janpot commented Oct 31, 2022

I didn't measure it exactly, but I see a clear difference:
before:

Screen.Recording.2022-10-31.at.11.21.01.mov

after:

Screen.Recording.2022-10-31.at.11.21.39.mov

Also Toolpad is much more responsive while switching the pages, I can actually click the hierarchy menus now.

@bytasv
Copy link
Contributor

bytasv commented Oct 31, 2022

I can see the difference in your recording, but I'm unable to reproduce same on my machine 🤔 are you running yarn dev or something different? Curious if Pedro will be able to feel the difference and why I'm not seeing the same 🤔

@Janpot
Copy link
Member Author

Janpot commented Oct 31, 2022

are you running yarn dev or something different?

In usually run yarn && yarn dev after switching branches. How are you checking out the code of this PR?

@bytasv
Copy link
Contributor

bytasv commented Oct 31, 2022

How are you checking out the code of this PR?

I have checked out branch locally, just tried to run yarn to install/refresh deps if anything is missing, then ran yarn dev. Wondering could it be related to how docker is set up? I.e. maybe you have more resources allocated 🤔

@apedroferreira could you also check? While it seems that I might be missing something on my end, it would be good to verify

@Janpot
Copy link
Member Author

Janpot commented Oct 31, 2022

Wondering could it be related to how docker is set up?

I doubt it, only the database is running in docker in development mode.

@apedroferreira
Copy link
Member

apedroferreira commented Oct 31, 2022

Doesn't really make a difference for me... My console shows the time each route takes to compile and the wait times are pretty huge:

Here some of them take about 5 seconds and one of them more than 30 seconds. So it can take a long time for me to open an app for the first time in dev - once it shows it's faster with the hot reload.

Screenshot 2022-10-31 at 17 41 04

Another run: 5.7s + 14s + 3.4s, not sure if these times are unusual:

Screenshot 2022-10-31 at 17 45 37

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Oct 31, 2022
Signed-off-by: Jan Potoms <2109932+Janpot@users.noreply.github.com>
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Oct 31, 2022
@Janpot Janpot enabled auto-merge (squash) October 31, 2022 18:40
@Janpot
Copy link
Member Author

Janpot commented Nov 2, 2022

It's important to understand that Next.js optimizes for startup time in dev mode. That means it will compile sources on-demand. As soon as it is compiled once, the HMR should make incremental compilation faster. The consequence is that the first time opening a page will be a little slower. It's either this or longer startup time. Here's to hoping that the upcoming turbopack update will make this a lot faster still. I don't think there's much we can do here, maybe we can profile some internals? All in all, I don't think it makes much sense to look at initial compile time. If there are reasons why you often have to restart the application, I'd love to hear about it.

@bytasv
Copy link
Contributor

bytasv commented Nov 2, 2022

I personally don't care much about initial load time as I understand it always takes longer. The problem is that page load time for me doesn't change even when I'm not changing any code, i.e. I just click back and forth between pages it takes as much time as in your video of "before". That's why I'm puzzled why I'm not seeing same improvement as you do 🤔

@Janpot Janpot merged commit 78293bf into master Nov 2, 2022
@Janpot Janpot deleted the dev-perf branch November 2, 2022 14:21
@apedroferreira
Copy link
Member

If we can't find any more quick wins I'm fine with things staying as they are and hoping upcoming Next.js updates improve things, it's not that bad of an issue to spend more time on it in my opinion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Infrastructure work going on behind the scenes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants