Bug 564448 - [Styled Text] Block Selection performance depends on selection direction
Summary: [Styled Text] Block Selection performance depends on selection direction
Status: NEW
Alias: None
Product: Platform
Classification: Eclipse Project
Component: SWT (show other bugs)
Version: 4.15   Edit
Hardware: PC Mac OS X
: P3 normal with 1 vote (vote)
Target Milestone: ---   Edit
Assignee: Platform-SWT-Inbox CLA
QA Contact:
URL:
Whiteboard:
Keywords: performance
Depends on:
Blocks:
 
Reported: 2020-06-18 17:25 EDT by Ari Kast CLA
Modified: 2022-04-14 01:02 EDT (History)
3 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Ari Kast CLA 2020-06-18 17:25:20 EDT
1. Using block selection mode, scroll down 200 lines, then begin your selection
2. scroll up 200 lines and shift-click
3. holding shift key, you can easily expand/contract your selection left or right, and performance is snappy and good.
4. now repeat the same experiment, but this time select from top to bottom instead of bottom to top.  Performance is now terrible.

What seems to be happening is in StyledTextRenderer, the "layouts" field only caches from topIndex downward (topIndex seems to be the top-most visible line on screen).  So selecting bottom to top, the top selected line is visible on screen and therefore layouts cache is used for all of the lines.  But top to bottom, lots of cache misses so lots of extra calls to the very slow TextLayout.getLineCount() method.

This discrepancy of course only occurs when block selection height is < CACHE_SIZE, ie 300 lines.  After that performance degrades regardless of direction, as mentioned in bugs 276265 and 311141.
Comment 1 Mickael Istria CLA 2020-06-22 02:58:35 EDT
Do you see the same issue when just moving up or scrolling up?
If not, there may be some possible simple fix to do, as expanding selection should be more expensive than just moving 1 line up.
Comment 2 Ari Kast CLA 2020-06-28 01:09:06 EDT
Yes same issue occurs regardless of how the selection is made, as long as selection is made in block mode the issue will always occur when expanding the selection downward, or left/right if current selection was made downward and currently exceeds a downward height of 50 lines or so (the more lines deep, the slower it gets).

The duration of delay is directly dependent on vertical depth of currently selected block, as measured from the selection origin.  

Some examples that WILL trigger the slowdown state (once in slowdown state, any further left/right selection expansion will be slow): 

- click on line 20, then scroll down to line 150 and shift-click.

- click on line 20, then hold shift and arrow down to line 150.

- click line 150, then scroll up to 20 and shift-click, which is fast, but then hold shift key and use arrow downward. This will be fast too, until the origin point (150) is crossed, and then at depth 50 or so more from there (so around line 200) you'll see selection slowing down again, the same as if you'd started at 150 and gone directly to 200.  The farther down you go, the slower it gets 


Some examples that will NOT trigger the slowdown:

- click line 150, then shift-click to line 20

- click line 150, then hold shift key and use arrow to expand up to line 20

