Community
Participate
Working Groups
Created attachment 11611 [details] First compared file
Created attachment 11612 [details] Second compared file
Hangs scenario: - Select "first compared file" and "second compared file" - Do "Compare with/Each other" - Click "Select Next Change" several times... - CPU - 100% - Eclipse hangs...
Are you sure Compare hangs? The second file contains one line with > 100k characters. Trying to step into this line took 5 minutes on my Mac, but otherwise worked fine. I could not reproduce the hang, however, I see the issue with very long lines here.
I'm in the office now where I have a Win2k box: There it really looks like the Compare is hung. But it is not :-) It just takes much longer because the default heap size on Window (64M?) is smaller than on Mac (150MB). Debugging revealed the following: Compare calls TextViewer.revealRange(67, 103293) to reveal the third line in the document (which has the length 103293) This calls TextViewer.getExtent(int start, int end) which itself uses fTextWidget.getLocationAtOffset(i) for every character of the 100000 char line. I stepped further into the loop and it seems that fTextWidget.getLocationAtOffset is quite expensive and takes a long time. I'm not sure that I can do something from Compare here. Moving to platform.text for comment.
Andre, thanks for the tracing of the 'hang' causes - it actually doesn't hang - but takes about 3 minutes on P4 3Ghz. :) It'd be great to eliminate mentioned expensive part of the code. PS. So, if I select in the text editor the line about 100K long - it'll 'hang'? :)
no action for 3.0
Duplicate of Bug 50335?
Do not think so. Here, the problem is in platform.txt - TextViewer.revealRange(). There, the problem is comparison itself.
adapting summary
Adding my name to the cc list as we are now tracking performance issues more closely. Please remove the performance keyword if this is not a performance bug.
Re comment #5: "This calls TextViewer.getExtent(int start, int end) which itself uses fTextWidget.getLocationAtOffset(i) for every character of the 100000 char line." That seems singularly inefficient. Why is that required?
AFAIK this has to be done in order to get the correct extent for BIDI. I guess the best way to improve this would be to have better support in StyledText. Moving to SWT for comment.
It's not at all clear to me why that should be true. At the very least, shouldn't we only need to compute the values on the segments where there are direction changes?
Yep, but the methods to get the bidi segments aren't public.
Daniel, does TextViewer.getExtent(int start, int end) return a Rectangle[], a Region, or a Rectangle (including all Rectangles)? If it returns a Rectangle we could add API to StyledText to expose TextLayout#getBounds(int start, int end). The thing is, at this point of the release cycle adding new API is kind of hard. ---*--- StyledText caches the TextLayout for each visible line, so the first call to fTextWidget.getLocationAtOffset(i) should be slow and the rest much faster. Things to check: Is the cache being used, is the line visible, is TextViewer doing something between iteration that would cause the cache to be flushed (this would cause very poor performance). Even with if cache on, for each iteration StyledText is getting the TextLayout (from the cache hopefully) again, and is calling all LineStyledListener again to make sure the line hasn't changed (this can be pretty slow).
Daniel, try adding this method to StyledText and using it from TextViewer. Let me know how much it helps: I hacked this is 2 min, I hope it works, it only work if start and end are within the same line. public Rectangle getBounds (int start, int end) { checkWidget(); int charCount = getCharCount(); if (start < 0 || start > charCount) { SWT.error(SWT.ERROR_INVALID_RANGE); } if (end < 0 || end > charCount) { SWT.error(SWT.ERROR_INVALID_RANGE); } int lineStart = content.getLineAtOffset(start); int lineEnd = content.getLineAtOffset(end); if (lineStart != lineEnd) SWT.error(SWT.ERROR_NOT_IMPLEMENTED); int lineOffset = content.getOffsetAtLine(lineStart); String line = content.getLine(lineStart); TextLayout layout = renderer.getTextLayout(line, lineOffset); Rectangle result = layout.getBounds(start - lineOffset, end - lineOffset); renderer.disposeTextLayout(layout); return result; }
>Daniel, does TextViewer.getExtent(int start, int end) return a Rectangle[], a >Region, or a Rectangle (including all Rectangles)? returns IRegion: /** * Returns the region covered by the given start and end offset. * The result is relative to the upper left corner of the widget * client area. * * @param start offset relative to the start of this viewer's view port * 0 <= offset <= getCharCount() * @param end offset relative to the start of this viewer's view port * 0 <= offset <= getCharCount() * @return the region covered by start and end offset */ final protected IRegion getExtent(int start, int end) { >The thing is, at this point of the release cycle adding new API is kind of >hard. Adding a new API (not changing existing one) to fix a performance problem will not be that hard, we simply need to convince one of the PMC members ;-) >Daniel, try adding this method to StyledText and using it from TextViewer. >Let me know how much it helps: I'll start now. Have to replace binary SWT with source which takes quite some time. >I hacked this is 2 min, I hope it works, it only work if start and end are >within the same line. That's how we currently use the method but unfortunately this isn't speced in the Javadoc and hence we have to support covering of multiple lines i.e. we will call your method for each line to get the extent.
Performance wise it is a big improvement (factors for long lines) but it looks like the returned bounds are wrong (or at least don't match mine), e.g. StyledText.getBounds(0,0) returned Rectangle {0, 0, 6, 13} for a document that contains just one single line with 100 'a's. "my" result is (2, 0). The x value is always off by 2.
Here are some numbers (code currently assumes start and end on same line) based on a document with a single line filled with n 'a's. The test calls TextViewer.reveal(0, i) with i= 0..n. n= 1000 old code: 19-20s new code: 0.38s n= 10000 old code: > 1 hour (killed it) new code: 4-5s
Similar code can be found in InformationPresenter, TextViewerHoverManager and TextConsoleViewer. Those places could profit as well.
That looks like a massive performance win. We should make sure that this one gets fixed.
Created attachment 20379 [details] screeshot Okay, I'm supposing IRegion is some kind of SWT Region, and I'm also supposing you add the bounds of each character from start to end to this region. If I'm right then we are changing the behaviour of TextViewer.getExtent(). The reason is because TextLayout#getBounds() returns of bounding box. See the screenshot, old region is what I believe is the old behaviour, new region is the result if we change the code.
I should add, the screeshot I'm getting the bounds from 3 to 7 of the string "Latin\u05E9\u05E0\u05D1\u05D2\u05E7". About the code on comment 17, sorry, I forgot to translate from TextLayout coordinates to StyledText. After the line: Rectangle result = layout.getBounds(start - lineOffset, end - lineOffset); You should add: result.x += leftMargin - horizontalScrollOffset; result.y = lineStart * lineHeight - verticalScrollOffset; result.height = lineHeight;
Here's what our code does. Since we call getLocationAtOffset for each offset and compute min and max, it should give the same result, shouldn't it? int left= fTextWidget.getLocationAtOffset(start).x; int right= left; for (int i= start +1; i <= end; i++) { int x= fTextWidget.getLocationAtOffset(i).x; if (left > x) left= x; if (right < x) right= x; } return new Region(left, right - left);
Okay good, I supposed wrong. The only thing we should take care is that end is excluded in TextViewer#getExtent and in TextLayout#getBounds it is included.
Sounds good. I already rewrote getExtent(...) so that it can also handle start/end covering more than one line.
Felipe couldn't your API allow to cover more than one line? There are two reasons that speak for that: 1) I found at least four places in the SDK and they will have to write the same helper code to handle this 2) just guessing: due to more data SWT might be able to implement this with better performance than clients can
> Felipe couldn't your API allow to cover more than one line? Yes, I will implement like this anyway. Just keep in mind that if start and end are not in the same line the API will returns a big rectangle including the bounds of all the lines involved.
The big rectangle is what we compute, so it's perfect.
Daniel, when the range spans over several lines, what do you expect in rectangle.width ? 1) the width of the clientArea.width - margins 2) the max width of all lines included in the range
Since we currently call fTextWidget.getLocationAtOffset(...) for every single character and compute min and max I'd say this doesn't include the margins.
I added the new API, it is called StyledText#getTextBounds (int start, int end), the rules for start and end are the same from StyledText#getText(int start, int end). I know I have some bugs in TextLayout on Windows (the rectangle is too big sometimes - when reordering occurs), I'll be fixing it early next week. Fixed in HEAD > 20050429
I've changed all locations in Platform/JDT Text to use the new API (>= I200505030010) and filed a bug against Platform Debug where the new API should be used.
