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 perf of internal resolve plugin #12363

Closed
4 tasks done
patak-dev opened this issue Mar 10, 2023 · 9 comments
Closed
4 tasks done

Improve perf of internal resolve plugin #12363

patak-dev opened this issue Mar 10, 2023 · 9 comments
Labels
p4-important Violate documented behavior or significantly improves performance (priority) performance Performance related enhancement

Comments

@patak-dev
Copy link
Member

Description

We need to follow Node resolution and also take into account resolve.extensions. Right now a single resolve call could end up exploding into many file system checks.

Creating an umbrella issue to discuss ways to improve the plugin. There are other possibly related issues:

Suggested solution

To be determined.

Alternative

No response

Additional context

No response

Validations

@patak-dev patak-dev added p4-important Violate documented behavior or significantly improves performance (priority) performance Performance related enhancement labels Mar 10, 2023
@sapphi-red
Copy link
Member

sapphi-red commented Mar 22, 2023

Some random ideas:

@bluwy
Copy link
Member

bluwy commented Mar 22, 2023

Yeah I think it'll be nice if we can get this fixed for windows. I saw you made a fix before to resolve Windows drive relative paths. It would need some testing to get it work, but I don't have a Windows laptop.

  • negative cache for package.json look up

Yeah I think this is implemented in #12512, in a way. Since in most case we will eventually find a package.json somewhere else, the directories traversed to reached there are all cached too so that future lookup is faster.

  • using package manager's knowledge

I think it might be good to have Vite be package manager agnostic for now, so future pms like Bun (which saves the lockfile in binary format) will work. Maybe using lockfiles that we understand could help fast-path things, but the idea of JSON.parse or yaml-parsing them is a bit scary 😄

@fi3ework
Copy link
Member

using package manager's knowledge

Relying on the knowledge of dependency package managers can be troublesome, and we cannot guarantee that the lockfile is up-to-date. Additionally, Yarn 1 does not guarantee a topological structure. 🤔

@sapphi-red
Copy link
Member

I think it might be good to have Vite be package manager agnostic for now ... but the idea of JSON.parse or yaml-parsing them is a bit scary 😄

Relying on the knowledge of dependency package managers can be troublesome, ...

Yeah, I agree with that. I think the ability to skip the FS traverse has great potential but better to be sought as a plugin.

@fi3ework fi3ework reopened this Mar 22, 2023
@fi3ework
Copy link
Member

Sorry for the misoperation 😅. I also have a speculative idea: is it possible to pre-scan all file structures we need one time and maintained them in memory? Could that reduce the JS/C++ communication overhead to minimal time as we can assume node_modules is immutable. 🤔

@hardfist
Copy link
Contributor

maybe you can try https://github.com/web-infra-dev/nodejs_resolver which we do lots of optimization

@bluwy
Copy link
Member

bluwy commented Mar 23, 2023

is it possible to pre-scan all file structures we need one time and maintained them in memory? Could that reduce the JS/C++ communication overhead to minimal time as we can assume node_modules is immutable. 🤔

I think that will trade more RAM instead which could be a problem for lower-end machines. We do already cache some crucial files in node_modules through the optimizer, which I think is a step towards it. Caching for source code is already somewhat there with Vite's module graph too.

maybe you can try https://github.com/web-infra-dev/nodejs_resolver which we do lots of optimization

It would be interesting to try that out. Right now Vite mixes a lot optimizer logic within it's node resolver, which could be refactored out.

I'm also concerned of the install size of using native binaries though. IMO it would need a significant improvement to justify the size, that's one of the reason (for me) that @parcel/watcher isn't worth it.

@sapphi-red
Copy link
Member

Perhaps this ensureEntryFromUrl call is doubling the resolveId call.

await moduleGraph?.ensureEntryFromUrl(options.id)

const resolved = await this.resolveId(url, !!ssr)

Since we know the id is already resolved, maybe we can skip resolveId? (I didn't find a good way to implement this without introducing a breaking change...)

@patak-dev
Copy link
Member Author

I think we can close this issue for now. Resolving perf in Vite 4.3 has greatly improved. We can continue looking for opportunities to speed up things further but I dont think this umbrella issue is needed now.

@github-actions github-actions bot locked and limited conversation to collaborators Apr 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
p4-important Violate documented behavior or significantly improves performance (priority) performance Performance related enhancement
Projects
None yet
Development

No branches or pull requests

5 participants