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

No need to run completion handler in async thread pool #2605

Merged
merged 2 commits into from
Apr 20, 2023

Conversation

testforstephen
Copy link
Contributor

Currently we use computeAsync and join the async result to handle completion and resolveCompletionItem requests synchronously. This is not quite efficient. A better implementation is to run them directly in the main dispatcher thread, which can ensure the completion is handled immediately and without waiting for another async scheduler.

@fbricon
Copy link
Contributor

fbricon commented Apr 18, 2023

Can you measure the alleged performance improvement?

@testforstephen
Copy link
Contributor Author

Can you measure the alleged performance improvement?

This PR improves the responsiveness of the completion feature by reducing thread contention. The performance benefit may vary depending on how busy the thread pool is.

Previously, the completion handler used computeAsync to spawn a new thread for completion task from a common thread pool, which was shared by many other requests (such as semantic highlighting, code actions, inlay hints, folding range, delegate commands, etc.). Meanwhile, the completion handler used result.join() to block the main jsonrpc thread. This resulted in two threads being occupied by a single completion task, which was wasteful of thread resources.

https://github.com/eclipse/eclipse.jdt.ls/blob/73ccd84d4a0f57312ece7c049c27c6e685a889fe/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/handlers/JDTLanguageServer.java#L592-L603

This PR changes the completion implementation to run completion task in main jsonrpc thread directly. This reduces the number of threads involved in each completion task from two to one, and also avoids competing with other requests for the common thread pool.

The performance benefit depends on how busy the thread pool is. If the pool is idle, the scheduling time for computeAsync is about 2 ms in my machine. But if the pool is busy, the scheduling time is unpredictable.

@jdneo
Copy link
Contributor

jdneo commented Apr 19, 2023

test this please

Copy link
Contributor

@jdneo jdneo left a comment

Choose a reason for hiding this comment

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

LGTM.

If it's on purpose to block the LSP request during completion. Then it's unnecessary to schedule it to the thread pool and join from the dispatch thread.

@testforstephen
Copy link
Contributor Author

If it's on purpose to block the LSP request during completion. Then it's unnecessary to schedule it to the thread pool and join from the dispatch thread.

Yes, it's on purpose. This synchronous join behavior is from this PR #425.

@testforstephen testforstephen added this to the End April 2023 milestone Apr 20, 2023
@testforstephen testforstephen merged commit 4a1695a into eclipse-jdtls:master Apr 20, 2023
6 checks passed
@testforstephen testforstephen deleted the jinbo_completion branch April 20, 2023 01:48
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.

None yet

3 participants