grep 処理速度改善 #77

beru · 2018-06-09T18:18:42Z

共通設定の検索タブの Grep リアルタイムで表示する のチェックが付いていると、Grep処理中に画面の更新処理を頻繁に行う為処理に時間が掛かっているので対策を行いました。

berryzplus

はじめまして。PRありがとうございます。
修正内容を拝見したところ、かなりの速度改善を見込める修正と判断しました。
個人的には是非取り込みたいと思ってますが、
このコミットについてもう少し詳しく教えていただけませんか・

以下の3点について追加情報をいただけるとありがたいです。

何をどう改善したかについて
　PR本文の説明から画面更新の頻度を落とす修正だと思って見てます。
　理解の祖語があるといけないので、もう少し詳しく説明していただけませんでしょうか？
DoGrepFileメソッドへの引数追加理由について
　元々引数が多いメソッドです。
　サクラエディタをより良くしていくために引数はむしろ削りたいです。
　引数を追加せざるを得ない理由があれば、ご教授いただけませんでしょうか？
更新間隔 200ticks の設定根拠について
　個人的には「なんとなく」でも構わないと思っています。
　根拠のある値なのか、感覚で決めた値なのかで変わるものがあるとおもいます。
　あとで見たときに分からなくなりそうなので、根拠があれば教えていただけませんか？

このレビューコメントは一度書き直ししています。
GitHubに不慣れなものでレビューコメントを書きなおししました。
初回に書いた「GetTickCout 関数を使用しているのは問題」については取り下げさせていただきます。
@kobakeさんが指摘されているとおり、@beru さんの利用方法では問題がおきないためです。

ぜひ取り込みしたいと考えておりますので、ご対応のほどよろしくお願いします。

kobake · 2018-06-10T04:19:55Z

PR ありがとうございます！

::GetTickCount関数を使用している。※注１

これについては差分計算での利用であれば問題ないと考えています。本 PR での利用方法も差分計算による利用であったので問題なさそうです。

GetTickCount64 function

これを使うとすると Win8 以降に動作環境を制限することになります。動作環境のサポート範囲については要検討ですね。本 PR とは別件としてどこかでお話しましょう。

beru · 2018-06-10T08:52:52Z

@berryzplus さんレビューありがとうございます。

何をどう改善したかについて
　PR本文の説明から画面更新の頻度を落とす修正だと思って見てます。
　理解の祖語があるといけないので、もう少し詳しく説明していただけませんでしょうか？

結果出力のタイミング調整
- 画面更新が頻繁に行われるとGrep処理に時間が掛かる為、一定の間隔を空けるようにしました。m_dwTickAddTail メンバ変数を追加し、Grep処理中に前回の AddTail() 呼び出しから一定以上の時間が経過していなければ AddTail() を呼び出さないようにしました。
- Grep結果文字列バッファ変数 cmemMessage を使いまわしてメモリの確保と解放が行われる頻度を下げるようにしました。cmemMessage は以前は DoGrepTree() のローカル変数で、フォルダ移動前に残りを先に出力する処理が入っていましたがそこらへんは削りました。
UI確認の頻度を落とす
- m_dwTickUICheck メンバ変数を追加し、処理中のユーザー操作の確認処理の実行に一定時間の間隔を取るようにしました。

DoGrepFileメソッドへの引数追加理由について
　元々引数が多いメソッドです。
　サクラエディタをより良くしていくために引数はむしろ削りたいです。
　引数を追加せざるを得ない理由があれば、ご教授いただけませんでしょうか？

Grep結果文字列バッファを使いまわしてメモリの確保と解放が行われる頻度を下げる為にそうしました。

更新間隔 200ticks の設定根拠について
　個人的には「なんとなく」でも構わないと思っています。
　根拠のある値なのか、感覚で決めた値なのかで変わるものがあるとおもいます。
　あとで見たときに分からなくなりそうなので、根拠があれば教えていただけませんか？

200ミリ秒というのは感覚で決めた値で明確な根拠はありません。なお後続のコミットでは50ミリ秒に変えました。モニターのリフレッシュレートと同じぐらいにはしたいですが、そうすると体感できるくらいにGrep処理が遅くなってしまいます。

感覚的なものなので出来ればユーザー側で調整出来るようにした方が良いかもしれません。

