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.

Sign up for GitHub

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

Jump to bottom

Make CodepointWidthDetector::GetWidth faster #3727

Merged
3 commits merged into from
Apr 4, 2020
Merged

Make CodepointWidthDetector::GetWidth faster #3727

3 commits merged into from
Apr 4, 2020

Conversation

skyline75489
Copy link
Collaborator

@skyline75489 skyline75489 commented Nov 27, 2019 •
edited by DHowett-MSFT
Loading

This is a subset of #3578 which I think is harmless and the first step towards making things right.
References #3546 #3578

Detailed Description of the Pull Request / Additional comments

For more robust Unicode support, CodepointWidthDetector should provide concrete width information rather than a simple boolean of IsWide. Currently only IsWide is widely used and optimized using quick lookup table and fallback cache. This PR moves those optimization into GetWidth.

Validation Steps Performed

API remains unchanged. Things are not broken.

@skyline75489
Copy link
Collaborator Author

skyline75489 commented Nov 27, 2019 •
edited
Loading

And by the way, isn't it just way more natural to implement IsWide as return GetWidth(glyph) == CodepointWidth::Wide? Code reuse, people. Am I right?

@zadjii-msft
Copy link
Member

(For the record, as it is Thanksgiving over here in the US, there's a good chance that no one reads this PR till next week. We're not just ghosting it 😋)

@jsoref
Copy link
Contributor

jsoref commented Apr 2, 2020

@miniksa : out of curiosity, what happened to this PR? (I'm mostly looking through PRs to make sure my action isn't making a mess of things)

@miniksa
Copy link
Member

miniksa commented Apr 3, 2020

@miniksa : out of curiosity, what happened to this PR? (I'm mostly looking through PRs to make sure my action isn't making a mess of things)

@jsoref, what happened = I spaced and now it's April.

miniksa
miniksa approved these changes Apr 3, 2020
View reviewed changes
Copy link
Member

@miniksa miniksa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fine. I'm sorry it took months and a pandemic to review it.

@miniksa miniksa added the Needs-Second It's a PR that needs another sign-off label Apr 3, 2020
@miniksa miniksa added the Area-Performance Performance-related issue label Apr 3, 2020
@ghost ghost requested a review from leonMSFT April 3, 2020 16:13
zadjii-msft
zadjii-msft approved these changes Apr 3, 2020
View reviewed changes
Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh wow yea this is better. Sorry for saying "we're not ghosting it" and then ghosting it 👻

@DHowett-MSFT
Copy link
Contributor

Pushed a merge to trigger new CI and friends.

@DHowett-MSFT
Copy link
Contributor

Thanks so much for rewriting this guy. 😄

@DHowett-MSFT
Copy link
Contributor

Actually, it looks like GetWidth still shares the pfnFallback logic with _lookup..WithCache .. is there a chance for us to make this even more streamlined?

@skyline75489
Copy link
Collaborator Author

@DHowett-MSFT I wish I could. I think the pfnFallback logic is delibrate. It reduces the need for the _lookupGlyphWidth call, which is expensive, when there is a fallback cache.

Actually the entire _fallbackCache should be rewrited to reflect the actual width information instead of booleans. The rewriting does not really fit in this PR. So I intended to leave it as is.
But rewriting _fallbackCache should be what I was heading to.

Honestly this PR feels vague to me at first. Took me a while to figure out what I was trying to accomplish.

@DHowett-MSFT
Copy link
Contributor

Thanks for paging it back into your memory. That all makes sense, I’ll mark it for merge :)

@skyline75489
Copy link
Collaborator Author

@miniksa @zadjii-msft It's all right. Back then when I wrote this PR I was still trying to crack job interviews. And here we are. Life is like a box of chocolates. You never know what you gonna get. Just like you never know the difference between the meaning of "ghosing" in dictionary and in real life. (just kidding, you know how I enjoy working with you guys 😸 )

@DHowett-MSFT DHowett-MSFT added the AutoMerge Marked for automatic merge by the bot when requirements are met label Apr 4, 2020
@ghost
Copy link

ghost commented Apr 4, 2020

Hello @DHowett-MSFT!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@ghost ghost merged commit 4f8acb4 into microsoft:master Apr 4, 2020
DHowett-MSFT pushed a commit that referenced this pull request Apr 14, 2020
@skyline75489 @DHowett
Make CodepointWidthDetector::GetWidth faster (#3727)
875680a 
This is a subset of #3578 which I think is harmless and the first step towards making things right.
References #3546 #3578 

## Detailed Description of the Pull Request / Additional comments

For more robust Unicode support, `CodepointWidthDetector` should provide concrete width information rather than a simple boolean of `IsWide`. Currently only `IsWide` is widely used and optimized using quick lookup table and fallback cache. This PR moves those optimization into `GetWidth`.

## Validation Steps Performed

API remains unchanged. Things are not broken.
@ghost
Copy link

ghost commented Apr 22, 2020

🎉Windows Terminal Preview v0.11.1121.0 has been released which incorporates this pull request.:tada:

Handy links:

@skyline75489 skyline75489 deleted the fix/codepointWidthDetectorPart1 branch February 9, 2021 06:10
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@miniksa miniksa miniksa approved these changes

@zadjii-msft zadjii-msft zadjii-msft approved these changes

@carlos-zamora carlos-zamora Awaiting requested review from carlos-zamora

@DHowett-MSFT DHowett-MSFT Awaiting requested review from DHowett-MSFT

@leonMSFT leonMSFT Awaiting requested review from leonMSFT

Assignees
No one assigned
Labels
Area-Performance Performance-related issue AutoMerge Marked for automatic merge by the bot when requirements are met Needs-Second It's a PR that needs another sign-off
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@skyline75489 @zadjii-msft @jsoref @miniksa @DHowett-MSFT @DHowett