- make a downward selection (this will be slow), then hold shift and use arrow to reduce selection upward (this is slow too).  But once you approach and cross your origin, selection gets fast again, and any sluggishness is gone, just as if you'd directly made an upward selection to begin with
Comment 3 Ari Kast CLA 2020-06-28 01:14:07 EDT
To be extra clear: the slowness only occurs when changing the boundaries of a block selection.  It does not occur during normal non-block selection, and does not occur during ordinary scrolling or arrowing up without holding the shift key.
Comment 4 Mickael Istria CLA 2020-06-28 12:48:57 EDT
(In reply to Ari Kast from comment #3)
> To be extra clear: the slowness only occurs when changing the boundaries of
> a block selection.  It does not occur during normal non-block selection, and
> does not occur during ordinary scrolling or arrowing up without holding the
> shift key.

OK, thanks.
I think in many cases, the block selection shouldn't be too much extra difficult to deal with compared to expanding a regular selection with shift+arrow, especially since it enforces a fixed width font, so we don't need too much extra computation when changing boundaries compared to eg. just scrolling.
Have you identified what makes the case of block selection require a synchrnonous access to the text layout?
Comment 5 Ari Kast CLA 2020-06-28 21:22:08 EDT
Hi Mickael,

Maybe I've misunderstood the question, but I did not know that access to text layout was synchronous?  The main thing I noticed is that the layout cache is mostly not used when selecting downward, while upward selection is able to make effective use of that cache.  

At a technical level it appears that the code only respects that cache for lines that are on current screen or below current screen, while lines that are above current screen are invalidated for some reason.  Therefore, selecting downward puts most of the lines above screen, whereas selecting upward puts most lines below screen where cache is honored.

But I don't really understand the logic behind why this is the case.  Any guidance on the matter appreciated!
Comment 6 Mickael Istria CLA 2020-06-29 03:09:28 EDT
(In reply to Ari Kast from comment #5)
> Maybe I've misunderstood the question, but I did not know that access to
> text layout was synchronous?

Maybe I'm overinterpreting, but usually, performance issues with the StyledText are caused by calls to methods getLineHeight() or similar that themselves call the StyledTextRenderer, itself either using the cache when available, or building a new TextLayout (which is usually the expensive operation causing a performance issue).

> The main thing I noticed is that the layout
> cache is mostly not used when selecting downward, while upward selection is
> able to make effective use of that cache.

That may be an issues, but I don't think it's what we should focus on.
What I think needs to be investigated is what Shift+⬆️ is more expensive/less performant with block selection than it is with regular selection; I believe there is one optimization we can make here to avoid calls some calls to TextLayout when expanding the block selection.
Comment 7 Ari Kast CLA 2020-06-30 00:36:10 EDT
Now that you mention it, I did notice something odd higher up in the call stack in TextViewer.getSelection() which looks like this:

	@Override
	public ISelection getSelection() {
		final ITextSelection res= computeSelection();
		cachedSelection= res;
		return res;
	}

So essentially the cachedSelection is never really used -- it is recomputed every time.

I did try rearranging some code in that class so that getSelection() returns cachedSelection without calling computeSelection(), and meanwhile the computeSelection() method when it was actually called would refresh the cachedSelection field. That approach yielded radically better performance and mostly worked, but it also introduced some weird layout issues so I abandoned that approach in favor of focusing at a lower level.

The performance improvement was huge though -- typing a character into a 180-line block selection went from taking 45 seconds down to 3 seconds.  I don't recall if it helped this specific block-selection bug though, or if it only helped with block char insertion/deletion performance.

But if you think that avenue is more promising I can go back to that approach and try to resolve the issues there.
Comment 8 Mickael Istria CLA 2020-06-30 03:06:30 EDT
I think your current investigation are interesting. The cache might be useless, but it's not IMO what causes the most pain, and I imagine the issue lays somewhere under computeSelection.
Have you tried using a profiler to find what are the time consuming operations and their related call hierarchy?
Comment 9 Ari Kast CLA 2020-06-30 11:51:07 EDT
Yes at a low level it's quite clear where the delay is.  Top of stack always looks like this:

    at org.eclipse.swt.internal.cocoa.OS.objc_msgSend_stret(Native Method)
    at org.eclipse.swt.internal.cocoa.NSLayoutManager.glyphRangeForTextContainer(NSLayoutManager.java:88)
    at org.eclipse.swt.graphics.TextLayout.computeRuns(TextLayout.java:334)
    at org.eclipse.swt.graphics.TextLayout.getLineCount(TextLayout.java:1018)
    at org.eclipse.swt.custom.StyledTextRenderer.getTextLayout(StyledTextRenderer.java:1261)

So the StyledTextRenderer.layouts field is all about avoiding those expensive TextLayout.getLineCount() calls.  With an upward select, TextLayout.getLineCount() is hardly called at all, while downward select calls it thousands of times.
Comment 10 Mickael Istria CLA 2020-06-30 11:52:50 EDT
(In reply to Ari Kast from comment #9)
> Yes at a low level it's quite clear where the delay is.

What's probably more interesting is what's in between: what makes that expanding a block selection 1 line up invokes a TextLayout, while it doesn't seem like a regular selection requires it.