Bugzilla – Full Text Bug Listing
| Summary: | SWT StyledText scrolling is very slow for text with lots of commas | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] Platform | Reporter: | Martin Robertson <mchr3k> | ||||||||||
| Component: | SWT | Assignee: | Felipe Heidrich <eclipse.felipe> | ||||||||||
| Status: | CLOSED DUPLICATE | QA Contact: | |||||||||||
| Severity: | critical | ||||||||||||
| Priority: | P3 | CC: | eclipse.felipe, ericwill, jonah, loskutov, mchr3k, Silenio_Quarti, simeon.danailov.andreev | ||||||||||
| Version: | 4.1 | ||||||||||||
| Target Milestone: | 4.16 M3 | ||||||||||||
| Hardware: | PC | ||||||||||||
| OS: | All | ||||||||||||
| See Also: |
https://git.eclipse.org/r/148051 https://bugs.eclipse.org/bugs/show_bug.cgi?id=536562 |
||||||||||||
| Whiteboard: | |||||||||||||
| Bug Depends on: | |||||||||||||
| Bug Blocks: | 577238 | ||||||||||||
| Attachments: |
|
||||||||||||
|
Description
Martin Robertson
Created attachment 200231 [details]
Test text files
Created attachment 200232 [details]
Test java class
Created attachment 200234 [details]
Enhanced test class
The output from running this code on my Windows7 machine is as follows:
Time to scroll space separated text: 4946
Time to scroll comma separated text: 29001
This shows the comma separated text is more than 5x slower to scroll.
I have cleaned up my example class and put it on github along with a pre-build cross platform runnable SWT jar. There are instructions on the page linked below to run the example on your own platform. https://github.com/mchr3k/Bugs/tree/master/StyledTextScroll Note that in the case where whitespaces are used as delimiters Uniscribe will put all chars for the line in one single run. When commas are used, Unicribe will put each comma in a run, so basically you will have one run for each characters in the line. SWT needs to handle each run separately, I don't think there is nothing I can do here. Can you give a little extra background please? - What is Uniscribe? Part on SWT/Java/Windows? - What is a "run"? - Why are runs split around commas? Could this be optional? (In reply to comment #6) > Can you give a little extra background please? > - What is Uniscribe? Part on SWT/Java/Windows? is the native technology we use to draw text http://msdn.microsoft.com/en-us/library/dd374091(v=vs.85).aspx http://en.wikipedia.org/wiki/Uniscribe > - What is a "run"? "run A run is a passage of text for Uniscribe to render. It should have a single style, that is, font, size, and color, but can be drawn from a variety of scripts. A run can contain both left-to-right and right-to-left content." see http://msdn.microsoft.com/en-us/library/dd374094(v=VS.85).aspx > - Why are runs split around commas? Could this be optional? I didn't implement Uniscribe so I don't know for sure, but I suspect that they did that to handle bidi reordering. Thanks for the clarifications. I have taken a closer look at the SWT source and found the key OS calls are in the following method. org.eclipse.swt.graphics.itemize() This calls into the native method OS.ScriptItemize(...) which calls through to the Windows specific code: http://msdn.microsoft.com/en-us/library/dd368556%28v=vs.85%29.aspx The settings for this method come from the SCRIPT_CONTROL struct. However, the defn of this struct used by SWT is missing the fMergeNeutralItems field. This sounds promising to me but I haven't managed to find anything clear online about whether this field would merge together the returned runs for my comma heavy text. I've raised this issue on StackOverFlow in case anyone else has hit similar issues with Uniscribe. http://stackoverflow.com/questions/6822170/how-to-reduce-the-number-of-runs-returned-from-uniscribe-scriptitemize The reason fMergeNeutralItems is not in our java code is because it didn't exist when we first implemented TextLayout,
This is from usp10.h from Win SDK 2003
typedef struct tag_SCRIPT_CONTROL {
DWORD uDefaultLanguage :16; // For NADS, also default for context
DWORD fContextDigits :1; // Means use previous script instead of uDefaultLanguage
// The following flags provide legacy support for GetCharacterPlacement features
DWORD fInvertPreBoundDir :1; // Reading order of virtual item immediately prior to string
DWORD fInvertPostBoundDir :1; // Reading order of virtual item immediately following string
DWORD fLinkStringBefore :1; // Equivalent to presence of ZWJ before string
DWORD fLinkStringAfter :1; // Equivalent to presence of ZWJ after string
DWORD fNeutralOverride :1; // Causes all neutrals to be strong in the current embedding direction
DWORD fNumericOverride :1; // Causes all numerals to be strong in the current embedding direction
DWORD fLegacyBidiClass :1; // Causes plus and minus to be reated as neutrals, slash as a common separator
DWORD fReserved :8;
} SCRIPT_CONTROL;
This is from the same file Win SDK 6.0 (Vista)
typedef struct tag_SCRIPT_CONTROL {
DWORD uDefaultLanguage :16; // For NADS, also default for context
DWORD fContextDigits :1; // Means use previous script instead of uDefaultLanguage
// The following flags provide legacy support for GetCharacterPlacement features
DWORD fInvertPreBoundDir :1; // Reading order of virtual item immediately prior to string
DWORD fInvertPostBoundDir :1; // Reading order of virtual item immediately following string
DWORD fLinkStringBefore :1; // Equivalent to presence of ZWJ before string
DWORD fLinkStringAfter :1; // Equivalent to presence of ZWJ after string
DWORD fNeutralOverride :1; // Causes all neutrals to be strong in the current embedding direction
DWORD fNumericOverride :1; // Causes all numerals to be strong in the current embedding direction
DWORD fLegacyBidiClass :1; // Causes plus and minus to be reated as neutrals, slash as a common separator
DWORD fMergeNeutralItems :1; // Causes merging neutral characters into strong items, when possible
DWORD fReserved :7;
} SCRIPT_CONTROL;
(Note that they used a bit from fReserved for that)
I will try setting the bit.
try adding 'scriptControl.fReserved = 1;' before the call to OS.ScriptItemize(chars,...) does that fix your problem ? OK I stepped through in the debugger and set OK I stepped through in the debugger and set 'scriptControl.fReserved = 1'. The String being processed was "1,2,3,4,5,". Without this change, the number of runs returned was 10. With fReserved set to 1 the number of runs returned was 1. I haven't rebuilt SWT to always set fReserved = 1 so I can't easily check the performance improvement. However, hopefully this will significantly speed up rendering of comma heavy text. Regarding next steps, will you want to default fReserved to 1 or make that an extra option on the StyledControl object? Okay, usually this type of change introduces new bugs. I googled fMergeNeutralItems and I found no hits regarding performance improvements or problems. MSDN also doesn't say much. That said, we are at good point in the release cycle to try this type of change. Hopefully it will good performance and won't cause trouble. I tested wrapping, bidi, and font substituion. So far looks good. Let me know if you need any help with testing this. Can you confirm what URL I should use to download dev builds of SWT? I would be keen to start using a build of SWT with a fix for this bug ASAP. (In reply to comment #13) > Regarding next steps, will you want to default fReserved to 1 or make that an > extra option on the StyledControl object? It should always be set I believe. Agreed ? The tricky part is how to set it. I can't add fMergeNeutralItems to SCRIPT_CONTROL cause we need to compile against old version of the SDK where fMergeNeutralItems does not exist. Setting 1 to fReserved looks funny cause it is not taking byte order in consideration (maybe it fails on a big endian machine...). Hopefully it is okay cause fReserved is the size of byte and it is an atomic unit. One option that can be better is add a variation of SCRIPT_CONTROL that has fReserved and add a version of ScriptItemize that takes pointer (int /*long*/) instead of a struct (SCRIPT_CONTROL). I will talk to Silenio about this problem. (In reply to comment #15) > Let me know if you need any help with testing this. > Can you confirm what URL I should use to download dev builds of SWT? I would be > keen to start using a build of SWT with a fix for this bug ASAP. Thank you very much for not giving up and investigating this problem further. Last time I checked MSDN I didn't see the new flag and without your help probably I would never see it. I will take care of this problem now, I will let you know when a build including the change is available. If this new field was only added in Win SDK 6.0 (Vista) does that mean that only Vista and newer will benefit from the fix or can newer Win SDKs be installed on older version of Windows in a similar way to installing .NET 4.0 on Windows XP? Created attachment 200552 [details]
c snippet to help test byte order
I was not certain that setting 1 in the fReserved was the right thing to do. So I decided to test it.
I tried the snippet.c in LSB machine and in a MSB machine.
Linux X86:
[felipe@wsfheidrichlnx motif]$ gcc -o t snippet.c
[felipe@wsfheidrichlnx motif]$ file t
t: ELF 32-bit LSB executable, Intel 80386, version 1 (SYSV), dynamically linked (uses shared libs), for GNU/Linux 2.6.32, not stripped
[felipe@wsfheidrichlnx motif]$ ./t
Zero=0
One=1
-One=1
0x80=0
Linux PowerPC:
-sh-3.00$ gcc -o t snippet.c
-sh-3.00$ file t
t: ELF 32-bit MSB executable, PowerPC or cisco 4500, version 1 (SYSV), for GNU/Linux 2.2.5, dynamically linked (uses shared libs), not stripped
-sh-3.00$ ./t
Zero=0
One=0
-One=1
0x80=1
---
Note that in a LSB machine one works fine (fMergeNeutralItems is set), on the MSB machine 0x80 sets fMergeNeutralItems.
(In reply to comment #17) > (In reply to comment #15) > > Let me know if you need any help with testing this. > > Can you confirm what URL I should use to download dev builds of SWT? I would be > > keen to start using a build of SWT with a fix for this bug ASAP. > Thank you very much for not giving up and investigating this problem further. > Last time I checked MSDN I didn't see the new flag and without your help > probably I would never see it. > I will take care of this problem now, I will let you know when a build > including the change is available. the fix would only affect runtimes where the bit exists, it was not mention in the doc (MSDN) when the bit was added so I assuming Windows Vista provided that the Windows Vista SDK was the first I saw that had the bit in the header file. Should we care about testing for byte order in this case ?
There are places where we assume the runtime is LSB on windows already (and MSB for WinCE).
I saw this code in GTK being used to detect byte order:
byte[] buffer = new byte[1];
int /*long*/ ptr = OS.malloc(4);
OS.memmove(ptr, new int[]{1}, 4);
OS.memmove(buffer, ptr, 1);
OS.free(ptr);
boolean bigendian = buffer[0] == 0;
Do you know any other technique to set that new field in the struct ?
Fixed: http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?id=82a54cc43807fc82d1a81b73000c952bbf4bb0b8 Note, one potential problem with this fix is that we will have less items but we will have bigger items, there is a known problem in Windows that when items are too big then will not draw. You can see this problem using your enhanced test case by replacing the line: "int max = r.nextInt(2000);" with "int max = 1024;" This fix causes bug#377472. I removed this optimization. We observe this in a 2MB source file with some long lines on RHEL 7.4 with GTK 3.22. Unfortunately we cannot share the source file due to IP.
I also see that a lot of time is spent retrieving line counts.
org.eclipse.swt.custom.StyledTextRenderer.getTextLayout(int, int, int, int) has some weird code:
int lineCount = layout.getLineCount();
int height = getLineHeight();
for (int i = 0; i < lineCount; i++) {
int lineHeight = layout.getLineBounds(i).height;
if (lineHeight > height) {
height = lineHeight;
index = i;
}
}
This is called for every line of the scroll, from org.eclipse.swt.custom.StyledText.handlePaint(Event):
int lineCount = isSingleLine() ? 1 : content.getLineCount();
int x = leftMargin - horizontalScrollOffset;
for (int i = startLine; y < endY && i < lineCount; i++) {
y += renderer.drawLine(i, x, y, gc, background, foreground);
}
So its not particularly surprising that the performance is bad. The code also doesn't look too great, somehow there is a different text layout for every line, yet rendering the line needs to know the total line count, which is highly likely computed based on other lines...
Probably there is a way to compute the line count only once; I've tried passing it down from the paint event but that broke the StyledText scrolling (most of the scrolled area was grey until mouse-over.
When debugging I also see freezes reported here: !ENTRY org.eclipse.ui.monitoring 2 0 2019-08-21 11:28:55.049 !MESSAGE UI freeze of 0.87s at 11:28:54.178 !SUBENTRY 1 org.eclipse.ui.monitoring 1 0 2019-08-21 11:28:55.050 !MESSAGE Sample at 11:28:54.511 (+0.333s) Thread 'main' tid=1 (RUNNABLE) !STACK 0 Stack Trace at org.eclipse.swt.internal.gtk.OS._g_utf16_offset_to_pointer(Native Method) at org.eclipse.swt.internal.gtk.OS.g_utf16_offset_to_pointer(OS.java:601) at org.eclipse.swt.graphics.TextLayout.computeRuns(TextLayout.java:200) at org.eclipse.swt.graphics.TextLayout.getLineCount(TextLayout.java:1012) at org.eclipse.swt.custom.StyledTextRenderer.getTextLayout(StyledTextRenderer.java:1255) at org.eclipse.swt.custom.StyledTextRenderer.getTextLayout(StyledTextRenderer.java:888) at org.eclipse.swt.custom.StyledTextRenderer.calculate(StyledTextRenderer.java:296) at org.eclipse.swt.custom.StyledTextRenderer.calculateClientArea(StyledTextRenderer.java:325) at org.eclipse.swt.custom.StyledText.calculateTopIndex(StyledText.java:1635) at org.eclipse.swt.custom.StyledText.scrollVertical(StyledText.java:8234) at org.eclipse.swt.custom.StyledText.handleVerticalScroll(StyledText.java:6499) at org.eclipse.swt.custom.StyledText.lambda$3(StyledText.java:5832) at org.eclipse.swt.custom.StyledText$$Lambda$332/1954953964.handleEvent(Unknown Source) at org.eclipse.swt.widgets.EventTable.sendEvent(EventTable.java:89) at org.eclipse.swt.widgets.Display.sendEvent(Display.java:5618) at org.eclipse.swt.widgets.Widget.sendEvent(Widget.java:1405) at org.eclipse.swt.widgets.Display.runDeferredEvents(Display.java:4882) at org.eclipse.swt.widgets.Display.readAndDispatch(Display.java:4406) at org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine$5.run(PartRenderingEngine.java:1160) at org.eclipse.core.databinding.observable.Realm.runWithDefault(Realm.java:338) at org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine.run(PartRenderingEngine.java:1049) at org.eclipse.e4.ui.internal.workbench.E4Workbench.createAndRunUI(E4Workbench.java:155) at org.eclipse.ui.internal.Workbench.lambda$3(Workbench.java:635) at org.eclipse.ui.internal.Workbench$$Lambda$35/893281161.run(Unknown Source) at org.eclipse.core.databinding.observable.Realm.runWithDefault(Realm.java:338) at org.eclipse.ui.internal.Workbench.createAndRunWorkbench(Workbench.java:559) at org.eclipse.ui.PlatformUI.createAndRunWorkbench(PlatformUI.java:150) at org.eclipse.ui.internal.ide.application.IDEApplication.start(IDEApplication.java:150) at org.eclipse.equinox.internal.app.EclipseAppHandle.run(EclipseAppHandle.java:203) at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.runApplication(EclipseAppLauncher.java:137) at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.start(EclipseAppLauncher.java:107) at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:400) at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:255) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) at java.lang.reflect.Method.invoke(Method.java:498) at org.eclipse.equinox.launcher.Main.invokeFramework(Main.java:661) at org.eclipse.equinox.launcher.Main.basicRun(Main.java:597) at org.eclipse.equinox.launcher.Main.run(Main.java:1476) at org.eclipse.equinox.launcher.Main.main(Main.java:1449) !SUBENTRY 1 org.eclipse.ui.monitoring 1 0 2019-08-21 11:28:55.050 New Gerrit change created: https://git.eclipse.org/r/148051 StyledText is apparently designed to apply the styles of the entire vertically visible lines (plus some offset below and above the visibility). The freezes on scrolling the long lines in our case occur due to applying all the styles in entire 150k symbol lines (the source file in question has about 30 such lines one after the other). I've tried to reduce the applying of styles to the visible area, however StyledText does not react to horizontal scrolling at all (due to the above mentioned design). We would need to re-work StyledText quite a bit to get better performance out of it. I did not see anything else that can be improved substantially; each time the vertical scrolling area "sees" the 30 lines that are very very long, all the styles are applied. Once those lines leave the vertical visible window (+ offset), the styles are dumped. So scrolling back to those lines again leads to a freeze and so on. I am guessing this is to make source files with 10k+ LOC work. I think the design works well for the standard use case, e.g. lines that fit a normal modern display. Bug 562165 which was merged in version 4.16 enables font ligatures all the time, which was the original fix for this bug before it was reverted because of regression in Bug 377472. AFAICT the problem described in Bug 377472 no longer exists, indicating it may have been a Windows bug that has since been fixed. *** This bug has been marked as a duplicate of bug 562165 *** PS I verified that the string "1,2,3,4,5," now does indeed only create one run. |