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
ファイル読み込みの高速化 #1951
ファイル読み込みの高速化 #1951
Conversation
@@ -225,7 +225,7 @@ TEST(CTextMetrics, GenerateDxArray8) | |||
// IVSのVariantSelectorが続く文字列は先頭1文字 + 幅0×2で生成する | |||
std::vector<int> v; | |||
FakeCache1 cache; | |||
CTextMetrics::GenerateDxArray(&v, L"葛󠄀", 2, 0, 0, 0, 10, cache); | |||
CTextMetrics::GenerateDxArray(&v, L"葛󠄀", 3, 0, 0, 0, 10, cache); |
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.
長さ指定が間違っていそうなのでついでに修正します。(PR反映後のテスト実行で失敗したため判明)
✅ Build sakura 1.0.4343 completed (commit 8eb83c2ab7 by @suconbu) |
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.
このコンストラクタでは、入力されたポインタを起点に文字列末尾までの長さをカウントを内部的に行うようで、呼び出し回数が多い場合にそれが負荷になっているようです。
(例:1億文字を含むファイル読み込んだ場合、IsVariationSelector の呼び出し回数は 197,721,600 回)
IsVariationSelector の処理では2ワード分だけ参照できればよいので、対策としては IsVariationSelector の引数の型を現在の std::string_view から const wchar_t* (+長さ) に変更しようと思います。
問題に対して提示された対策の内容が飲み込めませんでした。定数時間になる (5) のコンストラクタを使用しないのはなぜでしょうか?
constexpr basic_string_view(const CharT* str, size_type len); // (5)
負荷の掛かっていた該当箇所 (CNativeW::GetSizeOfChar からの呼び出し) 以外にも、既存で IsVariationSelector を呼んでいるところも同様に不要な長さカウントがされないようにしようとしたのですが、そうした時に5か所ある既存の呼び出し箇所はすべて wchar_t* 型で渡すようになっていたことから、IsVariationSelector の引数をそれに合わせた、ということになります。 |
sakura_core/charset/codechecker.h
Outdated
inline bool IsVariationSelector(std::wstring_view text) { | ||
const auto cp = ConvertToUtf32(text); | ||
return 0xe0100 <= cp && cp <= 0xe01ef; | ||
inline bool IsVariationSelector(const wchar_t* pStr, size_t nLen) { |
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.
ヌル終端の wchar_t* 型で受け取るので長さ (nLen) をもらう必要なない気がしてきました。(直します)
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.
引数を増やすのは反対です。C言語スタイルのインターフェースが必要ならオーバーロード関数を定義すると良いと思います。
*/ | ||
inline bool IsVariationSelector(const wchar_t* pStr, size_t nLen = SIZE_MAX) { | ||
return (2 <= nLen) && (pStr[0] == 0xDB40) && (0xDD00 <= pStr[1]) && (pStr[1] <= 0xDDEF); | ||
} | ||
/*! | ||
* 文字列がIVSの異体字セレクタで始まっているか判定する | ||
*/ | ||
inline bool IsVariationSelector(std::wstring_view text) { |
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.
今後、std::wstring_view 型で文字列を扱う処理からも使うことも考えて std::wstring_view 型を受け入れる版も引き続き利用できるようにします。
sakura_core/mem/CNativeW.cpp
Outdated
@@ -397,7 +397,7 @@ CLogicInt CNativeW::GetSizeOfChar( const wchar_t* pData, int nDataLen, int nIdx | |||
} | |||
|
|||
// IVSの異体字セレクタチェック | |||
if (IsVariationSelector(pData + nIdx + 1)) { | |||
if (IsVariationSelector(pData + nIdx + 1, nDataLen - (nIdx + 1))) { |
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.
if (IsVariationSelector(pData + nIdx + 1, nDataLen - (nIdx + 1))) { | |
if (IsVariationSelector(std::wstring_view(pData + nIdx + 1, nDataLen - (nIdx + 1)))) { |
としたら遅延が発生しないということですね。
実質 #1937 の不具合修正ですね。
sakura_core/charset/codechecker.h
Outdated
inline bool IsVariationSelector(std::wstring_view text) { | ||
const auto cp = ConvertToUtf32(text); | ||
return 0xe0100 <= cp && cp <= 0xe01ef; | ||
inline bool IsVariationSelector(const wchar_t* pStr, size_t nLen) { |
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.
引数を増やすのは反対です。C言語スタイルのインターフェースが必要ならオーバーロード関数を定義すると良いと思います。
sakura_core/charset/CCodeBase.cpp
Outdated
@@ -55,7 +55,7 @@ std::wstring CCodeBase::CodeToHex(const CNativeW& cSrc, const CommonSetting_Stat | |||
EConvertResult CCodeBase::UnicodeToHex(const wchar_t* cSrc, const int iSLen, WCHAR* pDst, const CommonSetting_Statusbar* psStatusbar) | |||
{ | |||
// IVS | |||
if (iSLen >= 3 && IsVariationSelector(cSrc + 1)) { | |||
if (iSLen >= 3 && IsVariationSelector(cSrc + 1, iSLen - 1)) { |
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.
発生している問題(不具合?)と関係ない変更に見えます。
sakura_core/charset/CUtf8.cpp
Outdated
@@ -227,7 +227,7 @@ EConvertResult CUtf8::_UnicodeToHex(const wchar_t* cSrc, const int iSLen, WCHAR* | |||
if (IsUTF16High(cSrc[0]) && iSLen >= 2 && IsUTF16Low(cSrc[1])) { | |||
cBuff._GetMemory()->SetRawDataHoldBuffer(cSrc, 4); | |||
} | |||
else if (iSLen >= 3 && IsVariationSelector(cSrc + 1)) { | |||
else if (iSLen >= 3 && IsVariationSelector(cSrc + 1, iSLen - 1)) { |
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.
発生している問題(不具合?)と関係ない変更に見えます。
_UnicodeToHexはキャレット位置の文字コードをステータスバーに表示するためのフォーマット関数で、ファイル読み込みで繰り返し呼び出されるものではありません。
残す場合、文字列長は3に決め打ちしてよいはず。
sakura_core/parse/CWordParse.cpp
Outdated
@@ -187,7 +187,7 @@ ECharKind CWordParse::WhatKindOfChar( | |||
} | |||
// IVS(正字 + 異体字セレクタ) | |||
else if (nCharChars == 3 && | |||
IsVariationSelector(pData + nIdx + 1)) | |||
IsVariationSelector(pData + nIdx + 1, pDataLen - (nIdx + 1))) |
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.
発生している問題(不具合?)とは無関係に見えます。
が、IVSを大量に含むテキストを開いたときの描画速度に影響する可能性があります。
「せっかくなので合わせて修正」でも「無関係なので捨て」でもどちらでもよいです。
sakura_core/charset/codechecker.h
Outdated
* 文字列がIVSの異体字セレクタで始まっているか判定する | ||
* nLenを省略した時はヌル文字の手前までが有効な文字列であると解釈する | ||
*/ | ||
inline bool IsVariationSelector(const wchar_t* pStr, size_t nLen = SIZE_MAX) { |
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.
inline bool IsVariationSelector(const wchar_t* pStr, size_t nLen = SIZE_MAX) { | |
inline bool IsVariationSelector(const wchar_t* pStr, size_t nLen) { |
これは許容したくありません。(個人の感想です。)
sakura_core/charset/codechecker.h
Outdated
* nLenを省略した時はヌル文字の手前までが有効な文字列であると解釈する | ||
*/ | ||
inline bool IsVariationSelector(const wchar_t* pStr, size_t nLen = SIZE_MAX) { | ||
return (2 <= nLen) && (pStr[0] == 0xDB40) && (0xDD00 <= pStr[1]) && (pStr[1] <= 0xDDEF); |
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.
return (2 <= nLen) && (pStr[0] == 0xDB40) && (0xDD00 <= pStr[1]) && (pStr[1] <= 0xDDEF); | |
return IsVariationSelector(std::wstring_view(pStr, nLen)); |
こうしておくとpStr,nLenに不正値が入っていた場合(=プログラムのバグ)を検出できます。
✅ Build sakura 1.0.4344 completed (commit b4462a5a14 by @suconbu) |
頂いたコメントを参考に、IsVariationSelector の引数/呼び出しについては以下の内容で対応したいと思います。
上記反映後で計測すると当初の修正方法よりも少しだけ処理時間が増加していますが、大勢には影響なさそうです。
|
e9d9d51
to
8df88f1
Compare
Quality Gate passedIssues Measures |
✅ Build sakura 1.0.4345 completed (commit 0a84f52504 by @suconbu) |
@@ -392,8 +392,7 @@ char32_t ConvertToUtf32(std::wstring_view text) { | |||
* 文字列がIVSの異体字セレクタで始まっているか判定する | |||
*/ | |||
inline bool IsVariationSelector(std::wstring_view text) { | |||
const auto cp = ConvertToUtf32(text); | |||
return 0xe0100 <= cp && cp <= 0xe01ef; | |||
return (2 <= text.size()) && (text[0] == 0xDB40) && (0xDD00 <= text[1]) && (text[1] <= 0xDDEF); |
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.
この変更は速度に影響ありますか?
「0xe0100~0xe01ef」が見えなくなることを気にしています。
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.
あります。ファイル読み込み処理全体 (DocFileOperation::FileLoad) の約10%はこの ConvertToUtf32 が消費していました。
レビューありがとうございました。マージします。 |
PR対象
カテゴリ
PR の背景
概要
サイズの大きいファイルを開く時のもたつきが気になるため改善を行います。
調査には VisualStudio 2022 に付属するパフォーマンスプロファイラーの フレームグラフ 機能を使ってみました。
調べた結果を元に、本PRでは以下の2点の改善をします。
詳細
以下はファイル読み込み時のフレームグラフです。今回は比較的割合の大きい「1」「2」の箇所に注目しました。
※Debugビルドにて調査、調査時は
USE_STRICT_INT
の定義を無効化1 について
CNativeW::GetSizeOfChar から IsVariationSelector を呼び出す際、std::wstring_view の (3) ※のコンストラクタが呼び出されています。
※https://cpprefjp.github.io/reference/string_view/basic_string_view/op_constructor.html
このコンストラクタでは、入力されたポインタを起点に文字列末尾までの長さをカウントを内部的に行うようで、呼び出し回数が多い場合にそれが負荷になっているようです。
(例:1億文字を含むファイル読み込んだ場合、IsVariationSelector の呼び出し回数は 197,721,600 回)
対策としては IsVariationSelector の呼び出し時に、計算量が定数時間となる (5) のコンストラクタを明示的に使うようにします。
また、CNativeW::GetSizeOfChar と同様に比較的長い文字列を IsVariationSelector に渡す可能性のある CWordParse::WhatKindOfChar, CTextMetrics::GenerateDxArray についても同様の修正を行うことにします。
2 について
現在の IsVariationSelector の実装では異体字セレクタ判定のために一時的に UTF-32 へ変換を行っていますが、呼び出し回数が多い場合にはここが負荷となっているようです。
対策としては UTF-16 のまま直接異体字セレクタの判定するようにして UTF-32 への変換を省略します。
効果
※単位:ミリ秒
※計測に使用したファイル:100m_chars_utf8.zip を展開
※Releaseビルドにて計測、時間計測には CRunningTimer を使用、3回計測した結果の平均
PR適用後のファイル読み込み時のフレームグラフです。IsVariationSelector が占める割合が小さくなりました。
仕様・動作説明
ユーザーに対する機能的なふるまいの変化はありません。
PR の影響範囲
特にありません。
テスト内容
関連 issue, PR
参考資料