Community
Participate
Working Groups
Steps to reproduce: 1. Create a file with many long lines (500+). 2. Enable block selection mode. 3. Put cursor at (1,1). 4. Press Shift + down arrow and hold it. => Observe how scrolling gets slower when selection gets bigger. I did some initial investigation and it turns out that slowdown is caused by the fTextWidget.getSelectionCount() calls invoked from here: Thread [main] (Suspended (breakpoint at line 3950 in TextViewer)) owns: RunnableLock (id=7773) SourceViewer(TextViewer).canDoOperation(int) line: 3950 SourceViewer.canDoOperation(int) line: 802 TextOperationAction.update() line: 154 TextEditor(AbstractTextEditor).updateAction(String) line: 5427 TextEditor(AbstractTextEditor).updateSelectionDependentActions() line: 5500 AbstractTextEditor$17.run() line: 3135 RunnableLock.run() line: 35 UISynchronizer(Synchronizer).runAsyncMessages(boolean) line: 135 Display.runAsyncMessages(boolean) line: 4145 Display.readAndDispatch() line: 3762 ...
I checked all calls to StyledText.getSelectionCount() in the jface.text project and it is always compared to 0. That means we don't really care about length of the selection, we just need to know if selection is empty or not. If we cannot get rid of the calls shown in comment 0, I think it is worth to consider adding StyledText.isSelectionEmpty(). In case of the scenario above, line selection is an empty string and overall block selection contains only line delimiters which means we can tell if selection is not empty right after the first line. The implementation of StyledText.isSelectionEmpty() could look like StyledText.getSelectionCount() with inlined optimized version of StyledText.getBlockSelectionText(String). Optimization would be to return true when buffer is not empty (I guess we could even get rid of the buffer itself). I guess having StyledText.isSelectionEmpty() could give performance improvement across the whole jface.text plugin.
(In reply to comment #0) > Steps to reproduce: > 1. Create a file with many long lines (500+). What is "long" for you? For me 100 chars is already (too) long ;-). Which editor? Do you see the same in the TextEditor SWT Example?
(In reply to comment #2) > (In reply to comment #0) > > Steps to reproduce: > > 1. Create a file with many long lines (500+). > > What is "long" for you? For me 100 chars is already (too) long ;-). > Which editor? > Do you see the same in the TextEditor SWT Example? 100 chars should be fine :-) I wrote "long" so that you can observe slowdown around 200th line (in my case). I tested also a file with several thousand of empty lines and it is also reproducible there, you just need to select more lines. You will see the slowdown either way, only selection needs to be "big" enough ;-) The bug happens in any editor that uses org.eclipse.jface.text.TextViewer.canDoOperation(int). Yes, I tested the same scenario in TextEditor SWT Example. Slowdown is caused by IAction's updates (see the stack in comment 0) so it is not reproducible in TextEditor SWT Example.
I forgot to add that non-block selection of the same size does not result in slowdown. That is what made me aware of how StyledText.getBlockSelectionText(String) is implemented.
Dani, if you agree with analysis in comment 1, and there is no way to get rid of those calls in TextViewer.canDoOperation(int), I can prepare a patch for StyledText. This would be an API change so we would need PMC approval to fix it in M7, right?
(In reply to comment #5) > Dani, if you agree with analysis in comment 1, and there is no way to get > rid of those calls in TextViewer.canDoOperation(int), I can prepare a patch > for StyledText. This would be an API change so we would need PMC approval to > fix it in M7, right? Sorry, I currently don't have time to look into this, unless it becomes a critical issue. I might have some time during M7, but no promise.
Created attachment 228530 [details] SWT patch v.0.1
Created attachment 228531 [details] Text patch v.0.1
The two patches implement solution proposed in comment 1 and fix slowdown caused by expensive calls to StyledText.getBlockSelectionText(String).
Moving to SWT for considering the API addition during Luna. Will work on the text part once the API is available.
Is it possible to release this API addition for 4.4 M2?
I think we should add this, to improve block selection performance in the editor. Setting target to 4.4 M3.
Created attachment 236109 [details] SWT patch v.0.2 + TestCases Attaching an optimized SWT patch for the new API isSelectionEmpty() in StyledText class, along with related TestCases. Note:- Unit Tested this API, but will require an end-to-end testing, as it works for me on Win7.
The implementation is fine. Let's rename the API to isSelected() instead and negate the return value. Reason: there are cases of isSelected() in the toolkit (in List and Table). isSelectionEmpty() would be the first.
Created attachment 236343 [details] SWT patch v.0.3 + TestCases Renamed the new API to isSelected() in StyledText & negated the return value.
Looks fine. Lakshmi, please review and commit the code. I marked this bug as noteworthy. Note that we still need to add the @since tag.
Niraj, I moved the tests into test_isSelected(). Please take a look, in all our tests the test function is named after the API. Committed the patch to master > http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?id=fcd2ef1db374a0d7da46dc6ce4535f35325185a4 Thanks Niraj & Silenio!
Opened Bug 420057 to track the Text side of the fix.
(In reply to Lakshmi Shanmugam from comment #18) > Opened Bug 420057 to track the Text side of the fix. I tried to update the patch for the Text part, but I get the following error: Invalid @since 4.4 tag on isSelected(); expecting @since 3.103 Are you sure that 4.4 is correct here? I have seen many places with @since 3.102 but none with anything like @since 4.x in the SWT plugin.
(In reply to Szymon Ptaszkiewicz from comment #19) > (In reply to Lakshmi Shanmugam from comment #18) > > Opened Bug 420057 to track the Text side of the fix. > > I tried to update the patch for the Text part, but I get the following error: > > Invalid @since 4.4 tag on isSelected(); expecting @since 3.103 > > Are you sure that 4.4 is correct here? I have seen many places with @since > 3.102 but none with anything like @since 4.x in the SWT plugin. The @since tag is wrong - it must be 3.103.
(In reply to Dani Megert from comment #20) > The @since tag is wrong - it must be 3.103. Fixed
(In reply to Silenio Quarti from comment #14) > The implementation is fine. Let's rename the API to isSelected() instead and > negate the return value. Reason: there are cases of isSelected() in the > toolkit (in List and Table). isSelectionEmpty() would be the first. When adopting this (see bug 420057), I was a bit puzzled by the name: what is selected? The widget? Any character? All characters? Actually there are no other isSelected() declarations in SWT. The ones mentioned all take an argument and hence the method name makes sense there. I'd prefer to rename the method to #hasSelectionText(). This fits well together with #getSelectionText().
(In reply to Dani Megert from comment #22) > (In reply to Silenio Quarti from comment #14) > > The implementation is fine. Let's rename the API to isSelected() instead and > > negate the return value. Reason: there are cases of isSelected() in the > > toolkit (in List and Table). isSelectionEmpty() would be the first. > > When adopting this (see bug 420057), I was a bit puzzled by the name: what > is selected? The widget? Any character? All characters? Actually there are > no other isSelected() declarations in SWT. The ones mentioned all take an > argument and hence the method name makes sense there. > > I'd prefer to rename the method to #hasSelectionText(). This fits well > together with #getSelectionText(). SWT does not have any API that starts with "has*". I would prefer not to add one now. For me isSelected() is clear. There are no parameters, so it means it applies to the widget (receiver). If there was a parameter, it would be an offset and the API would tell whether the offset is selected. It could be a range (start, end offsets) as well. If the user cares whether the whole text is selected, he can call getSelectionCount(). Anyways, would getTextSelected() or isTextSelected() be better for you?
(In reply to Silenio Quarti from comment #23) > (In reply to Dani Megert from comment #22) > > (In reply to Silenio Quarti from comment #14) > > > The implementation is fine. Let's rename the API to isSelected() instead and > > > negate the return value. Reason: there are cases of isSelected() in the > > > toolkit (in List and Table). isSelectionEmpty() would be the first. > > > > When adopting this (see bug 420057), I was a bit puzzled by the name: what > > is selected? The widget? Any character? All characters? Actually there are > > no other isSelected() declarations in SWT. The ones mentioned all take an > > argument and hence the method name makes sense there. > > > > I'd prefer to rename the method to #hasSelectionText(). This fits well > > together with #getSelectionText(). > > SWT does not have any API that starts with "has*". I would prefer not to add > one now. For me isSelected() is clear. There are no parameters, so it means > it applies to the widget (receiver). Exactly and this is misleading because it is not the widget that is selected (e.g. in a page), but it is the text. > Anyways, would getTextSelected() or isTextSelected() be better for you? #isTextSelected() works for me, thanks.
Changed API to isTextSelected and updated the tests. http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?id=5a748dd716a2f6cf95105a6f5f78eee6d06dbfc6
