ファイル読み込み高速化(行レイアウト化処理のマルチスレッド対応) #1994

suconbu · 2025-02-16T16:37:34Z

PR対象

アプリ(サクラエディタ本体)

カテゴリ

改善

PR の背景

#1988 の第2弾対応です。

ファイル読み込み時の処理で一番時間の掛かっている CLayoutMgr::_DoLayout の処理時間の短縮を図ります。

CLayoutMgr::_DoLayout では、前段の処理にて改行単位で分割されたテキストデータを入力として、見た目上の行単位 (レイアウト行) に分割する処理を主にやっていますが、その部分を yoshinrt/SakuraVz の 34d5593d の対応を参考に複数のスレッドで並列実行できるようにします。

仕様・動作説明

対応内容：

CLayoutMgr::_DoLayout のマルチスレッド対応
CLayoutMgr::_DoLayout の実質の処理を CLayoutMgr::_DoLayoutSub (新規) に移し、それを複数スレッドから実行できるようにします。
CLayoutMgr::_MakeOneLine の高速化
ループ処理の手前で色分けや禁則関係の設定有無をあらかじめ確認、フラグに保持しておき、ループ内での不要な関数呼び出しをスキップします。
CPoolResource クラスの排他制御対応
対応「1」により、CLayoutMgr クラスが持つ1つのメモリプールを複数スレッドから同時に操作することになるため、CPoolResource の do_allocate, do_deallocate メソッドに対して std::mutex を使った排他制御を追加します。

4スレッドで処理する場合のイメージ：

参考元対応から変えた主な点：

行をまたぐ色分け設定 (/*...*/ など) のいずれかが有効の時はマルチスレッド処理を無効化
有効時はひとつ前の行の色分け状態を連鎖的に考慮する必要があり、色分け区間の途中で処理が分割されると正しく色分けできなくなる懸念があったため、設定有効時は従来通りシングルスレッドで処理させます。
(巨大となりがちなログファイルなどの拡張子に対して、行をまたぐ色分けの設定がされることはまれだろう・・・という想定とします)
(参考元ではマルチスレッド処理するのは一部機能制限があるとする「巨大ファイルモード」の場合に限定)
CLayoutMgr クラスにおけるメモリプール (CPoolResource) の使用を継続
排他制御を追加したとしてもメモリプールを使った方が直接 new/delete するよりも処理時間が短くなることが確認できたので引き続き使うことにしています。
(参考元では 4f5c0285 にてメモリプールを廃止)

各区間の処理時間 (計測条件などは #1988 を参照)：

区間	対応前 (`d9d3242`)	本PR対応後
A. CLoadAgent::OnLoad 開始から ReadFile_To_CDocLineMgr 完了まで	1528.936	1458.327
B. そこから CLoadAgent::OnLoad 完了まで (レイアウトデータ化処理)	4469.037	676.073
C. CLoadAgent::OnAfterLoad 開始から完了まで	9.104	16.088
合計	6007.077	2150.488 (-64%)

PR の影響範囲

巨大なファイルを読み込む時のCPU使用率が著しく増加しますが、その分処理時間は短くなるので消費電力量はほぼ変わらないと思います。

テスト内容

test.zip 内のファイルを使って以下を確認します。

「test_1000lines.txt」を開き、各行の数字が行番号と一致していること。👈レイアウト処理後のCLayoutMgrの集約が正しい順序でできていることの確認
「test_1000lines.cpp」を開き、ファイル先頭から末尾まで文字色がコメントアウト色 (緑色) になっていること。👈マルチスレッド処理の対象外の判定ができていることの確認

参考資料

AppVeyorBot · 2025-02-16T16:42:59Z

❌ Build sakura 1.0.4405 failed (commit 8c05729532 by @suconbu)

suconbu · 2025-02-16T16:45:16Z

sakura_core/doc/layout/CLayoutMgr_DoLayout.cpp


 	constexpr DWORD userInterfaceInterval = 33;
-	DWORD prevTime = GetTickCount() + userInterfaceInterval;
+	ULONGLONG prevTime = GetTickCount64() + userInterfaceInterval;


対応内容と関係ないですが、C28159 が指摘されていたので修正

(指摘外) ULONGLONG でなく auto で良いような。

あと「userInterfaceInterval = 33」を仮置きした基準を残すと「より良く」なるような。

「userInterfaceInterval = 33」を仮置きした基準を残す

userInterfaceInterval の意味/用途説明があった方が良いんじゃないか？ってことですかね？

よくよく考えると、prevTime の初期化で userInterfaceInterval を加算するのはおかしいような。
1回目は 66ms 間隔、2回目以降は 33ms 間隔となる。意図通りではなさそう。
(直したところで動作の違いは体感できないと思いますが)

AppVeyorBot · 2025-02-16T16:56:43Z

❌ Build sakura 1.0.4406 failed (commit 0544092c52 by @suconbu)

berryzplus · 2025-02-17T21:52:55Z

sakura_core/doc/layout/CLayoutMgr_DoLayout.cpp

-		if(_DoKinsokuSkip(pWork, pfOnLine)){ }
+		// 禁則処理中ならスキップする	@@@ 2002.04.20 MIK
+		// 禁則関係の設定が全部OFFの場合もスキップ
+		if(!bKinsokuEnabled || _DoKinsokuSkip(pWork, pfOnLine)){ }


(sonarcloud指摘)　「空の分岐は分かりづらいから、なんで空なのかコメントしようね」的なやつ。
メンテナンス性が上がりそうだけど対応は不要。

個人的には、条件を反転して直後の else とくっ付けるのが好み。

berryzplus · 2025-02-17T21:56:39Z

sakura_core/doc/layout/CLayoutMgr_DoLayout.cpp

-		//@@@ 2002.09.22 YAZAKI
-		color.CheckColorMODE( &pWork->pcColorStrategy, pWork->nPos, pWork->cLineStr );
+		if( bCheckColorEnabled ){
+			//@@@ 2002.09.22 YAZAKI


(指摘外)　こういうコメントを残すかどうか、悩ましいです。

「あると便利か？」を考えるとNoです。
「ないと困るか？」を考えるとNoです。

削ればスッキリしますが、著作権的にもやもやします。
残せば著作権には触れませんが、YAZAKIさんはここ書いてません。

berryzplus · 2025-02-17T22:01:10Z

sakura_core/doc/layout/CLayoutMgr_DoLayout.cpp

+	int nLineIndexEnd = nAllLineCount;
+	CDocLine* pDocLineEnd = nullptr;
+	for (int i = nWorkerThreadCount; i >= 0; i--) {
+		const int nLineIndexBegin = (int)((double)nAllLineCount / (nWorkerThreadCount + 1) * i);


(sonarcloud指摘) 必ずしも対応不要。

Suggested change

const int nLineIndexBegin = (int)((double)nAllLineCount / (nWorkerThreadCount + 1) * i);

const auto nLineIndexBegin = (int)((double)nAllLineCount / (nWorkerThreadCount + 1) * i);

berryzplus · 2025-02-17T22:06:08Z

sakura_core/doc/layout/CLayoutMgr_DoLayout.cpp


 	constexpr DWORD userInterfaceInterval = 33;
-	DWORD prevTime = GetTickCount() + userInterfaceInterval;
+	ULONGLONG prevTime = GetTickCount64() + userInterfaceInterval;


(指摘外) ULONGLONG でなく auto で良いような。

あと「userInterfaceInterval = 33」を仮置きした基準を残すと「より良く」なるような。

berryzplus · 2025-02-17T22:08:38Z

sakura_core/mem/CPoolResource.h

@@ -65,6 +65,8 @@ class CPoolResource : public std::pmr::memory_resource
 	void* do_allocate([[maybe_unused]] std::size_t bytes,
 					  [[maybe_unused]] std::size_t alignment) override
 	{
+		std::lock_guard<std::mutex> lock(mtx);


(sonarcloud指摘)　対応必須ではありませんが、メンテナンス性は上がりそう。

Suggested change

std::lock_guard<std::mutex> lock(mtx);

std::lock_guard lock(mtx);

berryzplus · 2025-02-17T22:09:31Z

sakura_core/mem/CPoolResource.h

@@ -91,6 +93,8 @@ class CPoolResource : public std::pmr::memory_resource
 					   [[maybe_unused]] std::size_t alignment) override
 	{
 		if (p) {
+			std::lock_guard<std::mutex> lock(mtx);


berryzplus · 2025-02-17T22:16:13Z

sakura_core/doc/layout/CLayoutMgr_DoLayout.cpp

+		if( nListenerCount != 0 && 0 < nLineCount ) {
+			if( nLineIndex == 0 ){
+				const ULONGLONG currTime = GetTickCount64();
+				const ULONGLONG diffTime = currTime - prevTime;


(指摘外)　prevTime を auto にするならここも。

berryzplus · 2025-02-17T22:18:00Z

sakura_core/mem/CPoolResource.h

@@ -140,5 +144,7 @@ class CPoolResource : public std::pmr::memory_resource
 	Node* m_unassignedNode = nullptr; // 未割当領域の先頭
 	Node* m_currentBlock = nullptr; // 現在のブロック
 	Node* m_currentNode = nullptr; // 要素確保処理時に現在のブロックの中から切り出すNodeを指すポインタ、メモリ確保時に未割当領域が無い場合はここを使う
+
+	std::mutex mtx;


(指摘外)　メンバー変数に「m_」プレフィックスを付けるかどうか、悩ましい。

気持ち悪いので付けマス

berryzplus · 2025-02-20T06:17:29Z

sakura_core/doc/layout/CLayoutMgr.h

@@ -292,6 +292,7 @@ class CLayoutMgr : public CProgressSubject
 	// 2005.11.21 Moca 引用符の色分け情報を引数から除去
 public:
 	void _DoLayout(bool bBlockingHook);	/* 現在の折り返し文字数に合わせて全データのレイアウト情報を再生成します */
+	void _DoLayoutSub(CDocLine* pDocLineBegin, CDocLine* pDocLineEnd, int nLineIndexBegin, int nLineCount, std::atomic<bool>* pbCanceled);


appveyor のビルドエラーは「 atomic が宣言されてない」というものです。
appveyor(vs2017 community edition) 固有のエラーなので、スルーしてよい気もします。

include <atomic> したら解決する可能性はあります。

AppVeyorBot · 2025-02-25T19:41:26Z

✅ Build sakura 1.0.4426 completed (commit 8e655ff590 by @suconbu)

AppVeyorBot · 2025-02-28T17:24:19Z

❌ Build sakura 1.0.4430 failed (commit a0f45bde03 by @suconbu)

sonarqubecloud · 2025-02-28T17:49:29Z

Quality Gate passed

Issues
3 New issues
0 Accepted issues

Measures
0 Security Hotspots
88.2% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

AppVeyorBot · 2025-02-28T18:13:34Z

✅ Build sakura 1.0.4431 completed (commit 5ebe925ffd by @suconbu)

AppVeyorBot · 2025-02-28T19:01:33Z

✅ Build sakura 1.0.4432 completed (commit c8e62dbcb8 by @suconbu)

suconbu · 2025-02-28T23:11:20Z

Acceptありがとうございました。
以下2か所を追加修正しましたので、お手数ですが再レビューお願いします。

ここにconst追加 (SonarCould指摘対応)
マルチスレッド処理のパスを通るようWinMainTest.runEditorProcessテストを修正 (コードカバレッジ対応)

berryzplus · 2025-03-01T13:29:40Z

sakura_core/doc/layout/CLayoutMgr.h

@@ -292,6 +293,7 @@ class CLayoutMgr : public CProgressSubject
 	// 2005.11.21 Moca 引用符の色分け情報を引数から除去
 public:
 	void _DoLayout(bool bBlockingHook);	/* 現在の折り返し文字数に合わせて全データのレイアウト情報を再生成します */
+	void _DoLayoutSub(CDocLine* pDocLineBegin, const CDocLine* pDocLineEnd, int nLineIndexBegin, int nLineCount, std::atomic<bool>* pbCanceled);


指摘外) std::atomic<bool>* pbCanceled のポインタ渡しについて。

参照渡し( std::atomic<bool>& isCanceled )としたほうが可読性、保守性をあげられそうに思いました。

コードベース全体ではポインタ渡しが主流で、[out] をあえて [out, opt] にしてるトコ多いので微妙な指摘になりますが。

pbCanceled については、以下の所で bBlockingHook の値によってnull or 有効値を意図的に使い分けるためにポインタ渡しとした次第でした。(代わりにフラグ＋参照でも良かったカモ)
https://github.com/sakura-editor/sakura/pull/1994/files#diff-34d5593d67d75e50e7eb3db6eba862a33a37d3e568a87a21d13970626dab8335R389

ポインタ渡しじゃなきゃならない理由が特になければ、参照渡しが良いだろうという意見には同意します。

suconbu · 2025-03-01T14:54:57Z

レビューありがとうございました。マージします。

suconbu commented Feb 16, 2025

View reviewed changes

ファイル読み込み高速化(行レイアウト化処理のマルチスレッド対応)

522f22c

suconbu force-pushed the feature/speedup_fileopen_multithread_layout branch from eff4747 to 522f22c Compare February 16, 2025 16:51

beru added the 🚅 speed up 🚀 高速化 label Feb 16, 2025

berryzplus previously approved these changes Feb 17, 2025

View reviewed changes

berryzplus reviewed Feb 20, 2025

View reviewed changes

suconbu dismissed berryzplus’s stale review via bd98a09 February 25, 2025 18:52

berryzplus previously approved these changes Feb 26, 2025

View reviewed changes

SonarCloud対応など

58bc67d

suconbu dismissed berryzplus’s stale review via 91e4c92 February 28, 2025 16:31

suconbu force-pushed the feature/speedup_fileopen_multithread_layout branch from bd98a09 to 91e4c92 Compare February 28, 2025 16:31

suconbu marked this pull request as ready for review February 28, 2025 16:42

suconbu force-pushed the feature/speedup_fileopen_multithread_layout branch from 91e4c92 to e1a90b4 Compare February 28, 2025 16:49

マルチスレッド処理のパスを通るようWinMainTest.runEditorProcessテストを修正

90f8474

suconbu force-pushed the feature/speedup_fileopen_multithread_layout branch from e1a90b4 to 90f8474 Compare February 28, 2025 17:24

berryzplus approved these changes Mar 1, 2025

View reviewed changes

berryzplus reviewed Mar 1, 2025

View reviewed changes

suconbu merged commit 9f9948d into sakura-editor:master Mar 1, 2025
25 checks passed

berryzplus mentioned this pull request Mar 19, 2025

半角文字が全角文字扱いされる + 全角文字が半角文字扱いされる #2022

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ファイル読み込み高速化(行レイアウト化処理のマルチスレッド対応) #1994

ファイル読み込み高速化(行レイアウト化処理のマルチスレッド対応) #1994

suconbu commented Feb 16, 2025 •

edited

Loading

AppVeyorBot commented Feb 16, 2025

suconbu Feb 16, 2025

berryzplus Feb 17, 2025

suconbu Feb 25, 2025

suconbu Feb 25, 2025

AppVeyorBot commented Feb 16, 2025

berryzplus Feb 17, 2025

berryzplus Feb 17, 2025

berryzplus Feb 17, 2025

berryzplus Feb 17, 2025

berryzplus Feb 17, 2025

berryzplus Feb 17, 2025

berryzplus Feb 17, 2025

berryzplus Feb 17, 2025

suconbu Feb 25, 2025

berryzplus Feb 20, 2025 •

edited

Loading

AppVeyorBot commented Feb 25, 2025

AppVeyorBot commented Feb 28, 2025

sonarqubecloud bot commented Feb 28, 2025

AppVeyorBot commented Feb 28, 2025

AppVeyorBot commented Feb 28, 2025

suconbu commented Feb 28, 2025

berryzplus Mar 1, 2025

suconbu Mar 1, 2025

suconbu commented Mar 1, 2025

	const int nLineIndexBegin = (int)((double)nAllLineCount / (nWorkerThreadCount + 1) * i);
	const auto nLineIndexBegin = (int)((double)nAllLineCount / (nWorkerThreadCount + 1) * i);

	std::lock_guard<std::mutex> lock(mtx);
	std::lock_guard lock(mtx);

ファイル読み込み高速化(行レイアウト化処理のマルチスレッド対応) #1994

ファイル読み込み高速化(行レイアウト化処理のマルチスレッド対応) #1994

Conversation

suconbu commented Feb 16, 2025 • edited Loading

PR対象

カテゴリ

PR の背景

仕様・動作説明

PR の影響範囲

テスト内容

関連 issue, PR

参考資料

AppVeyorBot commented Feb 16, 2025

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

AppVeyorBot commented Feb 16, 2025

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 Feb 20, 2025 • edited Loading

Choose a reason for hiding this comment

AppVeyorBot commented Feb 25, 2025

AppVeyorBot commented Feb 28, 2025

sonarqubecloud bot commented Feb 28, 2025

Quality Gate passed

AppVeyorBot commented Feb 28, 2025

AppVeyorBot commented Feb 28, 2025

suconbu commented Feb 28, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

suconbu commented Mar 1, 2025

suconbu commented Feb 16, 2025 •

edited

Loading

berryzplus Feb 20, 2025 •

edited

Loading