Performance improvement for find & replace all in Batch Mode. #2268
Conversation
|
@iloveeclipse Do you have time to look into this PR? |
mehmet-karaman
force-pushed
the
perf_improvement_batch_replace
branch
from
66f734e
to
e973066
Compare
|
Rebased on the Latest master.. |
|
I think a document.set() will replace all annotations on the document, too. Can you confirm that bookmarks survive that operation? |
|
Oh you're right.. I will look into this how this works.. Maybe I can find the damage regions and feed it one by one .. |
There was a problem hiding this comment.
I support this initiative, Replace all is very slow and blocks UI even in moderately sized editors.
I believe we can improve the architecture of this approach; we don't need to have another dedicated "batch replace" operation in the Find/Replace targets. Instead, we should just always batch replace when possible (i.e.: if the replace mode is turned on and we are asked to replace, always batch replace)
If we end up staying with this approach, there are a few very important changes to consider.
This PR is breaking every project which uses the FindReplaceTarget system at any point (emf, PREEvision, ...) - if we want to merge it at all, we need to make sure that we extract the required Interfaces into a FindReplaceTargetExtension as has been convention for e.g. scoped search.
| @@ -100,4 +100,23 @@ public interface IFindReplaceTarget { | |||
| * @param text the substitution text | |||
| */ | |||
| void replaceSelection(String text); | |||
|
|
|||
There was a problem hiding this comment.
This is dangerous to do as many other projects implement the IFindReplaceTarget (EMF implements this too, for example) @merks
The current approach taken is to implement a new IFindReplaceTargetExtension. We are at target extension 4, it's time to break the tradition of counting up, please give it a meaningful name (eg. IFindReplaceTargetBatchCountExtension)
There was a problem hiding this comment.
Thanks, This gives me an idea.. Maybe I can fix the IMarker blocker with creating this new extension
| * @param regexSearch RegEex search. | ||
| * @param incrementalSearch search in selected lines. | ||
| */ | ||
| default int batchReplace(String findString, String replaceString, boolean wholeWordSearch, boolean caseSensitiveSearch, boolean regexSearch, boolean incrementalSearch) { |
There was a problem hiding this comment.
I am not sure why a method declaration in an interface has a method body and why that even is legal...
There was a problem hiding this comment.
Thats because it shouldn't break any other implementers of this interface. By default I've disabled the batch replace in the interface to avoid breaking / compilation errors in all other implementers..
| /** | ||
| * @return true, if its able to handle batch replacements. | ||
| */ | ||
| default boolean canBatchReplace() { |
There was a problem hiding this comment.
not required after extracting a new FindReplaceTargetExtension. Instead, we assume that any extender of FindReplaceTargetX can perform all of it's operations
|
|
||
| import org.eclipse.osgi.util.NLS; | ||
|
|
||
| public class Messages extends NLS { |
There was a problem hiding this comment.
Messages is not a helpful name - why are we even extracting this into a separate class?
There was a problem hiding this comment.
I believe this pattern is how most of the nls support is provided by the wizard.
| @@ -0,0 +1,18 @@ | |||
| package org.eclipse.jface.text.internal; | |||
There was a problem hiding this comment.
Please add a copyright-header (in all other files too)
| return target.batchReplace(findString, replaceString, wholeWordSearch, caseSensitiveSearch, | ||
| regexSearch, incrementalSearch); | ||
| } finally { | ||
| selectableTarget.setReplaceAllMode(false); |
There was a problem hiding this comment.
please set the correct FindReplaceLogicStatus here
| if (target.canBatchReplace() && target instanceof IFindReplaceTargetExtension selectableTarget) { | ||
| selectableTarget.setReplaceAllMode(true); | ||
| try { | ||
| boolean wholeWordSearch = isAvailableAndActive(SearchOptions.WHOLE_WORD); |
There was a problem hiding this comment.
I think this should be extracted into a subroutine for clarity (if we even end up chosing an explicit batchReplace operation)
|
@Wittmaxi Thanks for the review. But with this PR i stumbled over the blocker, that all IMarkers are lost.. Tasks, Bookmarks and Breakpoints.. I would be able to move them programmatically after all replaces but from this bundle I don't have an access to IMarkers.. There is much more effort needed than just putting a method which replaces in a string.. Only improvement is what we can do from here just execute the Batch Replace if there are no Annotations in the current open file, otherwise execute the original search and replace.. |
Do we have case with editors without annotations (except plain text edito)? |
This is an improvement of the search & replace all in text editors/java editors/xtext editors etc. Instead of replacing all occurrences one by one (and notify document change), it replaces all occurrences at once and then notifies the document change (document.set(..) ).
As this feature is only for users which have huge files in their workspace, I've added the ability to turn this feature on/off. For better reusability I had to refactor the two private methods (asRegPattern, substituteLineBreak) in FindReplaceDocumentAdapter and move them into org.eclipse.jface.text.internal.RegExUtils class as public static methods and adjusted the callees to call the refactored methods.
The batch replace was tested with 20.000 occurrences (Java Editor, around 50% improvement and xtext editor about 40%) .. Opening the Java file in plain text editor improved more than 90%.