perf: auto import cache #2237
perf: auto import cache #2237
Conversation
fixing virtual fs test and memory leak problems
| return undefined; | ||
| } | ||
|
|
||
| const factory = minor < 3 ? createProject50 : createProject53; |
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| // Just in case, if there is a parser error. Force ts to not use the cache | ||
| triggerKind: tsDoc.parserError ? undefined : completionContext?.triggerKind |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
jasonlyu123
force-pushed
the
auto-import-cache
branch
from
e36f0e8
to
4620a28
Compare
#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
projectServiceobject also caused problems with the virtual file system test because it is a singleton with thets.syspassed 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.