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.

Sign up for GitHub

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

Jump to bottom

Improve marker resolution performance #154

Merged
merged 1 commit into from
Jun 23, 2022
Merged

Improve marker resolution performance #154

merged 1 commit into from
Jun 23, 2022

Conversation

rubenporras
Copy link
Contributor

by not connecting the file where we retrieve marker information. That is
an unnecessary step which can be expensive (for example the Xtext
language server rebuilds the source) and causes timeouts on
checkMarkerResoultion

@rubenporras rubenporras force-pushed the fixMarkerResolution branch 2 times, most recently from 6738ee1 to 9d5bece Compare June 22, 2022 14:49
@rubenporras rubenporras marked this pull request as draft June 22, 2022 15:12
mickaelistria
mickaelistria reviewed Jun 22, 2022
View reviewed changes
Copy link
Contributor

@mickaelistria mickaelistria left a comment

Choose a reason for hiding this comment

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

i like this idea a lot. We indeed often don't need to connect a file to get some features. I'm actually even curious when connecting a file is really necessary; apart from open/edit/close? Maybe we should never connect by default and only do it when necessary?

org.eclipse.lsp4e/src/org/eclipse/lsp4e/LanguageServiceAccessor.java Outdated Show resolved Hide resolved
org.eclipse.lsp4e/src/org/eclipse/lsp4e/LanguageServiceAccessor.java Outdated
}

public static @NonNull List<CompletableFuture<LanguageServer>> getInitializedLanguageServers(@NonNull IFile file,
@Nullable Predicate<ServerCapabilities> request, boolean connectFile) throws IOException {
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest forceConnectFile instead, as the file may already be connected. Also, do you have any suggestion what could be a better term than connect ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the review. I have done the changes.

You mean, a better name for LanguageServerWrapper#connect? I guess open would be a better name, in the end the effect of it is to created the document synchronizer which sends a didOpen notification, isn't it?. I do not know if it is worth renaming the methods now that they already exist.

@rubenporras rubenporras force-pushed the fixMarkerResolution branch 2 times, most recently from dfb25e2 to 432f259 Compare June 23, 2022 07:38
@rubenporras
Copy link
Contributor Author

I would also think one should only connect if it is needed, but I did not checked the codebase. This part of the codebase is a problem for us because in the markers view one can have many markers, and displaying them would send the didOpen for a lot of documents.

@rubenporras rubenporras marked this pull request as ready for review June 23, 2022 07:48
@rubenporras
Copy link
Contributor Author

From the last version of the PR it is clear that there is nothing in LSP4E that needs the side-effect of calling LanguageServerWrapper#connect when asking for initialized servers, so the easiest change would just be to remove the side-effect and leave the methods as they are.
It could be a breaking change if some other project relies on the side-effect. Is there a policy in LSP4E for that kind of changes? Is it something which can be done if it is documented, for example in the changelog?

@mickaelistria
Copy link
Contributor

so the easiest change would just be to remove the side-effect and leave the methods as they are.

Great, let's try it!

Could be a breaking change if some other project relies on the side-effect. Is there a policy in LSP4E for that kind of changes?

The policy has been to stay on 0.x stream to somehow emphasize their is no API policy ;)

@rubenporras
Improve marker resolution performance
dc5c7a3 
by not connecting the file where we retrieve marker information. That is
an unnecessary step which can be expensive (for example the Xtext
language server rebuilds the source) and causes timeouts on
checkMarkerResoultion.

The side-effect of connecting the document when getting an initialized
server has been removed from all the
LanguageServerAccessor#getInitializedLangugeServer* methods.

Downstream project which rely on this behaviour should connect the
document after getting the initialized server.
@mickaelistria mickaelistria merged commit 482738b into eclipse:master Jun 23, 2022
@rubenporras rubenporras deleted the fixMarkerResolution branch July 11, 2022 07:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mickaelistria mickaelistria mickaelistria left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@rubenporras @mickaelistria