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

Wait a bit after change before sending diagnostics #1162

Closed
mickaelistria opened this issue Jan 31, 2022 · 2 comments · Fixed by #1202
Closed

Wait a bit after change before sending diagnostics #1162

mickaelistria opened this issue Jan 31, 2022 · 2 comments · Fixed by #1202
Labels
performance This issue or enhancement is related to performance concerns
Milestone

Comments

@mickaelistria
Copy link
Contributor

In LSP4E, linkedEdit is implemented so that it sends 2 document events and so 2 didChange events (I know this is not perfect, but it's how Eclipse Platform currently does that and not something I can change immediately). As a consequence, typing one char in a tag sends 1 change, LemMinX immediately returns an error diagnostic for non-matching end tag, then LSP4E/Eclipse does fix the other tag and sends the event to LemMinX, that will then remove the error diagnostic. This cause the diagnostic to show and disappear immediately, almost blinking above code, it doesn't look nice.
Although it is something that could be fixed on Eclipse side, I'm wondering whether LemMinX should be more tolerant of fast changes. For example, someone typing really fast may not want to see the error as they're typing, but would like some delay (~50ms) before the diagnostics are reported. This would cause less "blinking diagnostics" and could also reduce the amount of parsing done by LemMinX: someone typing 10chars at 20ms/char would then see the error after ~250ms, 50ms after typing the last char, and LemMinX would have parsed the file only once instead of 10 times. I think the gain in UX is worth it, especially since it would be an opportunity to gain CPU cycles on server side.

@angelozerr
Copy link
Contributor

In LSP4E, linkedEdit is implemented so that it sends 2 document events and so 2 didChange events (I know this is not perfect, but it's how Eclipse Platform currently does that and not something I can change immediately).

vscode does the same thing :)

This cause the diagnostic to show and disappear immediately, almost blinking above code, it doesn't look nice.

I have not pay attention with that, but you are right there are the same problem with vscode. Thanks for reporting this issue!

Although it is something that could be fixed on Eclipse side, I'm wondering whether LemMinX should be more tolerant of fast changes.

+1

@rgrunber I think you had the same problem with JDT LS? Do you think it's possible to adopt your work from JDT LS in LemMinx?

@rgrunber
Copy link
Contributor

rgrunber commented Jan 31, 2022

Have a look at https://github.com/eclipse/eclipse.jdt.ls/blob/master/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/handlers/BaseDocumentLifeCycleHandler.java#L141 for the idea. We basically have a validation Job with delay (it used to be a fixed delay, now it's adaptive based on 1.5 x average validation time). When a new diagnostic request is triggered, any existing (and running) diagnostics job is cancelled, and the new one takes over. This avoids emiting diagnostics on every keystroke, which can cause issues. It also should improve performance by not hammering the LS for every keystroke.

See eclipse-jdtls/eclipse.jdt.ls#1973 for an idea on why we went with an adaptive delay over some fixed delay.

angelozerr added a commit to angelozerr/lemminx that referenced this issue May 19, 2022
angelozerr added a commit to angelozerr/lemminx that referenced this issue May 19, 2022
angelozerr added a commit to angelozerr/lemminx that referenced this issue May 19, 2022
angelozerr added a commit to angelozerr/lemminx that referenced this issue May 19, 2022
angelozerr added a commit that referenced this issue May 19, 2022
@angelozerr angelozerr added this to the 0.21.0 milestone May 19, 2022
@angelozerr angelozerr added the performance This issue or enhancement is related to performance concerns label Jun 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance This issue or enhancement is related to performance concerns
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants