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: auto import cache #2237

Merged
merged 15 commits into from
Jan 11, 2024
Merged

Conversation

jasonlyu123
Copy link
Member

@jasonlyu123 jasonlyu123 commented Dec 21, 2023

#2232 #2193

Implementing TypeScript's internal APIs for cache with the help of Volar's typescript-auto-import-cache. It also adopted the packag.json auto-import added in TS 4.0 for TSServer. The package.json auto-import has some limitations, though. It only works for dependencies and not dev dependencies. And it seems like the first dependencies also don't work until a server restarts. This also happens in ts/js files.

For implementation, the original text-based package.json cache was removed and now relies on typescript-auto-import-cache and ts.ModuleResolutionCache for the cache, and we only watch the files for invalidation.

The reason I didn't use the default API of typescript-auto-import-cache is that there is some memory leak with the implementation. One is that an old program object is stored, and the other is that there is no API to clear the project object stored in a map during the tsconfig update. The former is a bit hard to fix without changing the public API. But we definitely will need to try to resolve this when we eventually migrate to Volar. The projectService object also caused problems with the virtual file system test because it is a singleton with the ts.sys passed in earlier. It should not be a problem if we handle this by ourselves.

Although migrating to Volar, which also uses typescript-auto-import-cache, also improves the auto import performance, it seems like we'll still need to put a lot of work into migrating existing post-processing and extra features for Volar. So, it probably still makes sense to add this now.

@jasonlyu123 jasonlyu123 added the Blocked Cannot proceed until something else is resolved label Dec 21, 2023
@jasonlyu123 jasonlyu123 removed the Blocked Cannot proceed until something else is resolved label Dec 29, 2023
@jasonlyu123 jasonlyu123 marked this pull request as ready for review January 4, 2024 03:45
Copy link
Member

@dummdidumm dummdidumm left a comment

Choose a reason for hiding this comment

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

Impressive work, left a few comments

return undefined;
}

const factory = minor < 3 ? createProject50 : createProject53;
Copy link
Member

Choose a reason for hiding this comment

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

This makes me a bit nervous that in the future we potentially need to adjust this for new TS versions - but I guess there's no other way around this since this is using internal methods. Ideally the cache becomes part of the TS service at some point...

Copy link
Member Author

@jasonlyu123 jasonlyu123 Jan 10, 2024

Choose a reason for hiding this comment

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

Yeah. This might break in the future. Maybe it'll be better to let the typescript-auto-import-cache handle it, but there is the integration problem I mentioned above. I spoke with @/johnsoncodehk, but he's not sure if the new API we added now in typescript-auto-import-cache can be available later since the potential changes in the future and with Volar 2.

Copy link
Member

Choose a reason for hiding this comment

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

Just to be sure: This doesn't mean that typescript-auto-import-cache isn't going away at some point, it's just that the APIs we need on top of it cannot be built into the package itself right now because of the potential Volar 2 changes.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah. Not saying the package is going away. I checked the chat history. What he said is that he might "bundle" the code responsible for the cache.

Comment on lines 240 to 241
// Just in case, if there is a parser error. Force ts to not use the cache
triggerKind: tsDoc.parserError ? undefined : completionContext?.triggerKind
Copy link
Member

Choose a reason for hiding this comment

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

Does this mean we got no advantage from this change in broken states, e.g when being in the middle of a template completion like <Comp?

Copy link
Member Author

Choose a reason for hiding this comment

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

I was wrong. The incompletion cache requires another internal API. And it seems only rarely to trigger. So I think it's not worth the effort to add it now. Maybe we can add it later or add it to Volar when we switch.

In the module resolution "node", TypeScript only loads 100 of them and marks the completion as incomplete. That's when the incomplete cache can be handy. The completion is reused and only resolves more completion label detail, the faded module specifier text after the completion label. But In most of the cases I tested, It only marks it as incomplete in global completion, and most of the time, the first letter can already narrow the number down.

In module resolution node16+ and bundler, TypeScript will check all the module specifiers in auto import. It is probably to filter out auto import blocked by the export map. And create-svelte recently switched to bundler by default. That explains why, all of a sudden, many people started to complain about slow completion.

In the case of <Comp. Do you remember why we'll reuse the completion in case of triggerKind === CompletionTriggerKind?TriggerForIncompleteCompletions? It seems like we'll reuse the completion in this situation before asking ts for a new list. Is that the intended behaviour?

Copy link
Member

@dummdidumm dummdidumm Jan 11, 2024

Choose a reason for hiding this comment

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

I think it was intentional because completion is rather expensive in this situation and if people type one character after the other they should be able to narrow down the existing completions.
Is this resulting in undesired behavior now?

Re "TS checks this more thoroughly now" - is there some inefficiency you've seen on the TS side which we could report upstream? Or is it unavoidable additional work that needs to be done to be more correct with the export conditions?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it's mostly unavoidable. If there is a cache, it at least only happens the first time and after various caches are invalidated. #2244 might have something to optimize on the TypeScript side, but I think It is an edge case. I'm not sure if there is something less extreme and also having performance issues.

seems like IncompleteCompletions only trigger very rarely and it require another internal API to work
keeping allowIncompleteCompletions since it will also make completion label detail shows up in more cases
@dummdidumm dummdidumm merged commit e1eacce into sveltejs:master Jan 11, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants