go-runewidth icon indicating copy to clipboard operation
go-runewidth copied to clipboard

Make RuneWidth faster for code points below 0x0300

Open p-e-w opened this issue 5 years ago • 10 comments

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.

p-e-w avatar Jun 12 '20 07:06 p-e-w

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?

p-e-w avatar Jun 12 '20 07:06 p-e-w

Codecov Report

Merging #42 into master will decrease coverage by 2.35%. The diff coverage is 63.63%.

Impacted file tree graph

@@             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 data Powered by Codecov. Last update 14e809f...3e1705c. Read the comment docs.

codecov-commenter avatar Jun 13 '20 03:06 codecov-commenter

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 avatar Jun 13 '20 03:06 p-e-w

@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!

mardukbp avatar Oct 10 '20 22:10 mardukbp

Thanks your contribution. Please add tests for this change.

mattn avatar Oct 11 '20 06:10 mattn

@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 avatar Jan 23 '21 05:01 p-e-w

@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?

mardukbp avatar Jan 29 '21 21:01 mardukbp

Is this still needed after #47?

makew0rld avatar Oct 21 '21 00:10 makew0rld

@mardukbp What is broken? Please provide information?

mattn avatar Oct 21 '21 00:10 mattn

@mattn I described the issue in #45. When #44 was merged that issue was fixed.

mardukbp avatar Oct 22 '21 12:10 mardukbp