Bugzilla – Full Text Bug Listing
| Summary: | [painting] "Show whitespace character" option makes the editor very slow on Mac | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] Platform | Reporter: | Lucas Galfaso <lgalfaso> | ||||||||||||
| Component: | SWT | Assignee: | Scott Kovatch <skovatch> | ||||||||||||
| Status: | RESOLVED FIXED | QA Contact: | Silenio Quarti <Silenio_Quarti> | ||||||||||||
| Severity: | normal | ||||||||||||||
| Priority: | P3 | CC: | daniel_megert, eclipse.org, markus.kell.r, martin.fleurke, paul.serby, remy.suen, skovatch | ||||||||||||
| Version: | 3.6 | ||||||||||||||
| Target Milestone: | 3.7 | ||||||||||||||
| Hardware: | PC | ||||||||||||||
| OS: | Mac OS X | ||||||||||||||
| Whiteboard: | |||||||||||||||
| Attachments: |
|
||||||||||||||
|
Description
Lucas Galfaso
Likely an SWT an issue but will let Text assess the matter. Do you have this problem with the Carbon build? I tested this with Cocoa 64bit Cocoa 32bit Carbon 32bit The issue is very prominent on both Cocoa versions (the editor it is close to unusable with this flag on) and less prominent on Carbon 32bit (it is usable in this environment with this flag on, but not even close to as smooth as with this flag off) In all cases, when this flag is off, the scrolling and the editor itself is very responsive. - Which editor (text, Java, ...)? - How big is the file? - "very slow/responsive" is very subjective. Can you be a bit more precise. - If it's really very slow, please make several stack dumps and attach them here. I tested this with a relatively small text file (less than 100 lines and less than 10k bytes in total) Eclipse 64bit Cocoa built 20100617-1415 Test environment: 2.5 GHz Core 2 duo, 4 GB I made no tweaks whatsoever to the eclipse.ini nor any other configuration file Here are the statistics on some simple action: - When in another application, and switching to Eclipse, waiting until the screen is refreshed: 1.2 seconds - In the text editor Page down: 1.2 seconds - Selecting the entire text in the file: 1.5 seconds - Moving one line up: 1.3 seconds - Undo typing one line: 0.8 seconds Did the same thing with a file that is 5 times bigger, the time that it took to do the same actions was about 3 times worst for most actions. With the "show whitespace" flag off, the response time was so fast I was not able to time it. Created attachment 174618 [details]
Four thread dumps when performing these actions
Looks like getting the font metrics is slow.
>at org.eclipse.swt.internal.cocoa.OS.objc_msgSend(Native Method)
>at org.eclipse.swt.internal.cocoa.NSMutableAttributedString.setAttributedString(NSMutableAttributedString.java:61)
>at org.eclipse.swt.graphics.GC.getFontMetrics(GC.java:2537)
I have what looks like a good fix. It's not necessary to haul in the text layout engine for font metrics. Created attachment 174692 [details]
Potential fix
Rewrite of string drawing to not use a layout manager. For simple string drawing it's not necessary.
Bug occurs also on Linux (Ubuntu 10.04 LTS, Eclipse Helios), so not Mac related. Performance increases when you make the visible part of the editor smaller (resize eclipse or the editor pane). And if you type outside php tags, there is no problem. The fix looks Mac specific. Maybe you are seeing bug 246196? And there's bug 26080 for QNX. When I uncheck the 'show whitespace', the problem is gone. Typing is fast again. So it seems this is the same bug. (In reply to comment #13) > When I uncheck the 'show whitespace', the problem is gone. Typing is fast > again. So it seems this is the same bug. But this bug is for the problem (and fix) on Macs. You are using Linux. The backend for this particular bug is not platform-agnostic. The symptoms may be identical but the fix is not. That is correct. So I can file a new bug, or this bug can be expanded. I.m.h.o. a new bug would be appropriate if the cause of the bug under linux and mac is totally different. I think this needs further investigation; having a patch for Mac does not automatically mean that the code is unrelated. There might be similar code for Linux. But I haven't seen any Eclipse code yet, so I can't tell. But if you (or Scott) prefers a new bug, I'm happy to create one. (In reply to comment #15) > I.m.h.o. a new bug would be appropriate if the cause of the bug under linux and > mac is totally different. I think this needs further investigation; having a > patch for Mac does not automatically mean that the code is unrelated. There > might be similar code for Linux. But I haven't seen any Eclipse code yet, so I > can't tell. > But if you (or Scott) prefers a new bug, I'm happy to create one. Yes, please file a new bug. I'm not that fluent in GTK, so it will likely need its own investigation. That said, if you are seeing this in both platforms there's probably some code that needs to change in both the SWT and the text editor. My patch shows definite improvements in scrolling and drawing on OS X Cocoa, but it doesn't make the difference from not having the whitespace there zero. I also didn't look at WhitespaceCharacterPainter to see if there were any improvements to be had there. (In reply to comment #8) > Created an attachment (id=174692) [details] > Potential fix > Rewrite of string drawing to not use a layout manager. For simple string > drawing it's not necessary. This is how GC drew/measured strings up to revision 1.71. Take a look a bug#270531 and bug#290373. I think this patch will introduce those bugs back. (In reply to comment #17) > (In reply to comment #8) > > Created an attachment (id=174692) [details] [details] > > Potential fix > > Rewrite of string drawing to not use a layout manager. For simple string > > drawing it's not necessary. > > This is how GC drew/measured strings up to revision 1.71. Take a look a > bug#270531 and bug#290373. I think this patch will introduce those bugs back. I wondered about that too, but the root problem in 1.71 was getFontMetrics, and in particular, the fact that NSFont methods that give you the ascender and descender are almost always wrong -- you have to measure a string and compute it. This patch doesn't put the old code back -- it works backwards from the size of the string like we do now. I do need to use NSFont.descender(), which I have found so far to be always correct. (In reply to comment #17) > This is how GC drew/measured strings up to revision 1.71. Take a look a > bug#270531 and bug#290373. I think this patch will introduce those bugs back. Damn. Bug 270531 has defeated me. The string measurements calculated by NSAttributedString/NSString is way off compared to what TextLayout calculates, using NSLayoutManager. And there's no way I'm going to rewrite TextLayout. Nevertheless, I still think we could improve GC.getFontMetrics() to not make so many attributed strings, since the average width isn't going to change for a given font. Maybe we can cache the average width in the font somehow. Created attachment 174811 [details] Better fix Fix caches average char width so we don't create an NSAttributedString, char array and NSString on every call to GC.getFontMetrics. Also adds fix for bug 276644, by calling setUsesScreenFonts(false) on the layout managers. >That said, if you are seeing this in both platforms >there's probably some code that needs to change in both the SWT and the text >editor. We could probably cache the GC's font metrics but: - Shouldn't the GC do that for me? Currently a new font metrics is created on each call to GC.getFontMetrics() but that is not specified in its Javadoc. - With each draw event we get a new GC where we have to update the cache. SWT could be smarter to reuse the font metrics and set it to the new GC to even further cache the info. See also bug 26080 where something along that lines has been suggested. Created attachment 174860 [details]
patch
Looking at our patch made me realize that the code in GC.textExtent() should be calling GC.createString() with false as the draw flag. If we make that change, your patch would be wrong. If I understand correctly, the changes you made in createString(), your to avoid the extra work done for delimiters and tabs when calculating the font metrics. This patch should have the same effect and also improve the performance of GC.textExtent().
I forgot to mentioned. The change to call setUsesScreenFont() makes the line for print margin be wrong for Monaco 11 (See bug#29073). (In reply to comment #21) > We could probably cache the GC's font metrics but: > - Shouldn't the GC do that for me? Currently a new font metrics is created on > each call to GC.getFontMetrics() but that is not specified in its Javadoc. > - With each draw event we get a new GC where we have to update the cache. SWT > could be smarter to reuse the font metrics and set it to the new GC to even > further cache the info. See also bug 26080 where something along that lines > has been suggested. On second look I think we could be more aggressive about caching the font metrics. For any given font the values won't change after we calculate them once, so yes, we should be able to cache an entire FontMetrics object in the Font. There's going to be some hit in getting the ascent and descent of that string. That might be enough to make a performance win by itself. (In reply to comment #23) > I forgot to mentioned. The change to call setUsesScreenFont() makes the line > for print margin be wrong for Monaco 11 (See bug#29073). This seems to be happening for me whether setUsesScreenFont is on or off. I'll have to see if there's a problem with the font metric caching. (In reply to comment #25) > (In reply to comment #23) > > I forgot to mentioned. The change to call setUsesScreenFont() makes the line > > for print margin be wrong for Monaco 11 (See bug#29073). I've opened up a can of worms with the whole setUsesScreenFont thing, so I'm leaving it out for now. It's not relevant to this bug anyway. Bug 276644 tracks that. Created attachment 174921 [details]
final patch
Added FontMetrics caching to the Font object. This seems to be helping for me.
Code seems good to me. But the line attribStr.initWithString(NSString.stringWith(s), dict); should reassign attribStr to be in the safe side. I have seem cases where the init() methods return a different object than "self" and that really cause problems. (In reply to comment #28) > Code seems good to me. But the line > > attribStr.initWithString(NSString.stringWith(s), dict); > > should reassign attribStr to be in the safe side. I have seem cases where the > init() methods return a different object than "self" and that really cause > problems. Good point. I'll just combine it all into one call like we do in createString -- that's what I intended to do in the first place. Fixed > 20100721. We may want to consider this for 3.6.1; it should provide a general performance boost. . Bug 320595 has been created for the same issue on Linux. > We may want to consider this for 3.6.1;
Sounds good to me.
I'm getting problems of a similar nature in 3.6.2. https://bugs.eclipse.org/bugs/show_bug.cgi?id=323401 Could this be related? I can confirm enabling "Show whitespace characters" slows down Eclipse 2021-03 (I haven't updated yet) on macOS 11.6.2. |