go-runewidth
go-runewidth copied to clipboard
Make RuneWidth faster for code points below 0x0300
RuneWidth is an extremely hot codepath for some software. This PR introduces a fast path optimization that avoids table lookups for code points below 0x0300.
It exploits the fact that runes below 0x0300 cannot be combining, non-assigned or double-width code points. For such code points, the modified function is much faster than the existing code, while being essentially the same speed for other code points, and remaining unchanged functionally (please double-check my logic here).
This optimization is used in this downstream PR for the Micro text editor and substantially improves buffer operations performance.
Can you help me understand the failed test? It seems to indicate that the symbol is supposed to have a width of 0, even though IsAmbiguousWidth returns true. How can a code point have a width of zero, while also having ambiguous width?
Codecov Report
Merging #42 into master will decrease coverage by
2.35%. The diff coverage is63.63%.
@@ Coverage Diff @@
## master #42 +/- ##
===========================================
- Coverage 100.00% 97.64% -2.36%
===========================================
Files 2 2
Lines 160 170 +10
===========================================
+ Hits 160 166 +6
- Misses 0 4 +4
| Impacted Files | Coverage Δ | |
|---|---|---|
| runewidth.go | 97.12% <63.63%> (-2.88%) |
:arrow_down: |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact),ø = not affected,? = missing dataPowered by Codecov. Last update 14e809f...3e1705c. Read the comment docs.
Fixed. It turns out that combining characters are considered to be "ambiguous width", probably because they have a non-zero width when rendered by themselves.
I have adjusted the logic accordingly, and all tests pass now. The order of checks inside the fast path now also matches the order of checks in the function itself, making this change easier to reason about.
Ready for review.
@p-e-w There is a mistake in go-runewidth that is fixed by #44, but it has not been merged. Could you combine both PRs into one and notify mattn? I am a happy micro user too. Thanks for your work!
Thanks your contribution. Please add tests for this change.
@mattn What exactly would I test for here? This PR doesn't alter anything functionally. It only makes the existing code faster. The fact that RuneWidth still works the same way as before should be covered by the existing tests, right?
@p-e-w Test Engineer here. Using a more efficient algorithm does not imply that the functionality remains intact. But I agree with you that the existing functional tests should catch any regression. That is what tests are for ;)
@mattn Can you run your test suite and verify whether this PR introduce any regression?
Is this still needed after #47?
@mardukbp What is broken? Please provide information?
@mattn I described the issue in #45. When #44 was merged that issue was fixed.