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

Improve the performance of LSPFoldingReconcilingStrategy.applyFolding #1182

Merged
merged 1 commit into from
Jan 15, 2025

Conversation

merks
Copy link
Contributor

@merks merks commented Jan 15, 2025

  • Avoid so many calls ProjectionViewer.postCatchupRequest by computing a map from Position to FoldingAnnotation for existing annotations.
  • Use that map to determine if a new annotation needs to be created.
  • LSPFoldingReconcilingStrategy.updateAnnotations is no longer required/used so delete it.
  • Make markInvalidAnnotationsForDeletion private because the signature has changed.

#1180

- Avoid so many calls ProjectionViewer.postCatchupRequest by computing a
map from Position to FoldingAnnotation for existing annotations.
- Use that map to determine if a new annotation needs to be created.
- LSPFoldingReconcilingStrategy.updateAnnotations is no longer
required/used so delete it.
- Make markInvalidAnnotationsForDeletion private because the signature
has changed.

eclipse-lsp4e#1180
@mickaelistria
Copy link
Contributor

Since the position may change (move 1 character left or right), would it be even more interesting to use Guava's RangeMap to find any overlapping good range to change?

@merks
Copy link
Contributor Author

merks commented Jan 15, 2025

You should run it under debug control. The position is computed from the document, it is not recorded in the annotation itself, so when you add some character, everything shifts and basically all this logic typically appears to be a big no-op...

This is the result adding a blank line at the beginning of a line:

image

@mickaelistria
Copy link
Contributor

OK, thanks!

@sebthom sebthom requested a review from rubenporras January 15, 2025 16:07
@rubenporras
Copy link
Contributor

I do not know this code well, would you mind doing the review @mickaelistria?

@mickaelistria
Copy link
Contributor

The general idea seems good; and I don't have the opportunity to go for detailed testing of the PR in a near future. However, I'm using snapshots build of everything all the time.
So I suggest we go for merging, and we'll see the outcome upon build ;)

@rubenporras
Copy link
Contributor

Okay, let us do that

@rubenporras rubenporras merged commit 2d515b2 into eclipse-lsp4e:main Jan 15, 2025
6 checks passed
@merks merks deleted the issue-1180 branch January 15, 2025 16:57
@merks
Copy link
Contributor Author

merks commented Jan 15, 2025

You guys are really responsive. I really appreciate that!

@sebthom
Copy link
Member

sebthom commented Jan 15, 2025

Thanks for the prompt fix!

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.

4 participants