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

perf(optimizer): start optimizer early #12593

Merged
merged 6 commits into from
Mar 26, 2023
Merged

perf(optimizer): start optimizer early #12593

merged 6 commits into from
Mar 26, 2023

Conversation

bluwy
Copy link
Member

@bluwy bluwy commented Mar 26, 2023

Description

Currently the deps optimizer is inited only after the server is created and listening. However, the deps optimizer only relies on the server for module graph invalidation and page reload.

As these are technically optional if the server isn't ready yet, we should be able to init the optimizer before the server is created, which is what this PR does.

When the server is ready, we pass the server to the optimizer again for future server handling.

Additional context

Without this, we can't call server.transformRequest early on to pre-cache things, as we need the optimizer to properly resolve paths.

With this PR, the optimizer can start 260ms earlier on a project I'm testing.


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

@bluwy bluwy added the performance Performance related enhancement label Mar 26, 2023
@stackblitz
Copy link

stackblitz bot commented Mar 26, 2023

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@bluwy
Copy link
Member Author

bluwy commented Mar 26, 2023

Somehow this PR makes the unit test more likely to segfault for some reason 😢

@patak-dev
Copy link
Member

I dont think it is this PR, segfaults increased a lot in the past week

@bluwy
Copy link
Member Author

bluwy commented Mar 26, 2023

I tested in main with node 14 and it does happen less often. Only with this PR's specific change it triggers it much more often. Not sure what's going on, but I'm looking forward to moving away from node 14 soon 🥲

Copy link
Member

@patak-dev patak-dev left a comment

Choose a reason for hiding this comment

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

This is awesome ❤️

Comment on lines +14 to +16
optimizeDeps: {
disabled: true,
},
Copy link
Member

Choose a reason for hiding this comment

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

Was this needed for the PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not really. A lot of changes actually wasn't needed until node14 start messing up everything 😄 Only the first commit was the real changes.

But I think the changes are fine still including this one, since for this case the unit test won't spin up an unused deps optimizer instance (for this PR onwards). I'm happy to revert it too though and we can check separately.

Copy link
Member

Choose a reason for hiding this comment

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

Nah, let's keep it :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Performance related enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants