-
Notifications
You must be signed in to change notification settings - Fork 165
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
テキスト幅計算に使用する文字間隔配列のコンテナを使いまわす事で負荷を削減 #1415
Conversation
テキスト幅計算を改良したいならば、テスト対象は文字がギッシリ詰まったファイルを使うのがよいと思います。 |
そのファイルを使用して確認を行いました。 変更前 470a1ff : 2046, 2037, 2031, 2031, 2047, 2034, 2046, 2047, |
✅ Build sakura 1.0.3119 completed (commit 94e7977aa3 by @beru) |
@@ -258,8 +257,8 @@ int CTextMetrics::CalcTextWidth2( | |||
|
|||
int CTextMetrics::CalcTextWidth3( | |||
const wchar_t* pText, //!< 文字列 | |||
int nLength //!< 文字列長 | |||
int nLength //!< 文字列長 |
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.
影響ないと思うのでスルーします。
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.
ついでにインデントの調整を行いました。
@@ -112,7 +112,8 @@ class CTextMetrics{ | |||
const wchar_t* pText, //!< 文字列 | |||
int nLength, //!< 文字列長 | |||
int nHankakuDx, //!< 半角文字の文字間隔 | |||
int nCharSpacing | |||
int nCharSpacing, //!< 文字の隙間 | |||
std::vector<int>& vDxArray //!< [out] 文字間隔配列 |
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.
100%好みに基づく変更要望ですが、std::vector<INT>
がいいです。
根拠は windows api の型定義で、DxArrayの型定義は本来 const INT*
だからです。
https://docs.microsoft.com/en-us/windows/win32/api/wingdi/nf-wingdi-exttextoutw
あとになってグダグダ言うのは嫌なのでいまのうちに指摘しときますが、元定義が vector<int>
やんけ!というのももっともだと思うのでこのままでもいいっす。
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.
確かにそうした方が好ましいですが将来的に問題になる事はおそらく無いと思います。
Microsoft Win64 のデータモデルが LLP64 なので。
レビューありがとうございました。Merge します。 |
PR の目的
処理の高速化です。
カテゴリ
PR の背景
CTextMetrics::CalcTextWidth2
メソッド内で文字間隔配列用にローカル変数でSTLコンテナを用意していますが、ローカル変数なのでスコープから抜けるとメモリ解放が行われます。頻繁に呼び出される処理内でメモリの確保と解放が行われるとそれ自体が負担になってしまうので、引数経由で渡す事でインスタンスを再利用するように変更しました。
PR のメリット
処理が少しだけ高速化します。
PR のデメリット (トレードオフとかあれば)
CTextMetrics::CalcTextWidth2
メソッドを呼び出す際に渡す必要のある引数が1つ増える事で使い勝手が悪くなります。ただし呼び出している箇所は3か所と多く無いので影響は少ないです。
テスト内容
#1411 で berryzplus さんが用意してくれた
flash_eol_test.js
マクロを使用しました。README.md
ファイルを開いてからウィンドウを最大化してマクロを複数回実行しました。単位は省略していますが ms(ミリ秒)です。
変更前 470a1ff : 5467, 5461, 5429, 5436, 5414, 5414, 5429,
変更後 c4a7fa6 : 5336, 5361, 5292, 5345, 5299, 5298, 5261,
1~2%程度とわずかな変化ですが有意差はあるんじゃないかと思います。CPUクロックのブーストによる影響が無いかとか心配になってしまいますが…。
なお #1405 で使われていた
redrawcounter.js
マクロだと殆ど差を確認出来ませんでした。処理内容の違いが原因なのか、それとも処理時間の数値が小さくて差が表れにくいのかは不明です。関連 issue, PR
#1405, #1411