berryzplus

ご対応ありがとうございます。
LGTMです。

Grep結果文字列バッファを使いまわしてメモリの確保と解放が行われる頻度を下げる為にそうしました。

これは仕方ない種類の理由だとおもいます。
DoGrepFileの引数が多い件については別途で対応することにします。

kobake · 2018-06-10T09:57:56Z

インターバル感覚に m_dwTickInterval という名前が付いたことにより文意はつかみやすくなったと思います。

感覚的なものなので出来ればユーザー側で調整出来るようにした方が良いかもしれません。

たぶんここをチューニングできるようなインターフェースを提供しても、いじる人ほとんどいない気がします。極力調整可能なパラメータは少なく絞っておいたほうが煩雑にならなくて良いかと。

プログラム的にもインターバルのためだけにインスタンスの状態を増やすのは煩雑なので、 m_dwTickInterval はメンバ変数よりも const 定数であったほうが好ましいと思いました (定数にする場合はそれが分かるように命名も変える)。インターバル間隔は今の 50ms の決め打ちで良いと思います。

kobake · 2018-06-10T10:07:02Z

sakura_core/mem/CMemory.cpp

@@ -312,6 +312,8 @@ void CMemory::AllocBuffer( int nNewDataLen )
 	}else{
 		/* ���݂̃o�b�t�@�T�C�Y���傫���Ȃ���ꍇ�̂ݍĊm�ۂ��� */
 		if( m_nDataBufSize < nWorkLen ){
+			// �p�ɂȍĊm�ۂ�s��Ȃ��悤�ɂ���ׁA�K�v�ʂ̔{�̃T�C�Y��m�ۂ���
+			nWorkLen <<= 1;


これは何かしら実際に計測してみた結果からの変更判断でしょうか。

変数値を2倍にする操作については <<= 1 よりも *= 2 のほうが文意がつかみやすくて好ましいです。昔は性能をカバーする意図でこういう書き方をした時代もありましたが、今はコンパイラが自動的に最も効率の良いバイナリコードに最適化してくれるはずなので、人間にとっての読みやすさを優先するのが良いと思います。

測定結果は残っていませんが過去に別ブランチでGrepの最適化を行っていた際にメモリ確保のコストがかなり重かったです。特に今回のような同じバッファに追記していくケースだと元の記述だとかなりの頻度でメモリの解放と確保が行われてしまうかと思います。

変数値を2倍にする操作はビットシフトではなく乗算を使うように記述を更新しました。念のためにオーバーフローしないように判定も追加しました。そもそも極端に大きいバッファサイズが指定されると色々なところで問題が出るはずなのでこれだけでは意味がないかもしれませんが。

あと本来は一度に大きいバッファを扱うべきでは無いのでデータ構造を変えた方が良いですが（例：VS Code）それはまた別の話なので話題を広げないでおきます。

kobake · 2018-06-10T10:14:17Z

sakura_core/CGrepAgent.cpp

@@ -524,7 +529,7 @@ DWORD CGrepAgent::DoGrep(
 	if( 0 < nWork && sGrepOption.bGrepHeader ){
 		AddTail( pcViewDst, cmemMessage, sGrepOption.bGrepStdout );
 	}
-	cmemMessage.Clear(); // ������Ȃ�
+	cmemMessage._SetStringLength(0);


Clear と _SetStringLength(0) って何が違うんでしたっけ？（昔触ったことありますが忘れちゃいました…）

Clear()はメモリ解放あり（解放→再確保のコストがかかる）
_SetStringLength(0)はメモリ解放なし（解放しないので再確保もしない）
という認識です。

なるほど、ご説明ありがとうございます

@berryzplus 代わりに説明していただいてありがとうございます。コメントで書こうか迷ったのですが、いちいち書いているとコメントばかりになってしまう気がして書きませんでした。

気になって調べてみたのですが、標準ライブラリの std::vector::clear() の場合はメモリ解放はしないみたいで、C++11 から追加された std::vector::shrink_to_fit() で無駄な領域の切り詰めを行うみたいですね。昔は空のインスタンスとswapするのがstd::vectorのインスタンスのメモリ解放の常套手段だったみたいですが…。

kobake · 2018-06-10T10:44:01Z

sakura_core/CGrepAgent.cpp

+		if( 0 == nLine % 32 ) {
+			DWORD dwNow = ::GetTickCount();
+			if ( dwNow - m_dwTickUICheck > m_dwTickInterval ) {
+				m_dwTickUICheck = dwNow;


ざっくり眺めた感じ、全体的に処理の割り込みタイミングを
「処理行数によるタイミングではなく」
「経過時間によるタイミングで」判定する方向性のように見えました。

が、この箇所だけ「処理行数によるタイミング判定」と「経過時間によるタイミング判定」の組み合わせになっているようですが、これは意図的なものですか？

1行のサイズが小さくてたくさん行数があるようなファイルの場合、毎行読み取り毎に時間チェックするのは無駄かもしれないので（とはいっても判定コストは微々たるものだと思いますが）残しておいても良いかもと思います。

1行のサイズがとてつもなく長くて（>100MBとか…）行数が少ないファイルの場合はこの判定の場合は不適切かもしれませんが、そんなファイルをサクラエディタで扱ってもしょうがない気もしますね。

UI確認・結果出力の時間間隔をメンバー変数から定数に変更 CMemory::AllocBuffer() における確保バッファサイズの調整記述を更新

kobake · 2018-06-11T03:04:36Z

sakura_core/mem/CMemory.cpp

+			// �p�ɂȍĊm�ۂ�s��Ȃ��悤�ɂ���ׁA�K�v�ʂ̔{�̃T�C�Y��m�ۂ���
+			if (nWorkLen < std::numeric_limits<int>::max() / 2) {
+				nWorkLen *= 2;
+			}


ご対応ありがとうございます。

他部分については問題ないと思うのですが、ここの影響が割と広範囲で、懸念ではあります。他の方の意見も伺いたいです。

お久しぶりです。ROMるのでいっぱいいっぱいの ds14050 です。少しだけ失礼します。

CMemory は std::vector に相当するものかなと最初は思ったのですが、より低レベルの allocator に当たる役割をアプリケーションの広範囲に提供しているように見受けられました。その根っこの部分に「２倍確保」などの判断を入れると道具としての使い勝手が悪くなり、下手をすると「AllocBuffer((exactSizeDesired+1)/2);」のような使い方を誘発する懸念があります。メモリについてまじめに考えコントロールしたい人ほどそうしかねません。

CMemory::AllocBuffer を呼ぶ場所を調べる一環で CNativeW::AllocStringBuffer の呼び出し箇所を調べましたが、当然ながら要求するサイズには根拠があります。明確な根拠なく CMemory がそこに手心を加えるのは乱暴です。

AllocBuffer や Set* 系の関数には命じられた仕事を粛々とこなしてもらい、Append 系の便利関数で２倍確保策を講じるのが落し所ではないでしょうか。

@ds14050 さん、お久しぶりです。

AllocBuffer や Set* 系の関数には命じられた仕事を粛々とこなしてもらい、Append 系の便利関数で２倍確保策を講じるのが落し所ではないでしょうか。

分かったような、分からないようなんですが、Append系のメモリ確保サイズを倍にしたらいい、という提案と理解してよいでしょうか？
つまりNativeW::AppendStringとかを修正しましょう、という提案？

そう言われればもっともな気がします。

呼出側コードで要求サイズを2倍にする対応はできないんでしたっけ？
できれば今回のGrepでボトルネックになりうる箇所だけをピンポイントで変えたいです。

ご検討いただけませんか？ｍ（。　。）ｍ　＞　@beru さん

つまりNativeW::AppendStringとかを修正しましょう、という提案？

そうですが、CMemory の _AppendSz と AppendRawData でやってもいいと思います。

レビューありがとうございます。頂いたコメントを元に手を加えました。再度確認お願いします。

頂いたコメントを元に手を加えました。再度確認お願いします。

すみません。ぼくが話についていけてないかも知れません。

読み返してみると、ぼく誤解させてそうなコメントしてますね・・・。

呼出側コードで要求サイズを2倍にする対応はできないんでしたっけ？
できれば今回のGrepでボトルネックになりうる箇所だけをピンポイントで変えたいです。

個人的には後半に言ってることのがポイント高めでした。
CMemoryやCNativeWを変更すると影響範囲が読めなくなるので、できれば触りたくないです。

「ボトルネックになりそうな箇所」をピンポイントで狙えないなら、
ループに入る前にドカンと4KB(約2000文字)確保して拡張の可能性を減らすとか、
少し消極的にはなるんですが、影響範囲を抑えたうえで最大限の効果を狙える手法もとれるのかな、と考えています。

size_t/ptrdiff_tの話を出しましたが、あれは気にしないでください。

ここのスコープに限定すると、オーバーフローについては深く考えなくて良いと思っています。
CNativeWは主に1行を表すバッファとして使われるものなので、
単体で1MB(100万文字)を超えるのはレアケースないし誤用(バイナリファイル読込など)だと思っています。
CMemory/CNativeについては別の機会にあり方を考えていきたいです。

すみません、そういうわけでCMemoryに手を加えない形でもう一度ご検討いただけませんでしょうか？

個人的にはそういう根拠のない不安には「やっちゃえよ」と反発するのですが、水掛け論になるので主張はしないことにしています。std::vector で普通にある挙動なので、やりすぎだとも予想外の結果になるとも思わないのですが。

@beru さんを迷わせてしまいましたね。承認する準備はできていますが円満迅速な結果を求めるなら berryzplus さんの指摘に従うのが手です。

CMemory の _AppendSz と AppendRawData でメモリ再確保サイズを倍々にして再確保頻度を減らす対応

ds14050

ゆくゆくは size_t/ptrdiff_t で扱うとして、符号の有無でオーバーフローの定義/未定義が変わるようです。_ReallocIfNeeded は一部考慮していますが現実問題としてオーバーフローを無視してコードをシンプルにする選択もあります。現在のコードは無視しています。

_ReallocIfNeeded が考慮していないのは newSize の上限を std::numeric_limits<int>::max() として切り詰めた場合に AllocBuffer で起こるオーバーフローです。典型的には負のサイズを確保しようとして確保しない＆(AllocBufferが)バッファを無効化もしないので、つづく _AddData が破滅への一歩です。これを AllocBuffer の可能な上限 (std::numeric_limits<int>::max()&~7) - 7 - 2 に切り詰めるとしても、newSize1 を切り詰めた場合には AllocBuffer の成否にかかわらずやはり破滅です。危険な _AddData 呼び出しは各所にあるのでここでは問題にしませんが、問題を解決しない対処を埋め込んでも益がありません。また int より大きい型として int64_t を採用する対処法は、size_t/ptrdiff_t の採用や x64 コンパイルによって近い将来に有効ではなくなりそうです。int64_t を使わずに他と同じ型(現在は int)で書けませんか？ GREP の改善が目的ですから CMemory の問題には深入りせず割り切っていいと思います。具体的には

m_nRawLen + appendLength + 2 はオーバーフローするに任せる。(どうせ _AddData が……)
余分に確保するサイズは AllocBuffer をオーバーフローさせない値まで切り詰める。

berryzplus さんの意味が変わっているという指摘は確保サイズに関することでしょうか。連結データサイズの２倍だったものがバッファサイズの２倍になっていると。確保済みバッファのサイズと連結するデータのサイズの大小によって効果が減ってしまうと。連結したデータサイズの２倍(というか求めやすい値)でいいと思います。

３点目。newSize1 に２を加える必要はないと思います。ターミネータのために AllocBuffer が確保する２ですよね？

berryzplus · 2018-06-12T12:42:03Z

sakura_core/mem/CMemory.cpp

+			// �p�ɂȍĊm�ۂ�s��Ȃ��悤�ɂ���ׁA�K�v�ʂ̔{�̃T�C�Y��m�ۂ���
+			if (nWorkLen < std::numeric_limits<int>::max() / 2) {
+				nWorkLen *= 2;
+			}


頂いたコメントを元に手を加えました。再度確認お願いします。

すみません。ぼくが話についていけてないかも知れません。

読み返してみると、ぼく誤解させてそうなコメントしてますね・・・。

呼出側コードで要求サイズを2倍にする対応はできないんでしたっけ？
できれば今回のGrepでボトルネックになりうる箇所だけをピンポイントで変えたいです。

個人的には後半に言ってることのがポイント高めでした。
CMemoryやCNativeWを変更すると影響範囲が読めなくなるので、できれば触りたくないです。

「ボトルネックになりそうな箇所」をピンポイントで狙えないなら、
ループに入る前にドカンと4KB(約2000文字)確保して拡張の可能性を減らすとか、
少し消極的にはなるんですが、影響範囲を抑えたうえで最大限の効果を狙える手法もとれるのかな、と考えています。

size_t/ptrdiff_tの話を出しましたが、あれは気にしないでください。

ここのスコープに限定すると、オーバーフローについては深く考えなくて良いと思っています。
CNativeWは主に1行を表すバッファとして使われるものなので、
単体で1MB(100万文字)を超えるのはレアケースないし誤用(バイナリファイル読込など)だと思っています。
CMemory/CNativeについては別の機会にあり方を考えていきたいです。

すみません、そういうわけでCMemoryに手を加えない形でもう一度ご検討いただけませんでしょうか？

CMemory は変更しないように元に戻す CGrepAgent の変更コードのインデントスタイルを周りに合わせる

beru · 2018-06-12T15:32:18Z

@berryzplus @ds14050 コメントありがとうございます。CMemory 側には変更を加えないようにしました。

beru · 2018-06-12T16:10:36Z

@ds14050

ゆくゆくは size_t/ptrdiff_t で扱うとして、符号の有無でオーバーフローの定義/未定義が変わるようで> す。_ReallocIfNeeded は一部考慮していますが現実問題としてオーバーフローを無視してコードをシ> ンプルにする選択もあります。現在のコードは無視しています。

ご指摘通りケアを完全に出来ないなら、他に合わせて無視する方が記述がシンプルになって良いですね。

_ReallocIfNeeded が考慮していないのは newSize の上限を std::numeric_limits::max() として切り> 詰めた場合に AllocBuffer で起こるオーバーフローです。典型的には負のサイズを確保しようとして確保しない＆(AllocBufferが)バッファを無効化もしないので、つづく _AddData が破滅への一歩です。これを AllocBuffer の可能な上限 (std::numeric_limits::max()&~7) - 7 - 2 に切り詰めるとしても、newSize1 を切り詰めた場合には AllocBuffer の成否にかかわらずやはり破滅です。危険な _AddData 呼び出しは各所にあるのでここでは問題にしませんが、問題を解決しない対処を埋め込んでも益がありません。

おっしゃるとおり AllocBuffer の実装を全然考慮していませんでした。最終的に問題を解決出来ないなら手前側で不完全な対策を入れてもしょうがないですね。

また int より大きい型として int64_t を採用する対処法は、size_t/ptrdiff_t の採用や x64 コンパイルによって近い将来に有効ではなくなりそうです。int64_t を使わずに他と同じ型(現在は int)で書けませんか？ GREP の改善が目的ですから CMemory の問題には深入りせず割り切っていいと思います。具体的にはm_nRawLen + appendLength + 2 はオーバーフローするに任せる。(どうせ _AddData が……)
余分に確保するサイズは AllocBuffer をオーバーフローさせない値まで切り詰める。

int64_t を使わないで書く場合の分岐を考えるのが大変で手抜きしました。
VC++ は LLP64 なので x64 でも int は 4バイトなので問題無いかなと思いました。

将来的に int より大きい型として int64_t を採用する場合は全体的に大幅に見直しが必要なのでその場合は自分が入れたコード CMemory::_ReallocIfNeeded も見直されるだろういう考えで。

berryzplus さんの意味が変わっているという指摘は確保サイズに関することでしょうか。連結データサイズの２倍だったものがバッファサイズの２倍になっていると。確保済みバッファのサイズと連結するデータのサイズの大小によって効果が減ってしまうと。連結したデータサイズの２倍(というか求めやすい値)でいいと思います。

どういう記述にすればシンプルな倍々再確保処理になるのか悩んで結局手抜きして int64_t を使いましたが、32bit整数演算だけで分岐使わないで書けたら良いですね。ビット演算だらけで読みにくくなりそうですが…。

連結データサイズ > 確保済みバッファサイズ の時に再確保が必要ですが、

連結データサイズ > 確保済みバッファサイズ * 2 というケースも有りうるので、

std::min(std::max((int64_t)連結データサイズ, (int64_t)確保済みバッファサイズ * 2), (int64_t)std::numeric_limits<int>::max())

等が良いのでしょうか…。あっ、int64_t は使わない縛りがあるんでしたね。

３点目。newSize1 に２を加える必要はないと思います。ターミネータのために AllocBuffer が確保する２ですよね？

それもそうですね。AllocBuffer の内部で終端2byte分を足すのと8byteにアライメントしているので呼び出し側ではやる必要が無かったですね。

ds14050 · 2018-06-12T19:06:49Z

連結データサイズ > 確保済みバッファサイズの時に再確保が必要ですが、連結データサイズ > 確保済みバッファサイズ * 2 というケースも有りうるので、std::min(std::max((int64_t)連結データサイズ, (int64_t)確保済みバッファサイズ * 2), (int64_t)std::numeric_limits<int>::max()) 等が良いのでしょうか…。

beru さんのもともとのコミットでは AllocBuffer への要求サイズ(nWorkLen = 連結データサイズ)の２倍を確保するものだったではありませんか。バッファサイズの２倍は考えずに連結データサイズを２倍にするのが「求めやすい値」です。

済んだことなので、的外れなコメントをしないために自分が考えたものも書いておきます。AllocBuffer の呼び出しをしないことを除けば _ReallocIfNeeded と処理内容はほぼ同じです(そのはずっ)。

/* CMemory に appendSize バイトのデータを連結するとき、AllocBuffer に要求するべきサイズ。 */
static inline int AllocSize(int bufferSize, int rawSize, int appendSize)
{
	const int AllocBufferLimit = (std::numeric_limits<int>::max()&~7) - 7 - 2; // AllocBuffer の内部でオーバーフローさせない引数の上限値。
	const int jointSize = rawSize + appendSize;
	if (jointSize + 2 + 7 <= rawSize + 2 + 7) { // appendLength が負、または和がオーバーフロー、または AllocBuffer の内部でオーバーフローしそう。符号付き整数のオーバーフローは未定義らしいから、appendSize が負であることしか確かめられていないかもしれない。その場合でもオーバーフローの予行の意味があるが、最適化によって取り除かれる可能性もありそう。
		return rawSize;
	}
	if (jointSize + 2 < bufferSize) {
		return rawSize; // 拡張の必要なし。
	}
	// Append が連続することを見越してサイズに余裕を持たせる。
	return jointSize + std::min(jointSize, AllocBufferLimit - jointSize);
}

// 呼び出し側(抜粋)
AllocBuffer(AllocSize(m_nDataBufSize, m_nRawLen, nDataLenBytes));
if (nDataLenBytes <= m_nDataBufSize - m_nRawLen - 2) {
	_AddData( pData, nDataLenBytes );
}

beru · 2018-06-12T19:59:43Z

@ds14050

beru さんのもともとのコミットでは AllocBuffer への要求サイズ(nWorkLen = 連結データサイズ)の２倍を確保するものだったではありませんか。バッファサイズの２倍は考えずに連結データサイズを２倍にするのが「求めやすい値」です。

それはそうなのですが倍々にしていく方がバッファの増え方としては綺麗な気がしてそう書きました。
バッファサイズも2のべき乗の方が何となく綺麗ですよね。でもそれなら連結データサイズ以上の最小の 2 のべき乗を求める処理にしてすっきりと分岐無しで書けそうですね。

AllocSize() のコードも提示ありがとうございます。AllocBuffer() 内部のケアがあるので何か色々と入っていますね…。ところで CNativeW : CNative : CMemory という継承構造になっていて std::vector 型のメンバーを持つ方式じゃないのは歴史的経緯とかでしょうか。。

kobake · 2018-06-13T02:03:49Z

AllocSize() のコードも提示ありがとうございます。AllocBuffer() 内部のケアがあるので何か色々と入っていますね…。ところで CNativeW : CNative : CMemory という継承構造になっていて std::vector 型のメンバーを持つ方式じゃないのは歴史的経緯とかでしょうか。。

歴史的経緯ですね。これは後々見直しても良いと思います。

kobake · 2018-06-13T02:07:28Z

10年くらい前に CNativeW は自分の手で作られました。

構造としては既存のメモリ確保挙動を壊さないように（挙動変えたい気持ちもありましたが、ここで余計な反発喰らって Unicode 対応の足並みが遅くなるのは嫌だったので挙動はそのままにした）、かつ、既存コードが以前よりは（Unicode 対応前よりは）読みやすくなるように苦肉の策でこうなってる感じです。

ds14050 · 2018-06-13T10:35:31Z

そういう意図を語れる人がいるのはいいですね。でないとどうしても「現状が正義」になりがちなので。

kobake · 2018-06-13T14:53:51Z

確認ですが、メモリ確保のロジックを変更しないとこの PR による速度改善は望めない感じでしょうか？メモリ確保部分変更しなくても十分高速化されるのであれば一旦メモリ確保部分は取り除いてレビューに回してしまったほうがレビューは通りやすいと思います。

Grep速度のボトルネックの多くがメモリ確保部分にあるとするならば、自分がやるとしたらメモリ確保をStrategyパターン的に切り出してGrep時にだけ別のアロケータ使うような実装にすると思います。

もちろん将来的にはサクラエディタ全体に関わるメモリ確保ロジックの改善が必要な雰囲気は伝わったのですが、簡単に判断できる問題ではないのでGrep改善と一緒に話を進めようとすると難航しそうな気がします。

beru · 2018-06-13T15:48:35Z

@kobake 時間を計ってみました。

確認版	リアルタイムで表示するON	リアルタイムで表示するOFF
何も対策無し	4850	1005
当PR版、CMemory触らない	2100	930
当PR版、CMemory倍々再確保	2200	930
別で色々手を加えた版	600	430
秀丸	一瞬

処理時間の単位はミリ秒で、サクラエディタのリポジトリ以下を hWnd でフィルタが *.txt *.h *.cpp *.c *.cc *.hpp で サブフォルダからも検索する にチェックを付けて検索を掛けた際の時間です。

CMemoryでメモリの倍々再確保をしたらGrep検索の速度が速くなると思ったのは私の勘違いでした。すみません。

秀丸はマルチスレッドも使って高速化したみたいですね。あとRust言語で書かれている ripgrep とかも高速みたいです。

beru · 2018-06-13T15:56:55Z

@kobake このPRでは CGrepAgent だけの変更で良いと思います。ちゃんと効果の確認もせずに CMemory に手を入れてPRを出してレビューワーを混乱させてしまった自分が悪かったです。

kobake · 2018-06-13T16:01:42Z

いえいえ、いろいろ検討するキッカケになって良かったです。
実測値があると分かりやすいですね。

ちなみにちゃんと調べたわけじゃないので言うの迷ってましたが、おそらく realloc がそれなりに効率の良いメモリ再確保の仕方を内部的にしてくれてると思ってます（単に愚直に引数ぴったり分の空きメモリを確保しているわけではないはず）。

明日以降でまた時間のあるときに再レビューしてみますね。
（または自分以外の誰かが先にレビューしてくれる気もする）

kobake · 2018-06-14T15:49:17Z

細かい指摘で恐縮ですが、コミットメッセージがぜんぶ同じ文言なので、もうこれはいっそのこと squash 等で1コミットにまとめてしまうのが良いかと思いました（それぞれのコミットメッセージが何かしら意味を持つものであればコミットが分かれている価値はあります）。

それはそれとして squash したとしても内容に差異は生じないはずですので、レビューはしておきます。

kobake · 2018-06-14T15:55:17Z

コミットメッセージがぜんぶ同じ文言なので

と思いましたがコミットメッセージの詳細部分に細かい内容書いてあったのですね。
理想を言うとコミットメッセージのサマリー部にもざっくり概要書いてあると見やすいです。
が、大した問題ではないので今回はそのままレビュー進めます。
rebase するかどうかは「お任せ」します。

kobake

LGTM

kobake · 2018-06-14T16:35:23Z

以下理由により LGTM と判断しました。

動作を確認した限り問題無さそう
処理速度も上がっている
致命的に問題のあるコードは見当たらない
Grep以外への影響もない

@berryzplus さんの request change が入っているので現時点ではマージがブロックされている状態です。
再度内容のレビューをお願い致します。

個人的にはこの PR での変更以前の問題として、CGrepAgent のコードがあまりにも肥大化しているため、どこかのタイミングでがっつりリファクタリングしたい気持ちが強いです（これは10年前の時点でも既に気になっていましたが、既存挙動を極力変更しないためにスルーしてました）。

berryzplus

LGTMです。

対応ありがとうございました。

grep 処理速度改善マージします。

grep 処理速度改善

e594f61

berryzplus requested changes Jun 10, 2018

View reviewed changes

grep 処理速度改善

cd5834b

berryzplus previously approved these changes Jun 10, 2018

View reviewed changes

kobake reviewed Jun 10, 2018

View reviewed changes

grep 処理速度改善

4fa304e

UI確認・結果出力の時間間隔をメンバー変数から定数に変更 CMemory::AllocBuffer() における確保バッファサイズの調整記述を更新

beru dismissed berryzplus’s stale review via 4fa304e June 10, 2018 15:31

kobake closed this Jun 11, 2018

kobake reopened this Jun 11, 2018

kobake reviewed Jun 11, 2018

View reviewed changes

grep 処理速度改善

282b7b2

CMemory の _AppendSz と AppendRawData でメモリ再確保サイズを倍々にして再確保頻度を減らす対応

ds14050 reviewed Jun 12, 2018

View reviewed changes

berryzplus requested changes Jun 12, 2018

View reviewed changes

grep 処理速度改善

b5e4bc9

CMemory は変更しないように元に戻す CGrepAgent の変更コードのインデントスタイルを周りに合わせる

kobake added this to the next release milestone Jun 13, 2018

kobake approved these changes Jun 14, 2018

View reviewed changes

berryzplus approved these changes Jun 14, 2018

View reviewed changes

berryzplus merged commit b03ddd7 into sakura-editor:master Jun 14, 2018

beru deleted the grep_speed_up branch June 16, 2018 16:30

m-tmatma added the 🚅 speed up 🚀 高速化 label Jun 17, 2018

beru mentioned this pull request Jun 25, 2018

ファイル読み込みとgrepの高速化 #125

Merged

kobake mentioned this pull request Jun 30, 2018

Grep時に「GREP実行中」「処理の実行中」ダイアログが交互に出て操作不能になる #194

Closed

HoppingTappy pushed a commit to HoppingTappy/sakura that referenced this pull request Jun 11, 2019

Merge pull request sakura-editor#77 from beru/grep_speed_up

68bc303

grep 処理速度改善マージします。

beru mentioned this pull request Aug 11, 2019

「すべて置換」処理の高速化 #987

Merged

This pull request was closed.

grep 処理速度改善 #77

grep 処理速度改善 #77

Conversation

beru commented Jun 9, 2018

berryzplus left a comment • edited Loading

Choose a reason for hiding this comment

kobake commented Jun 10, 2018

beru commented Jun 10, 2018 • edited Loading

berryzplus left a comment

Choose a reason for hiding this comment

kobake commented Jun 10, 2018 • edited Loading

Choose a reason for hiding this comment

beru Jun 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

berryzplus Jun 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ds14050 left a comment

Choose a reason for hiding this comment

berryzplus Jun 12, 2018 • edited Loading

Choose a reason for hiding this comment

beru commented Jun 12, 2018

beru commented Jun 12, 2018 • edited Loading

ds14050 commented Jun 12, 2018 • edited Loading

beru commented Jun 12, 2018 • edited Loading

kobake commented Jun 13, 2018

kobake commented Jun 13, 2018

ds14050 commented Jun 13, 2018

kobake commented Jun 13, 2018

beru commented Jun 13, 2018

beru commented Jun 13, 2018

kobake commented Jun 13, 2018

kobake commented Jun 14, 2018

kobake commented Jun 14, 2018

kobake left a comment

Choose a reason for hiding this comment

kobake commented Jun 14, 2018

berryzplus left a comment

Choose a reason for hiding this comment

berryzplus left a comment •

edited

Loading

beru commented Jun 10, 2018 •

edited

Loading

kobake commented Jun 10, 2018 •

edited

Loading

beru Jun 10, 2018 •

edited

Loading

berryzplus Jun 12, 2018 •

edited

Loading

berryzplus Jun 12, 2018 •

edited

Loading

beru commented Jun 12, 2018 •

edited

Loading

ds14050 commented Jun 12, 2018 •

edited

Loading

beru commented Jun 12, 2018 •

edited

Loading