Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make RuneWidth faster for code points below 0x0300 #42

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

p-e-w
Copy link

@p-e-w p-e-w commented Jun 12, 2020

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
Copy link
Author

p-e-w commented Jun 12, 2020

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 p-e-w marked this pull request as draft June 12, 2020 07:58
@codecov-commenter
Copy link

codecov-commenter commented Jun 13, 2020

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%) ⬇️

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.

@p-e-w p-e-w marked this pull request as ready for review June 13, 2020 03:39
@p-e-w
Copy link
Author

p-e-w commented Jun 13, 2020

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.

@mardukbp
Copy link

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

@mattn
Copy link
Owner

mattn commented Oct 11, 2020

Thanks your contribution. Please add tests for this change.

@p-e-w
Copy link
Author

p-e-w commented Jan 23, 2021

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

@mardukbp
Copy link

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

@makew0rld makew0rld mentioned this pull request Feb 27, 2021
@makew0rld
Copy link

Is this still needed after #47?

@mattn
Copy link
Owner

mattn commented Oct 21, 2021

@mardukbp What is broken? Please provide information?

@mardukbp
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants