Community
Participate
Working Groups
This bug report is a fork of bug https://bugs.eclipse.org/bugs/show_bug.cgi? id=37683 A test have been done to illustrate the completely unacceptable performance of the Java editor using file plugins/org.eclipse.swt/Eclipse SWT Custom Widgets/common/org/eclipse/swt/custom/StyledText.java A change of "a" to "a1" took about half an hour on a Linux SuSE 8.2 PC with AMD 2000+ processor with 512 Mb memory. Eclipse was choking and used over 90% CPU. The same test on a Windows 2000 PC with AMD 1G processor and 256 Mb memory took less than 20 seconds. <quote from steve_northover@ca.ibm.com> The issue here is that Eclipse is relying on the Windows version of the method setRedraw() to throw away all of the individual drawing requests. This is interesting issue and needs to be followed up with them. It may be that they will not be able to rely on this behavior or that SWT can provide a partial implementation of setRedraw() to fix this particular problem. </quote from steve_northover@ca.ibm.com>
We have done lots of performance improvements after M6. So, it would be good if you attach the build id in which you see this behavior. Tom, please verify that this scenario.
I repeated the test with Eclipse 3.0 M7 Platform 1: SuSE Linux 8.2 AMD 2000+ Eclipse running with 256 Mb RAM Platform 2: Windows 2000 AMD 1000 Eclipse running with 200 Mb RAM Performance on Windows acceptable: about 20 seconds. Performance on Linux has improved since M6. However it is still like watching grass grow. After a couple of minutes less than 10% was done: still around 30 minutes for this change. To give an idea of how slow the Eclipse editor is I repeated the same test with Kwrite. Kwrite did the change in the blink of an eye, I estimate about 0.2 second. Netbeans 3.5.1 did test in 64 seconds: about 30 times faster than Eclipse SWT/GTK
>A change of "a" to "a1" took about half an hour Do you mean find/replace all "a" with "a1"?
>Do you mean find/replace all "a" with "a1"? Exactly, find/replace all "a" with "a1".
Some other useful information from https://bugs.eclipse.org/bugs/show_bug.cgi? id=37683#c147 I did a test with gedit2-2.2.0.1-29 written in C using GTK. I clocked 44 seconds for "a" to "a1" replaceall in StyledText. This is horrendously slow compared with 0.2 second in KWrite. It is even slow compared with 64 seconds of Netbeans 3.5.1 considering that I did not switch off backgound compilation.
I suspect that we are forcing the screen to be updated (either Eclipse is doing this or SWT is doing this as part of scroll()). Under Windows, when setRedraw() is off, nothing happens. On platforms that don't have this capability, you see every draw.
The described problem is real and the replace-speed is not acceptable. Moving back to inbox for general discussion. I believe the only thing we can do from the editor team would be to not rely on Control.setRedraw(boolean) (see bug 26153 for Dougs patch to Control) and implement it ourselves, possibly by temporarily disconnecting the model from the viewer. Not sure whether this is feasible.
I have traced this down to StyledText calling scroll() and scroll() causing an update. We are looking at fixing it. I hacked something in that didn't draw but performance did not improve. It might be because string measuring is slow on GTK (fast on Windows) and StyledText is still doing all the work to determinte where to draw even though nothing is actually drawn. Fixing setRedraw() will not fix this problem because string measuring still has to work even though drawing is turned off. RADICAL IDEA: How about changing text "replace all" so that it relaces all the text in a string then resets the string into the widget? This is what IBM/Smalltalk and VisualAge for Java did. Seem to make more sense to mea and would make everything faster on every platform.
BTW: Dougs patch is incomplete and won't fix this problem.
Complex components like StyledText need a way to start a "long operation" where the model can be changed in an arbitrary way. GTK had "freeze" and "thaw" for this in 1.2 which wasn't optimal but worked. As for Dougs patch, I think this goes into the right general direction. Being able to remember rendering operations would solve some problems but it would also cause a whole lot of new problems (there must be a redraw-queue and if only a small part of the text component is changed, the whole thing will flicker).
I tried the GDK window freeze and thaw operations and they do not help. Doug's patch will not help in this exact case. Either fixing the replace to show only the final result (Platform/Text) or hacking a partial implementation of setRedraw() and speeding up string operations (Platform/SWT) will fix this particular problem.
Did "replaceall a to a1" test using anjuta-1.0.0-119 on StyledText. It took about 48 seconds. This is slower than Eclipse on Windows with a processor half the speed of the test Linux box. Furthermore, Eclipse is running other tasks in the background. Worst of all, after finishing the replace task Anjuta is hanging with 40% User CPU. The Anjuta route does not look very promissing to me. We are replacing one problem with more problems.
Is it possible to detach the model of the text view during a "replace all"? Or is the MVC not implemented in the StyledText component?
While looking at the code of StyledText, I noticed that the LineCache (which seems to store the widths of all the lines) is cleared (partially) during a redraw(...) which seems kind of strange. I mean: Why should a refresh of the screen invalidate the length of the lines in the cache? Since calculating of line widths is an expensive operation in Pango(GTK), I'd suggest to match the clearing of the cache with the changes of the document model and only recalculate line sizes when the line in the model changes. And if there is some other code which expects redraw(...) to invalidate the cache, I'd suggest to split the code in order to speed up screen refreshes.
Aaron, read comment #8 (RADICAL IDEA).
Daniel, can someone from Text look into this? Replacing occurences is fundamental behavior for an IDE. I have already done some analysis that shows that a hack implementation of setRedraw() on GTK doesn't help that much. I am willing to do a more formal analysis to be sure but I think the fundamental problem is that you should not be showing partial results, as the strings are replaced. Thanks.
Re #8: I've read that comment but I thought that the document class (StyledTextContent?) is somehow involved and I wanted something which works even when the content is not a text. Therefore the idea: Before ReplaceAll, either unregister the viewer from the content or tell the content not to send update events. I think the former will be more simple to implement but the latter might be more thorough (think of an indexer in the background: That thread should also wait until all S&Rs are done).
Re #17: Not to mention using regexps during S&R and other nifty stuff which we don't want to implement two times for Replace/Find and ReplaceAll :-)
the replacement test using scite editor (see http://scintilla.org) configured to use antialiasing takes about 10 seconds on an ibm thinkpad t21 (p3 800mhz, 384mb ram, fedora core 1) the scite editor uses the same editor widget as anjuta -> scintilla.org. maybe someone can look at that as scintilla does things different. it uses pango_layout_get_line(layout,0)/gdk_draw_layout_line(drawable, gc, x, ybase, pll);
Stefan, I repeated the ReplaceAll test with scite-1.50-27 editor. It took 6 seconds on SuSe Linux 8.2 with AMD 2000+ processor. Font settings in User Options did not make any difference. This is a lot faster than Anjuta and Eclipse on Windows. There must be something on Anjuta that makes Scite work slower.
The text team is tracking this. Steve, in comment 8 you wrote: > Fixing setRedraw() will not fix this problem because string measuring > still has to work even though drawing is turned off. Why is this so? To compute the bounds of the document for the scrollbars? Currently, on platforms other than GTK, TextViewer::setRedraw(false) *does* disconnect the model (a Document implementing IDocumentAdapterExtension) from the widget via TextViewer::disableRedrawing. On GTK, this call has been disabled to avoid flashing the top of the editor because StyledText::setRedraw does not work. Replace-All on a document in sequential rewrite mode (which is what replace-all uses) is pretty much the same as replace-all on a string. I will do some measurements when re-enabling TextViewer::setRedraw, any of you who have posted numbers are encouraged to do the same.
Just as a first teaser: when re-enabling setRedraw, replace-all of a to a1 in StyledText (no regex, no-incremental, 11000 sth. matches) took less than 5 seconds. This is on a fairly fast machine (P4 2.4G) on rh 9.
>> Fixing setRedraw() will not fix this problem because string measuring >> still has to work even though drawing is turned off. > > Why is this so? To compute the bounds of the document for the scrollbars? If anyone (StyledText included) calls GC.stringExtent() or GC.textExtent(), we still need to measure the string and return a result, regardless of setRedraw (). All my quick hack did was stop drawing (which is all that Windows does). > the widget via TextViewer::disableRedrawing. On GTK, this call > has been disabled to avoid flashing the top of the editor because > StyledText::setRedraw does not work. Why on earth do you need to go to the top of the editor all the time and then (I assume) back again? Is it something that StyledText is doing to you for free? Not doing this will make things faster on all platforms as well and will also fix the bug that occasionally (on Windows where setRedraw() is supported), I type something (often the hot key that comments a block of code) and the text editor scrolls to a different place for no reason.
> Why on earth do you need to go to the top of the editor all the time and then > (I assume) back again? Is it something that StyledText is doing to you for > free? When re-enabling drawing, the model does not replay the changes that have occurred in the meantime, but simply informs the widget that the entire content has changed. StyledText will flash the top of the file when doing that. This gets masked by setRedraw on Windows, but not on GTK.
Re comment 24: > StyledText will flash the top of the file when doing that. Just to make sure that I understand you correctly: When I so setRedraw(true), StyledText will scroll to the beginning of the file, draw all visible lines and then scroll back down to the current view position and draw all (now) visible lines again? Why is that? Also, how about my comment 14? I'm pretty sure that optimizing the LineCache would boost the performance of StyledText by an order of magnitude.
I think we (SWT) is "flashing to the top" for free in a StyledText. I am investigating further. WRT comment #14. We are considering more caching in GC. StyledText itself is being rewritten to use the new "TextLayout" classes as part of the BIDI work so hacking it now might not be worth it.
I've created a simple benchmark for StyledText which you might want to use as a base to create a benchmark for this bug here. See https://bugs.eclipse.org/bugs/show_bug.cgi?id=51693#c32
The reason for the flash is quite simple, I believe: when the content of StyledText is reset, it will display the new content, and will do so at the top position. Only after that, the viewer resets the scroll position using StyledText::setTopPixel since it knows that the new content is pretty close to the old one. StyledText cannot know this. What it could do is trying to restore the previous scroll position when its content is reset.
Yes but I thought setText() and setTopIndex() would simple damage the entire area, causing a single paint for both operations. I am investigating why this is not the case.
StyledText could be change to try to restore the contents to the closest position but this would break people that relied on the old behavior. I believe that you should release the code that "disconnects the model" for the "replaceAll" case and we will concentrate on getting rid of the "scrolling to top" behavior. If we can't get rid of that (we will), the worst case will be it flashes instead of taking 40,000 years to do the replace, making us look like fools. Here is a simple example that shows the "scrolling to the top" (ASIDE: It scrolls on GTK but is very fast so you hardly see it on my machine. Is it the syntax coloring that makes the equivalent Java Editor so slow?). I have some fixes to SWT that make the "scrolling to top" in this example go away. I will release them shortly after I have verified that they don't break anything. After that, you can verify that "scrolling to top" is gone for you and we can close this darn bug report. Agree? import org.eclipse.swt.*; import org.eclipse.swt.widgets.*; import org.eclipse.swt.layout.*; import org.eclipse.swt.custom.*; public class PR_52899a { public static void main(String[] args) { Display display = new Display (); Shell shell = new Shell(); shell.setLayout (new FillLayout ()); final StyledText text = new StyledText (shell, SWT.V_SCROLL | SWT.H_SCROLL); shell.setSize (250, 250); for (int i=0; i<512; i++) { text.append (i + "This is a line of text in a StyledText.\n"); } text.setTopIndex (128); Button button = new Button (shell, SWT.PUSH); button.addListener (SWT.Selection, new Listener () { public void handleEvent (Event event) { text.setRedraw (false); String string = text.getText (); int index = text.getTopIndex (); text.setText (string); text.setTopIndex (index); text.setRedraw (true); } }); shell.open (); while (!shell.isDisposed ()) { if (!display.readAndDispatch ()) display.sleep (); } display.dispose (); } }
> I believe that you should release the code that "disconnects the model" for > the "replaceAll" case I am quite reluctant to release this before we have something working, because I believe the "flash" is much more of a nuisance than the slightly artificial case of replacing a with a1 in StyledText.java. I can't just reenable this for "replaceAll". Note that we call setRedraw a lot, namely when indenting code (Smart Tab), toggling comments, formatting,... basically anywhere we do more than just inserting a little text. This will interfere a lot more with my daily editing than the replaceAll story. I will be happy to take out the GTK-bypass and release it once we have solved the flash problem. All it takes is commenting out two lines. > Is it the > syntax coloring that makes the equivalent Java Editor so slow? I guess. Syntax coloring does use some cycles and probably does make the flash more prominent. > After that, you can verify that "scrolling to top" is gone for you and we > can close this darn bug report. All right!
The SWT fix has been released on all platforms. It involves changing Canvas.scroll() to avoid doing and update() when asked to copy bits that are not visible. StyledText uses scroll() in the implementation of setTopIndex(). NOTE: This fixes the problem in the test case PR_52899a but may not fix Eclipse. Control.setRedraw() is still unimplemented (see bug #53791). >> I believe that you should release the code that "disconnects the model" >> for the "replaceAll" case > > I am quite reluctant to release this before we have something working, > because I believe the "flash" is much more of a nuisance than the slightly > artificial case of replacing a with a1 in StyledText.java. I can't just > reenable this for "replaceAll". > > Note that we call setRedraw a lot, namely when indenting code (Smart Tab), > toggling comments, formatting,... basically anywhere we do more than just > inserting a little text. This will interfere a lot more with my daily > editing than the replaceAll story. 1) There is nothing artificial about replacing one string with another string in a large file using an IDE. 2) Perhaps you should not be calling setRedraw() so much? For one thing, setRedraw() causes the entire control to be painted even if you only change one pixel. In the case of "replace a with a1", it seems clear that the user expects the whole thing to redraw so setRedraw() makes sense. Can you classify the various operations and decide in advance whether it should be called? What happens on Windows when you don't call it? Does it get "slow" and "draw too much" like GTK. This would be good information for us (SWT). Thanks!
Will release the fix to TextViewer right tomorrow morning (CET). > 2) Perhaps you should not be calling setRedraw() so much? For one thing, > setRedraw() causes the entire control to be painted even if you only change > one pixel. In the case of "replace a with a1", it seems clear that the user > expects the whole thing to redraw so setRedraw() makes sense. Can you > classify the various operations and decide in advance whether it should be > called? What happens on Windows when you don't call it? Does it get "slow" > and "draw too much" like GTK. This would be good information for us (SWT). Hm. We could go over the places where we call setRedraw and evaluate whether it's worth the cost everywhere we do - generally we do it to avoid seeing a multi-edition in many places (e.g. when commenting a block of code, we don't want the insertion of "//" at the beginning of every line to be visually inserted after each other, but rather all at the same moment). Perhaps we could come up with a heuristic about the "magnitude" of a change - but then, computing it should not be expensive either... I'll keep you informed of any findings.
I think that "//" is a good case. For example, someone could comment a couple of lines or comment an entire file. I think the issue here is that you need to know which calls cause scrolling and which do not. For calls that do not cause scrolling, setRedraw() should not be called. Instead, the control should just get damaged appropriately and redraw only the damaged area during the next paint. I wouldn't worry about a "heuristic magnitude" for now. Control.setRedraw() is still unimplemented on GTK and Motif (there is a partial hack on Mac) so doing heuristics to decide not to call it on these platforms won't do much. I hope this thing goes away soon because it is the poster boy for "GTK is slow".
released TextViewer.java, removed setRedraw bypass for gtk.
Tom, does it fix it? If so CLOSE THIS PR! Thanks.
Today's nightly failed, so there's no SWT drop I can try. We'll be fine for the I-build.
fixed >= 20040306
Whoooopie!
