-
Notifications
You must be signed in to change notification settings - Fork 432
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
Add fast path skipping grapheme counting #2817
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah good call
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
behavior and overall approach looks good to me! this particular set of optimizations is on a relatively hot path and improves performance in the common case and feels worthwhile.
we'll probably want to do the same thing in the go implementation, and can copy these test cases (maybe make them "shared interop test vectors") when we do.
Pushed style tweaks per review. Have not verified that it's equivalent. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lookin good 👍
gonna toss a changeset in here then we can merge
Not a huge deal but since we visit this codepath for every single validated string, seems like we'd want to avoid any unnecessary computation here, especially on low-end devices.
I don't know how encodings and grapheme work very well so this needs a close look.
Commits
Before
This was showing up in a profile for switching tabs with 6x throttling. (Note this is a DEV profile so in PROD its % would be higher compared to other things around it.)
After
It doesn't show up.