Community
Participate
Working Groups
The Word Wrap feature from bug 35779 makes some editor actions very slow. We need to investigate the cause for the slowness and fix it at the right layer (Text or SWT). Important point to know: When doing measurements with Word Wrap disabled, always use a new editor instance that *never* had Word Wrap enabled! setWordWrap(true) is one way to set StyledText#fixedLineHeight to false. After that, StyledText performance never returns completely to the untainted state. Two scenarios that can show a big slowdown: 1. Use Ctrl++ / Ctrl+- to zoom text editor font 2. Resize the editor area Factors that make the slowdown unbearable: [A] In both cases, performance looks OK if the editor shows the beginning of the file. But when you scroll to the end of a larger file, there are huge delays. Example: Copy source of the JavaEditor class (4228 lines) to a new Untitled Text File. At the end of the file, Zoom commands take about 1 second. And this is just with one editor. If you have multiple editors in that state, the delays add up. [B] Another factor is the Overview ruler in a file with many annotations. Example: - Open java.lang.String - select the identifier "String" and use Search > Occurrences in File > Text (Ctrl+Shift+U) => 106 Search matches - select the identifier "value" => 121 Mark Occurrences matches => Performance is already bad at the beginning of the file
Another point to consider when making measurements is StyledTextRenderer#calculateIdle(). This method pre-computes the contents of lineWidth and lineHeight in a sequence of asyncExecs (each a bit longer than 50ms). This can influence results in cases with multiple successive changes (resize; repeated Zoom In/Out commands).
Note that we will have to disable word wrap in Neon unless these problems get fixed.
(In reply to Markus Keller from comment #2) > Note that we will have to disable word wrap in Neon unless these problems > get fixed. I cannot guarantee that I will get free time for this (this performance issue is not a blocker for our company, so I have to work at home), but isn't this a bit too strict requirement? Disabling a feature because some corner cases aren't as fast as they could be? The line wrap is not enabled by default, so whoever will hit the corner cases with the performance will be "just" disappointed by this, but still will be able to use it in other 90% of cases. Removing the entire *possibility* to have word wrap is much worse IMHO.
Returning the bug to the team pool: I cannot guarantee that I will be able to address this in 4.6 time frame.
When Word Wrap was added, I enabled it for new editors. On the next day, Eclipse became too slow for normal work, and it took me a while to make the connection to the word wrap setting. We will not advertise a feature in Neon if we already know that it will generate more "Eclipse is slow" complaints that will be hard to track down. You know that new features always need some polishing investments in later milestones. We probably won't find anyone else who would have time for that, so if you're not in a position to fulfill your committer duties, we will have to pull it.
Created attachment 259466 [details] yourkit screen shot of resizing the editor
(In reply to Markus Keller from comment #5) > When Word Wrap was added, I enabled it for new editors. On the next day, > Eclipse became too slow for normal work, and it took me a while to make the > connection to the word wrap setting. So as a compromise we can only remove that setting (making the decision to toggle word wrap tightly connected to the "slow motion" result, so the user knows what caused it). At least after reopening the editor the "slow motion" effect of word wrap would be gone. > so if you're not in a position to fulfill your committer duties, we will > have to pull it. I hope we can agree on compromise above if the performance bug remains unresolved. Word wrap can be really a time saver if one have long generated lines. Anyway, after playing a bit with the resizing of editors (see screenshot from yourkit) I see this bug more in the SWT land / StyledText. As usually I have no idea about the underlined design, but it wonders me that setWordWrap() toggles setVariableLineHeight() which in turn makes many methods like getLinePixel(int line) to believe that each line has different height and so loop over all text lines calling StyledTextRenderer.getLineHeight(int) and preforming lot of unneeded calculations. In my ideal world word wrap in StyledText does NOT toggle setVariableLineHeight(), it only makes the "model split" between "widget lines" (drawn one character height text row) and "logical lines" (text characters between new line separators). The "widget line" height is constant, it does not depends on the word wrap, it depends solely on the font metrics. The "logical line" height is of course affected by word wrap, but it's calculation could be trivial if StyledText would not have setVariableLineHeight(). Also, in my ideal world getLinePixel(int) would first calculate the "widget line" for the given "logical line" and then return the calculated "widget line" top pixel (which is basically just lineIndex * lineHeight). This would be therefore just a map lookup and not looping multiple times over all lines calculating native bounds of each. Currently for me it looks like StyledText word wrap is implemented without any model split, and so each line height related method just does brute force loops summing up the line heights up to the given one. This probably worked well for small text boxes but this is of course does not scale with editors counting over 1000 lines. So if any from the SWT commiters could shed some light on StyledText design ideas, especially in the area of logical/widget line calculations, it would be great.
I'm afraid none of the current SWT committers were around when word wrap was added to StyledText, and I'm not aware of design documents other than what can be found in source or Bugzilla (i.e. there's probably nothing). Since the text editor framework is the first client that tries to use word-wrap on bigger documents, it's part of our job to solve encountered issues there as well. The SWT team has already more than enough on their plate for Neon and probably won't get to this. Your analysis matches my gut feeling about the problem area and possible solutions. Though I'm not sure we can avoid setVariableLineHeight(). I've recently fixed bug 485722, which showed me that TextLayout's use of native text rendering engines sometimes results in fractional line heights. This could quickly lead to gaps or overlaps when logical lines wrap to several visual lines. But some caching of logical line heights could help and shouldn't be too hard to get right.
I've spend another hour grocking on styled text. I think we can partly fix performance if we introduce new API which does not consider word wrapping as changing *fixed* line height. Current code in StyledText turns fixed line height off if word prap is turned on, but the irony is that for getLineHeight(i) this is simply not true and the code could in theory still use fixed height calculations. Because this method works with widget lines, it still returns wrong values for a "full wrapped line" and it does not make sense to run extra calculations here. A new API is needed for those clients who wants "fullLineHeight" for a full line mapped to given widget line. Those clients are exact the places which must be fixed for bug 483937. All others do not need this API and could use the "fixed" getLineHeight() with no overhead on WW enabled.
Performance issue on resizing editor depends (on the client side) solely on rulers paint code: if I completely disable painting on rulers, resizing the editor works as fast as without word wrap. Places to disable: AnnotationPainter.handleDrawRequest() AnnotationRulerColumn.doPaint1() LineNumberRulerColumn.doubleBufferPaint() OverviewRuler.doPaint() The problem is that they sooner or later trigger some ineffective StyledText API (e.g. StyledTextRenderer.calculate(int, int) and this one seem? to be pretty ineffective in wrap mode (why, I don't see it???), at least yourkit believes in it). Possible workaround: draw those annotations delayed (asynchronously), so only the last resize event will do the painting. I guess users do not need annotation updates while resizing editor? Still, the bad text zoom performance is not affected by the rulers code at all.
Since I'm unable to spend more time in the M6-M7 time frame for fixing this issue (and I fear this will require new API in StyledText area), I propose to disallow the WW preference to be persisted => bug 488162 and patch https://git.eclipse.org/r/66994. I think this is a fair compromise which let us ship the WW functionality in 4.6, since each time user might observe the performance slowdown it could get the connection between this and toggling the WW button before. This way users which *want* or even *need* WW functionality turned on for editing a specific file will still be able to use it (delays are acceptable in this case), and others will not be bothered by persisted preference and unexpected UI freezes while resizing some big files. P.S. I do not give up on this bug and I hope I will find some time later, but I'm just realistic - I simply have no free time to work on it in 4.6 time frame.
(In reply to Andrey Loskutov from comment #11) > Since I'm unable to spend more time in the M6-M7 time frame for fixing this > issue (and I fear this will require new API in StyledText area), I propose > to disallow the WW preference to be persisted => bug 488162 and patch > https://git.eclipse.org/r/66994. > > I think this is a fair compromise which let us ship the WW functionality in > 4.6, since each time user might observe the performance slowdown it could > get the connection between this and toggling the WW button before. > > This way users which *want* or even *need* WW functionality turned on for > editing a specific file will still be able to use it (delays are acceptable > in this case), and others will not be bothered by persisted preference and > unexpected UI freezes while resizing some big files. > > P.S. > I do not give up on this bug and I hope I will find some time later, but I'm > just realistic - I simply have no free time to work on it in 4.6 time frame. Sounds fair.
Resetting the target milestone as per the above discussion...
(In reply to Andrey Loskutov from comment #10) > The problem is that they sooner or later trigger some ineffective StyledText > API (e.g. StyledTextRenderer.calculate(int, int) and this one seem? to be > pretty ineffective in wrap mode (why, I don't see it???), at least yourkit > believes in it). The reason why this API is ineffective in word wrap mode is because in order to compute the rendered line height there seems to be no other reliable solution that asking the OS to do some kind text rendering offscreen which is able to compute whether a line has to be wrapped or not. This is what TextLayout seems to do in StyledTextRenderer and this seems to be an expensive call (as it most likely do some font rendering of the whole document). Recent optimizations have tried to reduce amount of calls to calculate(...) and have simplified how getLineHeight(i) is computed in some cases, but didn't yet manage to improve the case of wordwrap. Some possible paths to improve performance in StyledText(Renderer) consist in 1. minimizing the number of calls to calculate(...) or 2. find cases for which we can implement a calculate(...) without calling TextLayout. The simpler cases may be font with fixed width for instance, for which we may only have to do multiplication to compute size instead of making an offscreen rendering. Things become much harder when an editor uses multiple different fonts.
I believe the resize of the editor can be improved in the average case (not in the worst case) with a corollary of bug 530019: it seems very likely that in case of a resize with variable height, the widget decides to reset the whole line cache everytine, resulting in a lot of re-computation. Instead of resetting all lines, it could simply reset lines which 1. are already wrapped or 2. are longer than the area width when wordwrap is set.
Possible duplicate of bug 168557, where I shortly pushed a fix to gerrit.
I agree this is a dup of bug 168557. *** This bug has been marked as a duplicate of bug 168557 ***
