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 "Replace all" performance - Find/Replace-->Replace All freezes UI on a large Java file. #2257

Closed
raghucssit opened this issue Sep 9, 2024 · 3 comments
Assignees
Labels
bug Something isn't working performance
Milestone

Comments

@raghucssit
Copy link
Contributor

Please use this class generator to GenerateJava.txt
Try to Replace All Hello World! text to any text Hello New!. UI freezes for 40 seconds.
The above text has 20k occurrences. If the size of the file increases by little over the UI freeze time increases even more.
Already there are discussions on this issue.
892.

The problem here is in 2 parts:

  1. Find and Select
  2. Replace the Selection

Discussions and proposals above were mainly based on 2. Replace the Selection.
I explored on 1. Find and Select.
Here also the problem is with the org.eclipse.jface.text.source.projection.ProjectionAnnotationModel.

Replace All works exhaustively by select then replace. Each select is very expensive in case of Projection Model because for each find it iterates over all the annotations to check if the annotation falls within the range and is Collapsed.Expanded.
In the generated example it has 20k occurrences of text and 40k annotations(Collapse/Expand blocks).
So it it does entire 40k iterations for 20k times even in base case scenario where all the nodes are expanded.
Issue is here
We can instead use Region Iterator which returns the Annotation Regions which are enclosed by given offset.

Other solutions which I tried but cannot be implemented due to limitations in the org.eclipse.jface.text.IFindReplaceTarget

  1. We should have a method to return just the Region of the text being selected. So that client can replace it and replace taks care of selection. Currently it redundant. Replace Selection
  2. We can also have method for Replace All. Basically Target Delegates find/replace to Text Viewer. If Target can tell Text Viewer that we have entered a batch mode like Replace All, It can do all the operation at once and send notifications once it is done.

My idea was to, In case of Replace All We can find the last occurrence of the text then expand all the regions at once and execute replaceSelection. ReplaceSelection takes care of Selection. For this to work I want an API which returns just Region(No Selection/Expansion/Reveal)

@raghucssit raghucssit added the bug Something isn't working label Sep 9, 2024
raghucssit added a commit to raghucssit/eclipse.platform.ui that referenced this issue Sep 9, 2024
For Large Java file 'Replace All' takes long time and freezes the UI.
One of the reason is Projection Model tries to iterate over all the
Projection Annotations to expand/collapse status.
We can improve this situation by using Region specific iterator. This
returns annotations which are enclosed by given offset.
This improves the performance by 25% at least.

See eclipse-platform#2257
@Wittmaxi
Copy link
Contributor

This issue is also interesting for a future overhaul of the IFindReplaceTargetExtensions. We should discuss this in-depth and make sure to either coordinate well or at least define the interfaces well so that we both have a good understand what is happening.

I might work on this in the future when I overhaul the FindReplaceOvelray anyway

iloveeclipse pushed a commit to raghucssit/eclipse.platform.ui that referenced this issue Sep 24, 2024
For Large Java file 'Replace All' takes long time and freezes the UI.
One of the reason is Projection Model tries to iterate over all the
Projection Annotations to expand/collapse status.
We can improve this situation by using Region specific iterator. This
returns annotations which are enclosed by given offset.
This improves the performance by 25% at least.

See eclipse-platform#2257
iloveeclipse pushed a commit that referenced this issue Oct 1, 2024
For Large Java file 'Replace All' takes long time and freezes the UI.
One of the reason is Projection Model tries to iterate over all the
Projection Annotations to expand/collapse status.
We can improve this situation by using Region specific iterator. This
returns annotations which are enclosed by given offset.
This improves the performance by 25% at least.

See #2257
@iloveeclipse iloveeclipse added this to the 4.34 M2 milestone Oct 1, 2024
@iloveeclipse
Copy link
Member

Thanks Raghu. Do you plan to provide more patches in this area, or should we close the ticket?

@raghucssit
Copy link
Contributor Author

Thanks Raghu. Do you plan to provide more patches in this area, or should we close the ticket?

I did not find any other safe fixes. For now we can close this. If I find any better solution later. I will open a separate issue.

merks added a commit to merks/eclipse.platform.ui that referenced this issue Oct 4, 2024
- ITextSelection.emptySelection has offset and length -1 so those values
need to be tolerated downstream, included in the recent-optimized
ProjectionAnnotationModel.expandAll method.

eclipse-platform#2257
merks added a commit that referenced this issue Oct 4, 2024
- ITextSelection.emptySelection has offset and length -1 so those values
need to be tolerated downstream, included in the recent-optimized
ProjectionAnnotationModel.expandAll method.

#2257
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working performance
Projects
None yet
Development

No branches or pull requests

4 participants