Make CodepointWidthDetector::GetWidth faster by skyline75489 · Pull Request #3727 · microsoft/terminal

skyline75489 · 2019-11-27T03:27:10Z

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.

src/types/CodepointWidthDetector.cpp

skyline75489 · 2019-11-27T03:36:58Z

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 · 2019-11-27T14:23:33Z

(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 · 2020-04-02T22:31:25Z

@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 · 2020-04-03T16:10:41Z

@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

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

zadjii-msft

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

DHowett-MSFT · 2020-04-03T19:30:58Z

Pushed a merge to trigger new CI and friends.

DHowett-MSFT · 2020-04-03T19:31:33Z

Thanks so much for rewriting this guy. 😄

DHowett-MSFT · 2020-04-03T20:09:28Z

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 · 2020-04-04T00:44:27Z

@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 · 2020-04-04T00:54:28Z

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

skyline75489 · 2020-04-04T00:55:18Z

@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 😸 )

ghost · 2020-04-04T00:56:10Z

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.

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 · 2020-04-22T17:12:03Z

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

Handy links:

Make CodepointWidthDetector::GetWidth faster

ef5bf91

skyline75489 commented Nov 27, 2019

View reviewed changes

src/types/CodepointWidthDetector.cpp Show resolved Hide resolved

Fix ambiguous detection

ed79d7f

skyline75489 force-pushed the fix/codepointWidthDetectorPart1 branch from abc676a to ed79d7f Compare November 27, 2019 05:02

zadjii-msft requested a review from miniksa November 27, 2019 14:22

miniksa approved these changes Apr 3, 2020

View reviewed changes

miniksa added the Needs-Second It's a PR that needs another sign-off label Apr 3, 2020

ghost requested review from DHowett-MSFT, carlos-zamora and zadjii-msft April 3, 2020 16:12

miniksa added the Area-Performance Performance-related issue label Apr 3, 2020

ghost requested a review from leonMSFT April 3, 2020 16:13

zadjii-msft approved these changes Apr 3, 2020

View reviewed changes

Merge remote-tracking branch 'origin/master' into skyline

877cc21

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

ghost merged commit 4f8acb4 into microsoft:master Apr 4, 2020

skyline75489 deleted the fix/codepointWidthDetectorPart1 branch February 9, 2021 06:10

This pull request was closed.

Conversation

skyline75489 commented Nov 27, 2019 • edited by DHowett-MSFT Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

Uh oh!

Uh oh!

skyline75489 commented Nov 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zadjii-msft commented Nov 27, 2019

Uh oh!

jsoref commented Apr 2, 2020

Uh oh!

miniksa commented Apr 3, 2020

Uh oh!

miniksa left a comment

Choose a reason for hiding this comment

Uh oh!

zadjii-msft left a comment

Choose a reason for hiding this comment

Uh oh!

DHowett-MSFT commented Apr 3, 2020

Uh oh!

DHowett-MSFT commented Apr 3, 2020

Uh oh!

DHowett-MSFT commented Apr 3, 2020

Uh oh!

skyline75489 commented Apr 4, 2020

Uh oh!

DHowett-MSFT commented Apr 4, 2020

Uh oh!

skyline75489 commented Apr 4, 2020

Uh oh!

ghost commented Apr 4, 2020

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.

Uh oh!

ghost commented Apr 22, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

skyline75489 commented Nov 27, 2019 •

edited by DHowett-MSFT

Loading

skyline75489 commented Nov 27, 2019 •

edited

Loading

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.