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

MyCompletionItem is expensive #95324

Closed
jrieken opened this issue Apr 15, 2020 · 6 comments
Closed

MyCompletionItem is expensive #95324

jrieken opened this issue Apr 15, 2020 · 6 comments
Assignees
Labels
bug Issue identified by VS Code Team member as probable bug javascript JavaScript support issues perf typescript Typescript support issues verified Verification succeeded
Milestone

Comments

@jrieken
Copy link
Member

jrieken commented Apr 15, 2020

This is a follow-up from microsoft/TypeScript#36265 (comment)

Screenshot 2020-04-15 at 13 10 55

Also, it calls getWordRangeAtPosition after completions have arrived and therefore on a potentially different state, with different words etc. This should be all before

@jrieken
Copy link
Member Author

jrieken commented Apr 15, 2020

related to #95325

@mjbvz
Copy link
Contributor

mjbvz commented Apr 15, 2020

getReplaceRange

private getReplaceRange(line: string) {
is the most expensive part of this call

Now that microsoft/TypeScript#29270 is fixed, I believe I can just disable this branch for TS 3.9+

mjbvz added a commit that referenced this issue Apr 15, 2020
For #95324

- Removes repeated calls to `getWordRangeAtPosition`
- Only use our fuzzy string logic when using TS 3.8 or older
@mjbvz mjbvz added the bug Issue identified by VS Code Team member as probable bug label Apr 15, 2020
@mjbvz mjbvz added this to the April 2020 milestone Apr 15, 2020
@mjbvz mjbvz added javascript JavaScript support issues typescript Typescript support issues labels Apr 15, 2020
mjbvz added a commit that referenced this issue Apr 15, 2020
For #95324

- Get word range before we make the TS request
- Reuse the context
@mjbvz
Copy link
Contributor

mjbvz commented Apr 15, 2020

In a empty js file, I now see the TS extension's completion conversion take around 25ms (down from around 50ms on my machine):

Screen Shot 2020-04-15 at 2 20 56 PM

@jrieken Let me know if you notice any other obvious optimization opportunities.

@mjbvz mjbvz closed this as completed Apr 15, 2020
@mjbvz mjbvz added the perf label Apr 15, 2020
@jrieken jrieken reopened this Apr 16, 2020
@jrieken
Copy link
Member Author

jrieken commented Apr 16, 2020

Look at this: The memoize util is responsible for 15-20% of the time it takes to convert completion items.

Screenshot 2020-04-16 at 12 46 26

You are better off nuking that using a good old field. I also believe (measure, this used to be true is the past when I measured that) that computation is faster if you use static maps instead of switch-case-statements. So, have staticMap[entryKind] instead of the switch-case (the same would then be true convertKind)

@mjbvz mjbvz closed this as completed in c189b2b Apr 16, 2020
@mjbvz
Copy link
Contributor

mjbvz commented Apr 16, 2020

Good catch. Removing memo does improve _convert

The static map performs the same (or perhaps slightly worse) than the switch. I tested it by looping though a completion list 1000 times and converting the kind of every entry in it

@jrieken
Copy link
Member Author

jrieken commented Apr 17, 2020

Thanks

@jrieken jrieken added the verified Verification succeeded label Apr 28, 2020
@github-actions github-actions bot locked and limited conversation to collaborators May 31, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Issue identified by VS Code Team member as probable bug javascript JavaScript support issues perf typescript Typescript support issues verified Verification succeeded
Projects
None yet
Development

No branches or pull requests

2 participants