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

feat: improved cold start using deps scanner #8869

Merged
merged 32 commits into from Jul 4, 2022

Conversation

patak-dev
Copy link
Member

@patak-dev patak-dev commented Jun 30, 2022

Fix #8867

Description

See comment #8869 (comment)

image

Initial exploration of diff strategies

Click to read the original intent of the PR
Adds a new `optimizeDeps.devStrategy` option to allow users to define the cold start strategy.
  • 'pre-scan': pre-scan user code with esbuild to find the first batch of dependencies to optimize. This is the v2.9 scheme (this option replaces legacy.devDepsScanner, removed in this PR)
  • 'lazy': only static imports are crawled, leading to the fastest cold start experience with the tradeoff of possible full page reload when navigating to dynamic routes. This is the current default.
  • 'eager': both static and dynamic imports are processed on cold start completely removing the need for full page reloads at the expense of a slower cold start.

And there is a new strategy that I think should be the default for non MPAs:

  • 'dynamic-scan': delay optimization until static imports are crawled, then scan with esbuild dynamic import entries found in the source code.

Edit: after some discussions, I think if we have to use the scanner then the best would be a new mode (to be implemented)

  • 'scan': perform scanning in the background, then once the server is idle, use the scanned deps list to complete what was found normally. It would be like 'lazy' + completed with the scanned deps
image

Data point for a SPA (57 dynamic routes)

- v2.9:            20s
- v3.0: 'lazy':    11s  (adding one dep to `optimizeDeps.include`, if not there is a reload)
- v3.0: 'eager':   27s

What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

@netlify
Copy link

netlify bot commented Jun 30, 2022

Deploy Preview for vite-docs-main canceled.

Name Link
🔨 Latest commit 8861550
🔍 Latest deploy log https://app.netlify.com/sites/vite-docs-main/deploys/62c28c9c35d7bd0008ef6e94

@patak-dev patak-dev added p3-significant 🔨 High priority enhancement (priority) feat: ssr feat: deps optimizer Esbuild Dependencies Optimization and removed feat: ssr labels Jun 30, 2022
@patak-dev
Copy link
Member Author

patak-dev commented Jul 2, 2022

Our conclusions from the last team meeting:

  • We should keep the scanner for v3, as it is currently a good way to ensure DX on cold start.
  • As we are going to make using the scanner the default, we don't need the devStrategy options at this point.
  • Since we are using the scanner, and we now have the infra in place to detect the end of the static imports crawling, we can avoid full page reloads by delaying sending the optimized deps to the browser until the crawling is done and then check if we have all the deps needed to render the page. How it was before:

image

After this PR:

image

Same findings:

  • Using the scanner should fix:
    • optimizeDeps does not process aliased imports #8867.
      The problem here is that the preAliasPlugin uses the scanned deps for its logic. We should refactor it to resolve each aliased bare import. We may need to inline and modify Rollup plugin alias for this. I'll tackle this in a future PR once I add back the possibility to run dev without the scanner
  • For the same reason, we need the scanner in build time until the preAliasPlugin is changed. This PR adds it back.

@patak-dev patak-dev changed the title feat: optimizeDeps.devStrategy, new 'dynamic-scan' strat feat: improved cold start using deps scanner Jul 2, 2022
@patak-dev patak-dev added this to the 3.0 milestone Jul 2, 2022
@patak-dev
Copy link
Member Author

patak-dev commented Jul 2, 2022

It took me a while to find the race condition that was causing the errors. I think some of the errors we've seen lately may be related to the SSR devs optimizer running before the scanner (or when the scanner was not even run).

We need to find a way later to implement the preAlias plugin without the scanner. Now only if the scanner finds an aliased and optimized dependency, the plugin works correctly. I think it is the only place left where the scanner needs to be awaited. For the moment, the PR implements guards to start the optimizers after the scanner is done. I'll try to modify the preAlias plugin after v3 is out.

@patak-dev
Copy link
Member Author

