search

eclipse / lsp4e

Language Server Protocol support in Eclipse IDE
Eclipse Public License 2.0
60 stars 53 forks source link

[#932] prevent reconcile on every dirty region #933

Closed ghentschke closed 5 months ago

ghentschke commented 5 months ago

This change prevents reconcile run on every dirty region which can be many thousands in a large source file (> 5000 lines) Since the reconciler operate on the whole document it needed to be executed only when the file document changes.

fixes #932

rubenporras commented 5 months ago

I am not sure we not covering for some other fault with this PR.

For example:

Besides, will this PR not stop reconcilers from doing the job until the user saves the file? If so, I think that would be a regression.

ghentschke commented 5 months ago
  • the folding reconciler: it has a subregion as input parameter. Should we not reconcile only the changed regions? If that takes a bit too long, why is that a problem? Should reconcile not happen in a background thread?
  • the semantic reconciler: even thought we do not implement it yet, we could also reconcile only changed regions.

The problem/difficulty is, at least for clangd, a format with many modified lines (e.g. by changing the indent width) via LSP could result in a event queue with many thousands dirty regions. I've noticed that it takes very long (several minutes) until this queue has been processed in background threads (these thousands of changed regions could be separate issue as well). IMO performing the folding and the semantic highlighter on the whole document (as it is currently) has in many cases a better performance than processing each dirty region. But I'll check that.

Besides, will this PR not stop reconcilers from doing the job until the user saves the file? If so, I think that would be a regression.

IMO not, because DocumentUtil.getDocumentModificationStamp returns a new timestamp when the document gets edited.

rubenporras commented 5 months ago

DocumentUtil.getDocumentModificationStamp returns a new timestamp when the document gets edited.

DocumentUtil.getDocumentModificationStamp returns a new timestamp when the document gets edited but not when formats are applied using the formatter?

Is this desired behaviour or some corner case that has been forgoten?

ghentschke commented 5 months ago

DocumentUtil.getDocumentModificationStamp returns a new timestamp when the document gets edited but not when formats are applied using the formatter

No, it returns a new timestamp when the document has been modified. This includes also formatting changes. That was the problem here. The method: https://github.com/eclipse/lsp4e/blob/8df7e08de54f325a40835480e6fdf9a1cd281cfe/org.eclipse.lsp4e/src/org/eclipse/lsp4e/operations/folding/LSPFoldingReconcilingStrategy.java#L314 gets called on every dirty region. This can be several thousands in a large file with many formatting changes. I made this PR to prevent the repetitive call of the folding reconcile which operates on the whole document.

rubenporras commented 5 months ago

Okay, I see. Is the problem that the method void reconcile(IRegion subRegion) is not using the subRegion because the LSP has no option to only calculate the folding ranges for a subregion?

ghentschke commented 5 months ago

Okay, I see. Is the problem that the method void reconcile(IRegion subRegion) is not using the subRegion because the LSP has no option to only calculate the folding ranges for a subregion?

Yes

ghentschke commented 5 months ago

@rubenporras Thank you!