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

Lazy resolve the new text of text edit #2642

Merged
merged 1 commit into from May 11, 2023

Conversation

jdneo
Copy link
Contributor

@jdneo jdneo commented May 9, 2023

  • When 'java.completion.lazyResolveTextEdit.enabled' turns on, the completion proposal will not resolve the text edit until 'completionItem/resolve'.
  • Because only the text is lazy resolved, while the range is available during textDocument/completion, this avoids the situation mentioned in delay resolve TextEdit of completion item #1868 (comment)

fix #1864

- When 'java.completion.lazyResolveTextEdit.enabled' turns on, the completion
  proposal will not resolve the text edit until 'completionItem/resolve'.

Signed-off-by: Sheng Chen <sheche@microsoft.com>
@jdneo
Copy link
Contributor Author

jdneo commented May 9, 2023

When completion after new S|

#1 #2 #3 #4 #5 #6 #7 #8 #9 #10 Avg.
without PR 3466 780 646 446 409 401 396 483 509 442 797.8
with PR 1144 477 375 329 339 264 261 256 255 256 395.6

When completion after S| (completion for type)

#1 #2 #3 #4 #5 #6 #7 #8 #9 #10 Avg.
without PR 271 136 113 121 97 94 98 92 92 95 120.9
with PR 222 93 106 78 65 65 69 62 78 62 90

@jdneo jdneo marked this pull request as ready for review May 9, 2023 07:31
Copy link
Contributor

@rgrunber rgrunber left a comment

Choose a reason for hiding this comment

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

This should be merged for the performance benefit.

However, I was just looking at the original issue which mentioned that toRequiredTypeEdit(..) -> computeTypeArgumentProposals(..) is roughly the part that takes longest. However, this change doesn't defer that call to resolve, and still does it on the initial textDocument/completion request.

I can confirm this improves performance though for new S| in spring-petclinic 🚀 :

Without PR (Med: 1182ms, Avg: 1372ms)
975ms 1015ms 1058ms 1064ms 1108ms 1182ms 1232ms 1134ms 1143ms 1705ms 3475ms
With PR (Med: 564ms, Avg: 693ms)
444ms 449ms 489ms 530ms 563ms 564ms 579ms 587ms 603ms 669ms 2638ms

Update: I guess deferring appendReplacementString is what really improves this case :

(without PR)
image

@rgrunber rgrunber merged commit 21c9a79 into eclipse-jdtls:master May 11, 2023
6 checks passed
@jdneo jdneo deleted the cs/lazy-resolve branch May 11, 2023 23:58
@jdneo
Copy link
Contributor Author

jdneo commented May 12, 2023

I guess deferring appendReplacementString is what really improves this case

Yes, it's true.

Thanks for mentioning that, it reminds me that there are still space to improve if we delay the toRequiredTypeEdit calculation.

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

Successfully merging this pull request may close these issues.

completion: calculating textEdit for constructor proposals is slow
3 participants