patak-dev commented Jul 3, 2022

@sapphi-red @bluwy I'm using this PR to check the CI flakiness, since after enabling the scanner again it becomes more evident
I think I found one of the main issues. We had several playgrounds with two or three .spec.ts files. Each of them would trigger the creating of a default dev server, so we had race conditions with two or even three dev servers scanning, creating caches with the same dir name, etc.
Two different cases:

  • For source maps we had the pattern:
    • __tests__/serve.spec.ts
    • __tests__/build.spec.ts
      We could extend vitestSetup to support this, but for the moment I combined them in a single spec.ts.
  • In the CSS playground we had two extra .spec.ts that were starting their own servers. But if we don't add a serve.ts in the same folder, they will also start a default server. So for the CSS playground, we had three servers up in CI.

This explains why the CSS tests were one of the most usual to fail, and also the sourcemaps had failures regularly.

I wasn't checking this myself, so it is something to be aware of when we review PRs in the future. This commit in the PR removes these: 6c72736

I'm still fixing other things, so fails are expected

@patak-dev
Copy link
Member Author

patak-dev commented Jul 3, 2022

@sapphi-red I see why you did the build.spec.ts/serve.spect.ts split. The snapshot takes the name, so they are competing between server and build now that I mixed them into one. Any ideas? If not I'll have to revert the change and give support to the pattern in vitestSetup 🤔
We could check if build.spect.ts is used, then the dev server isn't started, and server.spec.ts, then the build server is not run.

@sapphi-red
Copy link
Member

sapphi-red commented Jul 3, 2022

@patak-dev I don't completely grasp the situation but maybe using inline snapshot helps? (It makes the test file long and messy so it is not a good choice though...)

@patak-dev
Copy link
Member Author

patak-dev commented Jul 3, 2022

@sapphi-red the problem is that vitest is deleting the snapshots if we call pnpm run test-build -u because they are skipped. This may count like a bug in vitest, or maybe a feature request. I think at one point we should replace runIf by onlyIf and show all tests grayed instead of yellow/skipped like now, and these tests could ignore -u for their snaphots.
I think this may not be a new issue though, so we can move on and later find a good solution.
I pushed hints so the cases are easier to find in the snapshot file

@patak-dev
Copy link
Member Author

patak-dev commented Jul 3, 2022

@bluwy I tried to implement auto-exclude of explicit external deps here:

You can see the tests in that commit working well.

But VitePress failed to build with it. Vue is setting itself as ssr.external in plugin-vue. But normally it is optimized for the browser. I'm starting to think that we can't safely use noExternal and external to define which dependencies should be optimized during SSR.
I think we should remove these heuristics in v3. I did so in this commit:

@bholmesdev this may also affect Astro, as I think you were counting on having noExternal optimized automatically.

So if someone has an issue with a noExternal dep, they need to manually add it to optimizeDeps.include.
We will still be using the same optimizeDeps for SSR and non-SSR deps. During build time we can use ssrBuild to conditionally add diff deps. But during dev it isn't possible.
I'm thinking that we should provide a way to configure optimization for SSR only during dev using a function:

optimizedDeps: ssr => {
  if (ssr) {
    return {
      include: [ ... ]
      exclude: [ ... ]
    }
  } 
}

Or we could have a function like include: string[] | (ssr: boolean) => string[], and also exclude, esbuildOptions. Both look ok to me, leaning towards a general ssr function at the optimizeDeps level that seems to be the most flexible one.

What do you think? I would implement this in another PR, and it is a feature we could add later also.

Did another run of vite-ecosystem-ci and the svelte tests are still failing with the same error: https://github.com/vitejs/vite-ecosystem-ci/runs/7171919769?check_suite_focus=true

Maybe optimizing during SSR shouldn't be shared at all? And if we don't get the functional form in optimizeDeps it means that it only applies for the browser?
This feels safer, first because of how the ecosystem has been using optimizedDeps, and second because both envs are quite different and you may not want to optimize in the same way.

@patak-dev
Copy link
Member Author

patak-dev commented Jul 3, 2022

The function form could cause issues in the ecosystem, as there are a lot of plugins that are expecting an object only there. I think it would be better to keep things as objects. We could do:

optimizeDeps: {
  ...
},
ssr: {
  optimizeDeps: {
    ...
  },
  ...
}

That I think is better compared to optimizeDeps.ssr because we can start a pattern of centralizing everything related to ssr under the same entry, instead of having several ssr objects around the configuration.

Since we are discussing this API, @DylanPiercey proposed a few days ago having something similar using overrides:

  optimizeDeps: {
    include: ['stuff included in all targets'],
    exclude: ['stuff excluded in all targets'],
    overrides: {
      node: {
        include: ['only included for node']
      },
      webworker: {}.
      browser: {}
    }
  }

I think that if we want something like this, it is already better to use multiple config files. We need the entry in ssr because during dev, we only have one config.

@patak-dev
Copy link
Member Author

patak-dev commented Jul 4, 2022

We discussed with @bluwy that we should merge to continue to build on top of this PR. There is an issue in vite-plugin-svelte with this but looks like it is on the plugin side. And it may be resolved once we correct the noExternal/optimizeDeps logic in a PR I'll send next

@patak-dev patak-dev merged commit 188f188 into main Jul 4, 2022
18 checks passed
@patak-dev patak-dev deleted the feat/optimize-deps-strategy branch Jul 4, 2022

// Add deps found by the scanner to the discovered deps while crawling
for (const dep of scanDeps) {
if (!crawlDeps.includes(dep)) {
Copy link

@Veath Veath Jul 15, 2022

Choose a reason for hiding this comment

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

Excuse me, why does the scanner dependency not exist in the crawling discovered dependency

Copy link
Member

@bluwy bluwy Jul 16, 2022

Choose a reason for hiding this comment

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

This line checks that when we crawl for dependencies, those that are in scanDeps (already optimized), shouldn't be added as a missing import since it's already optimized.

Copy link

@Veath Veath Jul 18, 2022

Choose a reason for hiding this comment

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

There is one point that I don't quite understand.

Dependencies found during the scan phase will be stored in metadata.discovered, and then runOptimizeDeps will generate result metadata.optimized(scanDeps). In the craw phase, crawlDeps should contain all the dependencies of the scan phase (scanDeps). Think this conditional statement will not hit, right?

Copy link
Member

@bluwy bluwy Jul 21, 2022

Choose a reason for hiding this comment

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

Sorry for the late reply. I think it's a bit out of my territory now since I haven't been following up on this part of the code. Maybe @patak-dev knows more about this. Also, are you suggesting of a perf improvement, or a bug?

Copy link
Member Author

@patak-dev patak-dev Jul 21, 2022

Choose a reason for hiding this comment

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

With the current code, it could still hit, but I think you have a point @Veath. We should move

const crawlDeps = Object.keys(metadata.discovered)

after the await of the scanner
const result = await postScanOptimizationResult

and then it should never hit. PR welcome if you would like to tackle that

Copy link

@Veath Veath Jul 22, 2022

Choose a reason for hiding this comment

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

I think this part of the code can be deleted.

for (const dep of scanDeps) {
if (!crawlDeps.includes(dep)) {
addMissingDep(dep, result.metadata.optimized[dep].src!)
}
}

I understand it this way. The scan phase found foo, bar dependencies. A new dependency C is found during the crawling phase, then at the end of the crawling, it should be scanDeps = [foo,bar], crawlDeps = [foo,bar,C].

The precondition for executing this part of the code is scanDeps = [foo, bar], crawlDeps = [C], Will there be such a scene? @patak-dev

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat: deps optimizer Esbuild Dependencies Optimization p3-significant 🔨 High priority enhancement (priority)
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

optimizeDeps does not process aliased imports
4 participants