search

eclipse-platform / eclipse.platform.ui

Eclipse Platform
https://projects.eclipse.org/projects/eclipse.platform
Eclipse Public License 2.0
81 stars 188 forks source link

Improve "Replace all" performance - Find/Replace-->Replace All freezes UI on a large Java file. #2257

Closed raghucssit closed 1 month ago

raghucssit commented 2 months ago

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)

Wittmaxi commented 1 month ago

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 commented 1 month ago

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

raghucssit commented 1 month ago

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.