Skip to content

fix(resolve): fix resolve cache to consider conditions and more#18302

Merged
sapphi-red merged 10 commits into
vitejs:mainfrom
hi-ogawa:fix-refine-resolved-cache
Oct 10, 2024
Merged

fix(resolve): fix resolve cache to consider conditions and more#18302
sapphi-red merged 10 commits into
vitejs:mainfrom
hi-ogawa:fix-refine-resolved-cache

Conversation

@hi-ogawa
Copy link
Copy Markdown
Contributor

@hi-ogawa hi-ogawa commented Oct 8, 2024

Description

I simply expanded the cache key of "resolved cache", but I'm not sure if just concatenating everything is the best way in terms of performance.

todo

@bolt-new-by-stackblitz
Copy link
Copy Markdown

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

@hi-ogawa hi-ogawa marked this pull request as ready for review October 8, 2024 10:13
@hi-ogawa
Copy link
Copy Markdown
Contributor Author

hi-ogawa commented Oct 9, 2024

I created a bench script with server.pluginContainer.resolveId("some-package") loop. hi-ogawa/reproductions#41

Negative perf impact on resolvePackageEntry seems clear, but I'm not sure if there's a way to mitigate this. Can this per hit be considered acceptable?

cpuprofile viz

beta: CPU.20241009.130606.40833.0.001.cpuprofile
image

this pr: CPU.20241009.130349.39525.0.001.cpuprofile
image

Comment thread packages/vite/src/node/packages.ts
patak-cat
patak-cat previously approved these changes Oct 9, 2024
Copy link
Copy Markdown
Member

@patak-cat patak-cat left a comment

Choose a reason for hiding this comment

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

I think we should move forward with this fix for now. In the future, we could explore moving the resolve cache to the environment instance and pass it to the resolve plugin options.

@patak-cat patak-cat added the p3-minor-bug An edge case that only affects very specific usage (priority) label Oct 9, 2024
@hi-ogawa
Copy link
Copy Markdown
Contributor Author

hi-ogawa commented Oct 10, 2024

Maybe a simpler approach to split the cache for environment is to split PackageCache, which I think is currently shared at base ResolvedConfig.packageCache.

I was testing css @import which uses style condition and I thought it would have the same issue, but it didn't on main. This made me wonder it's because css resolver doesn't use packageCache.

sapphi-red
sapphi-red previously approved these changes Oct 10, 2024
Copy link
Copy Markdown
Member

@sapphi-red sapphi-red left a comment

Choose a reason for hiding this comment

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

I tested removing the whole resolvedCache but it seems that degrades perf by ~10%. It seems we need to keep this.

I also tested this branch with #16631. The issue is now fixed and server.ssrLoadModule resolves to dist/emotion-react.cjs.mjs even after server.pluginContainer.resolveId is called.

@hi-ogawa
Copy link
Copy Markdown
Contributor Author

I was thinking we can instantiate a dedicated config.packageCache for each environment by proxying it here. Build is working but dev seems to have some issues. Is this approach supposed to work or packageCache should be still singleton?

this.config = new Proxy(
options as ResolvedConfig & ResolvedEnvironmentOptions,
{
get: (target, prop: keyof ResolvedConfig) => {
if (prop === 'logger') {
return this.logger
}
if (prop in target) {
return this._options[prop as keyof ResolvedEnvironmentOptions]
}
return this._topLevelConfig[prop]
},
},
)

@hi-ogawa
Copy link
Copy Markdown
Contributor Author

I made environment.config.packageCache approach in a separate PR #18319. This seems like a sensible solution, but I need to double check if we can replace currently usages of config.packageCache nicely.

Copy link
Copy Markdown
Member

@patak-cat patak-cat left a comment

Choose a reason for hiding this comment

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

As proposed by @hi-ogawa in the linked PR, we could move forward with this one to get the fix in, and then rework in a future PR to optimize it. @sapphi-red I'll let you merge it if you are ok with this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

p3-minor-bug An edge case that only affects very specific usage (priority)

Projects

None yet

3 participants