Initial implementation of fine-grained text analysis#9202
Conversation
|
@miniksa FYI |
zadjii-msft
added
the
Area-Rendering
miniksa
left a comment
miniksa
left a comment
There was a problem hiding this comment.
Generally speaking, this is exactly what I was meaning yesterday when I described the first attempt at this. Thanks for taking a shot at it.
ghost
added
the
Needs-Author-Feedback
ghost
removed
the
Needs-Author-Feedback
| RETURN_IF_FAILED(hr); | ||
| _SetCurrentRun(pos); | ||
| _SplitCurrentRun(pos); | ||
| pos += std::max(uiLengthRead, 1u); |
There was a problem hiding this comment.
This should handle the situation when uiLengthRead is 0.
|
Overall I think this is ready, even without the optimization for AnalyzeFontFallback. I’ve been testing it with cmatrix, cacafire, CJK content and everything, and found no obvious regression in rendering result. That being said, I hate to see another regression roller coaster like the one in #6695. @miniksa if you’re OK landing this, please guide me through the process to avoid any pitfall I don’t know. |
ghost
added
Issue-Task
|
Weird, why did the bot tag this PR with |
|
Oh I forgot to mention. This PR will save ~3% of CPU when running cmatrix. |
Uh, it is tagged as conhost?
|
|
Ha, you got me. Well if we're going to use DX renderer in conhost eventually in the future, I guess the tag makes sense. Also to be more specific about the CPU usage. About ~3% of CPU is saved in the analysis process. The overall CPU usage is roughly the same, but more cycles can be used in the rendering process. So the terminal is theoretically faster. |
ghost
requested review from
DHowett,
PankajBhojwani,
carlos-zamora,
leonMSFT and
zadjii-msft
zadjii-msft
left a comment
zadjii-msft
left a comment
There was a problem hiding this comment.
I can't believe I never gave this the ✔️
DHowett
left a comment
DHowett
left a comment
There was a problem hiding this comment.
If Michael and Mike trust this, I am comfortable with it. Thank you, and sorry for the delay.
| while (uiLengthRead > 0) | ||
| { | ||
| auto& run = _FetchNextRun(uiLengthRead); | ||
| run.isTextSimple = isTextSimple; | ||
| } |
There was a problem hiding this comment.
nit: This might deserve a comment calling out that _FetchNextRun decrements uiLengthRead. I thought this was an infinite loop!
DHowett
added
the
AutoMerge
|
Hello @DHowett! Because this pull request has the 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 (
|
4 participants
This PR aims to optimize the text analysis process by breaking the text
into simple & complex runs according to the result of
GetTextComplexity. For simple runs, we can skip certain processingsteps to improve the analysis performance.
Previous to this PR, we rely on the result of
AnalyzeBidi,AnalyzeScriptandAnalyzeNumberSubstitutionto both break the textinto different runs and attach the corresponding
bidi/script/number_substitution information to the run. Thanks to #6695
we have the chance to skip the expensive analysis process when we found
the entire text is determined to be simple.
Inspired by microsoft/cascadia-code#411 and
discussions in #9156, I found that the "entire text simplicity" is often
hard to meet. In order to fully utilize the complexity information of
the text, we need to first break the text into simple & complex ranges.
These ranges are also the initial runs prior to the
bidi/script/number_substitution analysis. This way we can skip the text
analysis for simple runs to speed up the process.
VALIDATION
Build & run cmatrix, cacafire, cat big.txt with it.
Initial simple run PR: #6695
Closes #9156