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 template interpolation features performance when no changes to the file #2642

Open
1 task done
jasonlyu123 opened this issue Jan 21, 2021 · 6 comments
Open
1 task done

Comments

@jasonlyu123
Copy link
Contributor

  • I have searched through existing issues

Feature Request

Hi, First of all, thanks for the great works you guys put into it. The template interpolation feature really makes the developer experience way better and it's a good boost to productivity. But the performance is really not very good sometimes. I would like to help out and make it better.

I'll focus on the performance of template interpolation features performance when no changes to the file in this issue. That is features like hover, completion resolve, go to definition, and find references. Especially the second request. I have done some investigation using my local clone version. Here's what I found out and think can make the performance better:

The problems

vueInfoService.getInfo

return this.vueFileInfo.get(getFileFsPath(doc.uri));

This method is very demanding it often took more than 500ms on my machine. Besides the performance of this method, what strange to me is that the vueFileInfo map here seems like a kind of cache but it'll always get updated when getInfo is called. Furthermore, the stored info doesn't get removed when the document is closed or deleted in the file system, so it's a sort of memory leak. I think we could remove the map here.

const defaultExportType = checker.getTypeAtLocation(defaultExportNode);

The most demanding part of vueInfoService.getInfo seems to be this line. Analyzing the type afterward isn't that demanding. I notice typescript does cache the type checker result here. When typescript cache the type it runs significantly faster. So the problem is mainly on why typescript won't use the cached version. Once that's resolved. Most of the features that use this method might have a good boost in performance.

Auto import, on the other hand, the implementation makes the script version mismatch on every resolve request. Thus typescript would always analyze it again. We probably need to find another way to improve it. I wonder if we can make the mockDoc here different than the actual script doc.

serviceHost.updateCurrentVirtualVueTextDocument

if (isVirtualVueTemplateFile(fileFsPath)) {

The version of the virtual vue file bumps whenever the function is called. This makes typescript analyze the type again. It seems to also affect the language service the script section used. And it seems to be the main problem of the interpolation performance. Once I commented out the version bump, most of the feature is way faster. I think we can use the version of TextDocument to avoid bumping when not necessary. But always update ChildComponentsInfo in case the dependent component is updated.

TL;DR

From my understanding/investigation, the main problem is that the version of the virtual vue file in the ServciceHost bumps every time. I think we should use the TextDoucment.version to check if we should bump the version. So that typescript won't do the type analysis again. And the vueFileInfo map in the vueInfoService doesn't get used and seems like it can be removed.

Feel free to point out what I missed or what I understand incorrectly. I can help to implement what I mention above. But I think it would be better to have an issue to discuss it first.

@yoyo930021
Copy link
Member

Thanks for your research.

If you can provide a profile, we can study it more simply.
https://github.com/vuejs/vetur/blob/master/.github/PERF_ISSUE.md

@yoyo930021
Copy link
Member

yoyo930021 commented Jan 21, 2021

This method is very demanding it often took more than 500ms on my machine. Besides the performance of this method, what strange to me is that the vueFileInfo map here seems like a kind of cache but it'll always get updated when getInfo is called. Furthermore, the stored info doesn't get removed when the document is closed or deleted in the file system, so it's a sort of memory leak. I think we could remove the map here.

It's a interesting discovery. It runs fast on my computer every time. If you can provide more information like profile, os, hardware, typescript version, we can study it.
I don't think this is a mistake about doesn't get removed when the document is closed or deleted in the file system. It is actually useful to keep this information for many situations. I don't think this will affect the performance either.

We may later abandon our current method. Change to simply use the typescript definition. #2344

The most demanding part of vueInfoService.getInfo seems to be this line. Analyzing the type afterward isn't that demanding. I notice typescript does cache the type checker result here. When typescript cache the type it runs significantly faster. So the problem is mainly on why typescript won't use the cached version. Once that's resolved. Most of the features that use this method might have a good boost in performance.

More in-depth analysis may be required. I did not touch this part in my previous research.

Auto import, on the other hand, the implementation makes the script version mismatch on every resolve request. Thus typescript would always analyze it again. We probably need to find another way to improve it. I wonder if we can make the mockDoc here different than the actual script doc.
The version of the virtual vue file bumps whenever the function is called. This makes typescript analyze the type again. It seems to also affect the language service the script section used. And it seems to be the main problem of the interpolation performance. Once I commented out the version bump, most of the feature is way faster. I think we can use the version of TextDocument to avoid bumping when not necessary. But always update ChildComponentsInfo in case the dependent component is updated.

I expect to need a major refactoring on this part. We are encountering more and more need to customize typescript file feature. This side needs to be changed to be more flexible to allow for needs like this feature. It is expected that the issue will be handled during the long holidays.

From my understanding/investigation, the main problem is that the version of the virtual vue file in the ServciceHost bumps every time. I think we should use the TextDoucment.version to check if we should bump the version.

You can indeed add the appropriate cache here. Perhaps it can also be changed to incremental updates.

So that typescript won't do the type analysis again. And the vueFileInfo map in the vueInfoService doesn't get used and seems like it can be removed.

Don't really understand your idea.

Feel free to point out what I missed or what I understand incorrectly. I can help to implement what I mention above. But I think it would be better to have an issue to discuss it first.

Your contribution is welcome! Need more input to be better.

@jasonlyu123
Copy link
Contributor Author

jasonlyu123 commented Jan 21, 2021

I don't think this is a mistake about doesn't get removed when the document is closed or deleted in the file system. It is actually useful to keep this information for many situations. I don't think this will affect the performance either.

Yeah, no solid evidence here but the fact that the extension is getting slower over time makes me think it's so what to do with memory.

What I mean is like this. As I didn't see it get used in other places.
https://github.com/jasonlyu123/vetur/blob/37374ef3d20552b0f358f569a00d797fb9c3183b/server/src/services/vueInfoService.ts#L94

But I think a bigger problem about memory when a file is deleted is probably the typescript source file object and script snapshot object in the serviceHost.

@jasonlyu123
Copy link
Contributor Author

It is actually useful to keep this information for many situations.

If you're talking about the language features for a deleted file when it's still open. Or the language feature of other files that depend on a closed document. Those won't be affected.

Since you're saying that the auto-import would be refactored later. I'll just wait for it and see what can I help with.

@yoyo930021
Copy link
Member

But I think a bigger problem about memory when a file is deleted is probably the typescript source file object and script snapshot object in the serviceHost.

I think it's necessary because they are interdependent.
VSCode's built-in TS support does exactly the same thing.

Provide a profile, we can study it more simply.
https://github.com/vuejs/vetur/blob/master/.github/PERF_ISSUE.md
I have not yet encountered the new version of the slow problem.

The parts I expect to refactor:

  • New TS files can be added efficiently in TypeScript language service.
  • Most unnecessary updates can be reduced.
  • Provide syntax mapping to TS files.

@jasonlyu123
Copy link
Contributor Author

jasonlyu123 commented Jan 27, 2021

Sorry, I can't get the profiler working.
The project I encounter slow performance is a v2 vue typescript project with composition API and typescript strict mode enabled. Some of the components need more than 1s for go to definition. I'll see if I can provide a simple repro. But I guess it's just the typing being too complex so it took quite some time for typescript to fully analyze the type.
With that said, once the typescript uses the cached type to process language feature, it really just run much faster. At least it's more responsive when reading the codebase.

@yoyo930021 yoyo930021 added this to the v0.36.0 milestone Oct 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants