[FIX] Change to old withCrLf for better performance #1722
Conversation
|
reduces time from 9 to 4 sec :-) |
bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/widgets/Display.java
Outdated
Show resolved
Hide resolved
bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/widgets/Display.java
Outdated
Show resolved
Hide resolved
| if (length == 0) return string; | ||
|
|
||
| /* | ||
| * Check for an LF or CR/LF and assume the rest of |
There was a problem hiding this comment.
This check could also be left away. Especially since it does not work with partly mixed delimiters and does not improve performance either.
There was a problem hiding this comment.
Not sure about that one. If there is a really long text with correct line breaks (\r\n) this will save some time. For example withCrLf("\r\n".repeat(100_000_000));.
On the other hand, as you said, this is not working with partly mixed delimiters. So I'm not really sure how to proceed here.
There was a problem hiding this comment.
Not sure about that one. If there is a really long text with correct line breaks (\r\n) this will save some time.
did you measure it? Anyhow Functional correctness beats performance requirements
There was a problem hiding this comment.
Using withCrLf("\r\n".repeat(100_000_000)); results in a noticeable performance difference: the operation takes 13ms with the check and 1382ms without it. Given that most strings passed to Display.withCrLf are likely already correctly formatted with proper line breaks, this optimization could significantly impair the overall performance of Eclipse.
There was a problem hiding this comment.
13ms with the check and 1382ms without it.
that sounds like a big difference, but its only that fast because the check stops at the first occurrence. the following handling of the String however will still need to walk through the entire string so that in the broader context it may not be a hotspot but negligible
There was a problem hiding this comment.
Yes exactly. This only saves time for already correctly formatted strings. I removed the check, it's still better than the regex/replaceAll version.
I would suggest to merge it without the check and if we see that it impairs the performance we can change it.
bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/widgets/Display.java
Outdated
Show resolved
Hide resolved
bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/widgets/Display.java
Show resolved
Hide resolved
Christopher-Hermann
force-pushed
the
withCrLf
branch
from
38cec61 to
4cc6304
Compare
Christopher-Hermann
force-pushed
the
withCrLf
branch
2 times, most recently
from
c00f633 to
1f6d70c
Compare
Christopher-Hermann
force-pushed
the
withCrLf
branch
from
1f6d70c to
277c39c
Compare
|
Test seems to be unrelated, but I'm not sure... |
Christopher-Hermann
force-pushed
the
withCrLf
branch
from
277c39c to
a0af9a4
Compare
Changing back to the old withCrLf coding. Regex is causing performance issues. Furthermore, handling of mixed \n and \r\n is improved. Fixes: eclipse-platform#1718
Christopher-Hermann
force-pushed
the
withCrLf
branch
from
a0af9a4 to
9143f85
Compare
This fixes wrong formatting introduced to the Win32 display class in eclipse-platform#1722
This fixes wrong formatting introduced to the Win32 display class in eclipse-platform#1722
This fixes wrong formatting introduced to the Win32 display class in #1722
2 participants
Changing back to the old withCrLf coding. Regex is causing performance issues.
Furthermore, handling of mixed \n and \r\n is improved.
Fixes: #1718