Skip to content

Delay revalidation and handle validation cancellation correctly#252

Merged
angelozerr merged 1 commit intoeclipse-lsp4mp:masterfrom
datho7561:reduce-thread
Jun 15, 2022
Merged

Delay revalidation and handle validation cancellation correctly#252
angelozerr merged 1 commit intoeclipse-lsp4mp:masterfrom
datho7561:reduce-thread

Conversation

@datho7561
Copy link
Copy Markdown
Contributor

@datho7561 datho7561 commented Jun 13, 2022

Delays revalidating after an edit is made in order to reduce the number of validations run. Handles validation, hover, completion, and definition cancellation better by checking if the request is cancelled between expensive steps.

See eclipse-lemminx/lemminx@420076e, eclipse-lemminx/lemminx@974099d

Signed-off-by: David Thompson davthomp@redhat.com

@datho7561 datho7561 marked this pull request as ready for review June 13, 2022 18:43
@datho7561
Copy link
Copy Markdown
Contributor Author

@angelozerr when you get a chance, could you please look at this?

Comment thread javaConfig.json Outdated
@angelozerr
Copy link
Copy Markdown
Contributor

angelozerr commented Jun 14, 2022

Your PR looks promising but the main idea is to get th eproperty model at first (instead of getting the project info cache)

For instance completion should start like this:

return getPropertiesModel(params.getTextDocument(), (document, cancelChecker)

and the project cache should be used after.

Why? Because the cancel checker that you will receive will be bind with the cancel of the client. All features like hover, completion, etc must return return getPropertiesModel(params.getTextDocument(), (document, cancelChecker) to receive the proper cancel checker bind with the cancel of client. Ex: if the user hover someting and move the mous in an another location, hover will be cancelled and your cancel checker will be bind with client cancel and it will cancel correctly.

For hover I will try:

@Override
public CompletableFuture<Hover> hover(HoverParams params) {
	 // Get the Properties model document
	return getPropertiesModel(params.getTextDocument(), (document, cancelChecker) -> {
		// Get MicroProfile project information which stores all available MicroProfile
		// properties
		MicroProfileProjectInfoParams projectInfoParams = createProjectInfoParams(params.getTextDocument());			
		MicroProfileProjectInfo projectInfo = getProjectInfoCache().getProjectInfo(projectInfoParams).getNow(null);
		// then return hover by using the MicroProfile project information and the
		// Properties model document
		return getPropertiesFileLanguageService().doHover(document, params.getPosition(), projectInfo,
				sharedSettings.getHoverSettings());
	});
}

BUT it means that projectInfo can be null, and we will need tocheck that in the doHover. I think we should do that in the doHover because some hover don't need projectInfo (ex : hover for expression to evaluate it).

For completion if projectInfo is every time required, we could check that before calling the doComplete of language service.

It seems that you take care of only properties file. It should be nice to do the same thing for Java file, but we can do that in a separate PR if you wish.

@datho7561 datho7561 force-pushed the reduce-thread branch 2 times, most recently from 66eff36 to d991abb Compare June 15, 2022 14:50
Delays revalidating after an edit is made in order to reduce the number of validations run.
Handles validation, hover, completion, and definition cancellation
better by checking if the request is cancelled between expensive steps.

See
eclipse-lemminx/lemminx@420076e,
eclipse-lemminx/lemminx@974099d

Signed-off-by: David Thompson <davthomp@redhat.com>
@datho7561
Copy link
Copy Markdown
Contributor Author

@rgrunber I tested completion on a fresh Quarkus project, and instead of blocking while the properties are loading, it instead returns no completion results until the properties are fully loaded.

@angelozerr angelozerr merged commit 1d03775 into eclipse-lsp4mp:master Jun 15, 2022
@angelozerr
Copy link
Copy Markdown
Contributor

Great job @datho7561 !

@angelozerr angelozerr added this to the 0.5.0 milestone Jun 15, 2022
@datho7561 datho7561 deleted the reduce-thread branch June 15, 2022 15:25
@rgrunber
Copy link
Copy Markdown
Contributor

Yup, definitely a nice improvement. The completion returns immediately now and results are populated as expected once projectInfo completes.

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.

3 participants